Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move HandlerType to spin-http #2863

Merged
merged 3 commits into from
Sep 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions crates/core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ use tracing::instrument;
use wasmtime::{InstanceAllocationStrategy, PoolingAllocationConfig};

pub use async_trait::async_trait;
pub use wasmtime::Engine as WasmtimeEngine;
pub use wasmtime::{
self,
component::{Component, Instance, InstancePre, Linker},
Expand Down
6 changes: 3 additions & 3 deletions crates/core/src/store.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use anyhow::Result;
use std::time::{Duration, Instant};

use crate::{limits::StoreLimitsAsync, State};
use crate::{limits::StoreLimitsAsync, State, WasmtimeEngine};

#[cfg(doc)]
use crate::EngineBuilder;
Expand Down Expand Up @@ -80,14 +80,14 @@ impl<T> wasmtime::AsContextMut for Store<T> {
///
/// A new [`StoreBuilder`] can be obtained with [`crate::Engine::store_builder`].
pub struct StoreBuilder {
engine: wasmtime::Engine,
engine: WasmtimeEngine,
epoch_tick_interval: Duration,
store_limits: StoreLimitsAsync,
}

impl StoreBuilder {
// Called by Engine::store_builder.
pub(crate) fn new(engine: wasmtime::Engine, epoch_tick_interval: Duration) -> Self {
pub(crate) fn new(engine: WasmtimeEngine, epoch_tick_interval: Duration) -> Self {
Self {
engine,
epoch_tick_interval,
Expand Down
10 changes: 10 additions & 0 deletions crates/factors-executor/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,16 @@ impl<'a, T: RuntimeFactors, U> FactorsInstanceBuilder<'a, T, U> {
pub fn factor_builder<F: Factor>(&mut self) -> Option<&mut F::InstanceBuilder> {
self.factor_builders().for_factor::<F>()
}

/// Returns the underlying wasmtime engine for the instance.
pub fn wasmtime_engine(&self) -> &spin_core::WasmtimeEngine {
self.instance_pre.engine()
}

/// Returns the compiled component for the instance.
pub fn component(&self) -> &Component {
self.instance_pre.component()
}
}

impl<'a, T: RuntimeFactors, U: Send> FactorsInstanceBuilder<'a, T, U> {
Expand Down
3 changes: 2 additions & 1 deletion crates/http/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,14 @@ anyhow = { workspace = true }
http = { workspace = true }
http-body-util = { workspace = true }
hyper = { workspace = true }
indexmap = "1"
indexmap = "2"
percent-encoding = "2"
routefinder = "0.5.4"
serde = { workspace = true }
spin-app = { path = "../app", optional = true }
spin-locked-app = { path = "../locked-app" }
tracing = { workspace = true }
wasmtime = { workspace = true }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should, I think, be under the runtime feature. We have consumers of this crate who use it for things like route parsing (e.g. the cloud plugin), but do not want to drag the whole of wasmtime along with it.

(I'm happy if you prefer to reorganise things to avoid the feature thing, but I really really want a way for cloud-plugin to get what it needs without including wasmtime. Whether that is features or separate crates, I don't care, but it makes a big difference!)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I just tried updating cloud-plugin and it looks like some of the other recebt changes have also brought in wasmtime dependency. I'll get a PR out for all of it together.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I did some more investigation and I'm gonna punt on it for now. The wasmtime dependency makes 1 min of difference to a clean cloud-plugin build, but not enough difference to an incremental build to be worth the candle, and we are churning cloud-plugin dependencies a lot more slowly than when it drove me to extremes. Something to maybe think about how we can streamline in a more sustainable way in future, but not worth fretting about for now.

Sorry for bending your ear @rylev!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a really good point, and something I would like to start getting a better handle on. Even though it doesn't make a huge difference, I can take a look at fixing this.

wasmtime-wasi-http = { workspace = true, optional = true }

[dev-dependencies]
Expand Down
65 changes: 65 additions & 0 deletions crates/http/src/trigger.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use serde::{Deserialize, Serialize};
use wasmtime::component::Component;

#[derive(Clone, Debug, Default, Deserialize, Serialize)]
#[serde(deny_unknown_fields)]
Expand All @@ -11,3 +12,67 @@ pub struct Metadata {
pub fn default_base() -> String {
"/".into()
}

/// The type of http handler export used by a component.
#[derive(Clone, Copy)]
pub enum HandlerType {
Spin,
Wagi,
Wasi0_2,
Wasi2023_11_10,
Wasi2023_10_18,
}

/// The `incoming-handler` export for `wasi:http` version rc-2023-10-18
pub const WASI_HTTP_EXPORT_2023_10_18: &str = "wasi:http/[email protected]";
/// The `incoming-handler` export for `wasi:http` version rc-2023-11-10
pub const WASI_HTTP_EXPORT_2023_11_10: &str = "wasi:http/[email protected]";
/// The `incoming-handler` export for `wasi:http` version 0.2.0
pub const WASI_HTTP_EXPORT_0_2_0: &str = "wasi:http/[email protected]";
/// The `incoming-handler` export for `wasi:http` version 0.2.1
pub const WASI_HTTP_EXPORT_0_2_1: &str = "wasi:http/[email protected]";
/// The `inbound-http` export for `fermyon:spin`
pub const SPIN_HTTP_EXPORT: &str = "fermyon:spin/inbound-http";

impl HandlerType {
/// Determine the handler type from the exports of a component.
pub fn from_component(
engine: &wasmtime::Engine,
component: &Component,
) -> anyhow::Result<HandlerType> {
let mut handler_ty = None;

let mut set = |ty: HandlerType| {
if handler_ty.is_none() {
handler_ty = Some(ty);
Ok(())
} else {
Err(anyhow::anyhow!(
"component exports multiple different handlers but \
it's expected to export only one"
))
}
};
let ty = component.component_type();
for (name, _) in ty.exports(engine) {
match name {
WASI_HTTP_EXPORT_2023_10_18 => set(HandlerType::Wasi2023_10_18)?,
WASI_HTTP_EXPORT_2023_11_10 => set(HandlerType::Wasi2023_11_10)?,
WASI_HTTP_EXPORT_0_2_0 | WASI_HTTP_EXPORT_0_2_1 => set(HandlerType::Wasi0_2)?,
SPIN_HTTP_EXPORT => set(HandlerType::Spin)?,
_ => {}
}
}

handler_ty.ok_or_else(|| {
anyhow::anyhow!(
"Expected component to export one of \
`{WASI_HTTP_EXPORT_2023_10_18}`, \
`{WASI_HTTP_EXPORT_2023_11_10}`, \
`{WASI_HTTP_EXPORT_0_2_0}`, \
`{WASI_HTTP_EXPORT_0_2_1}`, \
or `{SPIN_HTTP_EXPORT}` but it exported none of those"
)
})
}
}
62 changes: 2 additions & 60 deletions crates/trigger-http/src/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,14 @@ use spin_http::{
body,
config::{HttpExecutorType, HttpTriggerConfig},
routes::{RouteMatch, Router},
trigger::HandlerType,
};
use tokio::{
io::{AsyncRead, AsyncWrite},
net::TcpListener,
task,
};
use tracing::Instrument;
use wasmtime::component::Component;
use wasmtime_wasi_http::body::HyperOutgoingBody;

use crate::{
Expand Down Expand Up @@ -104,7 +104,7 @@ impl<F: RuntimeFactors> HttpServer<F> {
let handler_type = match &trigger_config.executor {
None | Some(HttpExecutorType::Http) => {
let component = trigger_app.get_component(component_id)?;
HandlerType::from_component(trigger_app.engine(), component)?
HandlerType::from_component(trigger_app.engine().as_ref(), component)?
}
Some(HttpExecutorType::Wagi(wagi_config)) => {
anyhow::ensure!(
Expand Down Expand Up @@ -464,61 +464,3 @@ pub(crate) trait HttpExecutor: Clone + Send + Sync + 'static {
client_addr: SocketAddr,
) -> impl Future<Output = anyhow::Result<Response<Body>>>;
}

/// Whether this handler uses the custom Spin http handler interface for wasi-http
#[derive(Clone, Copy)]
pub enum HandlerType {
Spin,
Wagi,
Wasi0_2,
Wasi2023_11_10,
Wasi2023_10_18,
}

pub const WASI_HTTP_EXPORT_2023_10_18: &str = "wasi:http/[email protected]";
pub const WASI_HTTP_EXPORT_2023_11_10: &str = "wasi:http/[email protected]";
pub const WASI_HTTP_EXPORT_0_2_0: &str = "wasi:http/[email protected]";
pub const WASI_HTTP_EXPORT_0_2_1: &str = "wasi:http/[email protected]";

impl HandlerType {
/// Determine the handler type from the exports of a component
pub fn from_component(
engine: impl AsRef<wasmtime::Engine>,
component: &Component,
) -> anyhow::Result<HandlerType> {
let mut handler_ty = None;

let mut set = |ty: HandlerType| {
if handler_ty.is_none() {
handler_ty = Some(ty);
Ok(())
} else {
Err(anyhow::anyhow!(
"component exports multiple different handlers but \
it's expected to export only one"
))
}
};
let ty = component.component_type();
for (name, _) in ty.exports(engine.as_ref()) {
match name {
WASI_HTTP_EXPORT_2023_10_18 => set(HandlerType::Wasi2023_10_18)?,
WASI_HTTP_EXPORT_2023_11_10 => set(HandlerType::Wasi2023_11_10)?,
WASI_HTTP_EXPORT_0_2_0 | WASI_HTTP_EXPORT_0_2_1 => set(HandlerType::Wasi0_2)?,
"fermyon:spin/inbound-http" => set(HandlerType::Spin)?,
_ => {}
}
}

handler_ty.ok_or_else(|| {
anyhow::anyhow!(
"Expected component to export one of \
`{WASI_HTTP_EXPORT_2023_10_18}`, \
`{WASI_HTTP_EXPORT_2023_11_10}`, \
`{WASI_HTTP_EXPORT_0_2_0}`, \
`{WASI_HTTP_EXPORT_0_2_1}`, \
or `fermyon:spin/inbound-http` but it exported none of those"
)
})
}
}
7 changes: 2 additions & 5 deletions crates/trigger-http/src/wasi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,13 @@ use spin_factor_outbound_http::wasi_2023_10_18::exports::wasi::http::incoming_ha
use spin_factor_outbound_http::wasi_2023_11_10::exports::wasi::http::incoming_handler as incoming_handler2023_11_10;
use spin_factors::RuntimeFactors;
use spin_http::routes::RouteMatch;
use spin_http::trigger::HandlerType;
use tokio::{sync::oneshot, task};
use tracing::{instrument, Instrument, Level};
use wasmtime_wasi_http::bindings::http::types::Scheme;
use wasmtime_wasi_http::{bindings::Proxy, body::HyperIncomingBody as Body, WasiHttpView};

use crate::{
headers::prepare_request_headers,
server::{HandlerType, HttpExecutor},
TriggerInstanceBuilder,
};
use crate::{headers::prepare_request_headers, server::HttpExecutor, TriggerInstanceBuilder};

/// An [`HttpExecutor`] that uses the `wasi:http/incoming-handler` interface.
#[derive(Clone)]
Expand Down
Loading