trigger crate

[lann]

Spin currently has two built-in trigger types: HttpTrigger and RedisTrigger. There is also a TimerTrigger example and ongoing work towards new cron / "Schedule" (#365) and "CloudEvents" (#394) triggers.

These triggers - especially the two built-in types - share some common logic in how they prepare and invoke their handler components. In order to reduce duplication and future divergence, we should extract that commonality into a new trigger crate that exposes trait(s) that can be implemented by individual trigger types.

[Mossaka]

I can create a trigger trait. This will help me to work on the CloudEvents trigger feature

[Mossaka]

I am thinking about merging the two triggers, HTTP and Redis, to one crate called spin-trigger. The spin-trigger crate exposes a Trigger trait

#[async_trait]
pub trait Trigger<T> {
    pub async fn new(
        builder: Builder<T>,
        app: Application<CoreComponent>,
        config: TriggerConfig<T>,
    ) -> Result<Self>;
    async fn run(&self) -> Result<()>;
}

and changes how we import triggers:

use spin_trigger::{HttpTrigger, RedisTrigger, CloudEventTrigger};
use spin_engine::{Builder, ExecutionContextConfiguration};

fn main() {
  ...
  let builder = self.prepare_ctx_builder(app.clone()).await?;
  let trigger = HttpTrigger::new(builder, app, self.opts.address, tls).await?;
  trigger.run().await?;

}

The benefits of doing this are

1. easy to author a trigger
2. easy to import a trigger
3. author a trigger

Let me know your thoughts @lann @radu-matei

[radu-matei]

I agree with having a crate that contains the common logic between the triggers, but not sure with merging our triggers into this crate.
Rather, I would expect the existing triggers to remain separate crates that implement the trait from the new one and import the common logic.

Unless you are thinking about something else with merging the existing triggers?

What do you think?

[lann]

I haven't put a ton of thought into this yet, but I was vaguely thinking that the trigger crate would consist of one or more traits to define the trigger interface plus some common logic to be shared between (most) triggers. Specifically, I'd be happy if we could move this:

spin/src/commands/up.rs

Lines 158 to 172 in ad51cdd

	async fn prepare_ctx_builder<T: Default + 'static>(
	&self,
	app: Application<CoreComponent>,
	) -> Result<Builder<T>> {
	let config = ExecutionContextConfiguration {
	log_dir: self.opts.log.clone(),
	..app.into()
	};
	let mut builder = Builder::new(config)?;
	builder.link_defaults()?;
	builder.add_host_component(wasi_outbound_http::OutboundHttpComponent)?;
	builder.add_host_component(outbound_redis::OutboundRedis)?;
	Ok(builder)
	}
	}

Ideally we could also come up with generic versions of some of these (and anything else that makes sense!):

spin/crates/http/src/lib.rs

Lines 70 to 82 in ad51cdd

	let trigger_config = app
	.info
	.trigger
	.as_http()
	.ok_or_else(|| anyhow!("Application trigger is not HTTP"))?
	.clone();
	let component_triggers = app.component_triggers.try_map_values(|id, trigger| {
	trigger
	.as_http()
	.cloned()
	.ok_or_else(|| anyhow!("Expected HTTP configuration for component {}", id))
	})?;

spin/crates/http/src/lib.rs

Lines 225 to 234 in ad51cdd

	let shutdown_signal = on_ctrl_c()?;
	tokio::select! {
	_ = server => {
	log::debug!("Server shut down: exiting");
	},
	_ = shutdown_signal => {
	log::debug!("User requested shutdown: exiting");
	},
	};

spin/crates/http/src/lib.rs

Lines 327 to 336 in ad51cdd

	fn on_ctrl_c() -> Result<impl std::future::Future<Output = Result<(), tokio::task::JoinError>>> {
	let (tx, rx) = std::sync::mpsc::channel::<()>();
	ctrlc::set_handler(move || {
	tx.send(()).ok();
	})?;
	let rx_future = tokio::task::spawn_blocking(move || {
	rx.recv().ok();
	});
	Ok(rx_future)
	}

[lann]

I'd also be happy if we could better decouple triggers from the Application struct, but that might make more sense as part of a config refactor.

[Mossaka]

Thanks for the pointers @lann @radu-matei

I was looking at the http trigger and redis trigger impl and didn't find too many common ground. Their usage of handle is totally different, so that's why I was thinking the Trigger trait might not do too much.

But @lann 's code blocks make sense. I will make a draft PR today and let you to review it.

[Mossaka]

Ideally we could also come up with generic versions of some of these (and anything else that makes sense!):

I am trying to define ApplicationConfiguration to be a generic type over the trigger type, like ApplicationConfiguration<Trigger>. In fact, the whole Application struct could be Application<Trigger> since there could only be either Http Application or Redis Application. Then I found that I need to convert the enum (from toml) to generic struct... which is pretty difficult to do.

[lann]

which is pretty difficult to do.

No problem! We don't need to boil the ocean all at once here.

[lann]

@Mossaka I have time today to take a look at this; if you would like we could work together on supporting what you need for a CloudEvents trigger?

[lann]

I think something like this should be feasible: https://github.com/fermyon/spin/pull/415/files#diff-52052d5f095ea9072b1d41ab7754a0b4479f7340f8bedd9da41f8575f380150e

[lann]

This could be relevant, depending on timing: #417

[lann]

Fixed by #418