Lift http handler type discovery up a layer #2373
Conversation
alexcrichton
force-pushed
the
probe-handler-type-once
branch
from
fbb5859 to
4a022e1
Compare
crates/trigger-http/src/handler.rs
Outdated
| for (name, _) in ty.exports() { | ||
| match name { | ||
| WASI_HTTP_EXPORT_2023_10_18 => return Ok(HandlerType::Wasi2023_10_18), | ||
| WASI_HTTP_EXPORT_2023_11_10 => return Ok(HandlerType::Wasi2023_11_10), | ||
| WASI_HTTP_EXPORT_0_2_0 => return Ok(HandlerType::Wasi0_2), | ||
| "fermyon:spin/inbound-http" => return Ok(HandlerType::Spin), | ||
| _ => {} |
There was a problem hiding this comment.
Looks like the old code uses the same order, so I think it would behave the same in that (unusual) case.
There was a problem hiding this comment.
If I'm reading correctly, for an app that exports both spin and wasi interfaces the old code will always return wasi because it checks all exports for wasi first, while the new code will depend on the order of exports() (presumably the order of items in the binary).
There was a problem hiding this comment.
That's right yeah, I can preserve the exact same behavior as before if desired, but I figured that level of complexity probably wasn't what was intended here
There was a problem hiding this comment.
As long as tooling isn't "likely" to be emitting multiple of these exported interfaces I'm fine with the code as-is. I'm mostly just unsure about spin-componentize.
There was a problem hiding this comment.
I think we're unlikely to run into this situation with components built using Spin provided tooling (e.g., the SDKs). However, it does seem like we'd not want to rely on order of exports to determine which will be invoked, and indeed there should be a preferred ordering of wasi 0.2, wasi release candidates, Spin specific.
There was a problem hiding this comment.
I've pushed up a commit to generate an error if more than one handler is found to help resolve this
crates/core/src/lib.rs
Outdated
| // should probably be removed to just use that instead. | ||
| self.linker.substituted_component_type(component) | ||
| } | ||
|
|
||
| /// Creates a new [`ModuleInstancePre`] for the given [`Module`]. | ||
| #[instrument(skip_all)] |
There was a problem hiding this comment.
...though I'm not sure why this one is instrumented either... 🤔
There was a problem hiding this comment.
Ah it used to acquire a lock I was probably concerned about. Could probably be removed now. 🤷
alexcrichton
force-pushed
the
probe-handler-type-once
branch
from
bd5bfa8 to
d8cf70a
Compare
alexcrichton
force-pushed
the
probe-handler-type-once
branch
from
d8cf70a to
b2dfbf0
Compare
|
I lost this a bit in the shuffle of things, but @lann or others mind if I ask for a re-review on this? |
crates/core/src/lib.rs
Outdated
| // NB: With Wasmtime 20 and bytecodealliance/wasmtime#8078 this method | ||
| // should probably be removed to just use that instead. |
There was a problem hiding this comment.
Perhaps we can land #2512 and then remove this?
crates/trigger-http/src/handler.rs
Outdated
| drop(exports); | ||
| Handler::Latest(Proxy::new(&mut store, &instance)?) | ||
| } | ||
| HandlerType::Spin => unreachable!(), |
There was a problem hiding this comment.
nit: I prefer using a panic with a message just in case (through some refactoring) this becomes technically reachable.
This commit lifts the discovery of what type of handler a component is, for example spin-vs-wasi-vs-wasi-snapshot, up one layer from where it currently resides. Previously this determination was made on each request and now it's made when a component first loaded and pre-instantiated instead. This would shift possible errors earlier in the system, for example. This removes a bit of per-request work and makes the creation of the handler in `execute_wasi` a bit more straightforward with less guessing of exports for something that was already probed. Signed-off-by: Alex Crichton <[email protected]>
alexcrichton
force-pushed
the
probe-handler-type-once
branch
from
b2dfbf0 to
e565b78
Compare
This commit lifts the discovery of what type of handler a component is, for example spin-vs-wasi-vs-wasi-snapshot, up one layer from where it currently resides. Previously this determination was made on each request and now it's made when a component first loaded and pre-instantiated instead. This would shift possible errors earlier in the system, for example.
This removes a bit of per-request work and makes the creation of the handler in
execute_wasia bit more straightforward with less guessing of exports for something that was already probed.