-
Notifications
You must be signed in to change notification settings - Fork 261
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
Update to Wasmtime 15.0.0, support multiple WASI snapshots #2108
Conversation
b896657
to
6baf8ee
Compare
let handler = match instance | ||
.exports(&mut store) | ||
.instance("wasi:http/[email protected]") | ||
{ | ||
Some(mut instance) => Some(Handler::Handler2023_10_18(IncomingHandler2023_10_18::new( | ||
&mut instance, | ||
)?)), | ||
None => None, | ||
}; | ||
let handler = match handler { | ||
Some(handler) => handler, | ||
None => Handler::Latest(Proxy::new(&mut store, &instance)?), | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let handler = match instance | |
.exports(&mut store) | |
.instance("wasi:http/[email protected]") | |
{ | |
Some(mut instance) => Some(Handler::Handler2023_10_18(IncomingHandler2023_10_18::new( | |
&mut instance, | |
)?)), | |
None => None, | |
}; | |
let handler = match handler { | |
Some(handler) => handler, | |
None => Handler::Latest(Proxy::new(&mut store, &instance)?), | |
}; | |
let handler = match instance | |
.exports(&mut store) | |
.instance("wasi:http/[email protected]") | |
{ | |
Some(mut instance) => Handler::Handler2023_10_18(IncomingHandler2023_10_18::new( | |
&mut instance, | |
)?), | |
None => Handler::Latest(Proxy::new(&mut store, &instance)?), | |
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I tried this originally yeah but that runs afoul of cannot borrow
store as mutable more than once at a time
where the instance.exports(...)
borrows the store
for the whole match
meaning that the Proxy::new
call can't be part of the same match
. Either that or I hand-wave and say "NLL isn't fully done yet and polonius will fix this" or something along those lines.
|
||
let handler = match instance | ||
.exports(&mut store) | ||
.instance("wasi:http/[email protected]") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure we want to handle non-"[email protected]" interfaces? Our public guarantee is that we only support "[email protected]" so we don't necessarily need to support newer versions. If we choose to not support the newer versions as first class, then we won't need to support N number of wasi-http interfaces in the future. We'll only need to make "[email protected]" work on the latest version of wasmtime-wasi-http.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do want to support newer version, yes. While our guarantees only cover maintaining compatibility with the 10-18 snapshot, the framing of our preview2 announcement explicitly mentions keeping up with development.
Additionally, we want to ensure that we and our customers can provide feedback on the latest version of preview2, not just the first one to get a snapshot.
Also, the idea is for the November snapshot to be very very close to final preview2, so we shouldn't need to do too much work after this to update to 0.2.0.
And finally, we'll need support for multiple versions after 0.2.0 anyway, so we didn't see a strong reason not to put things in place to do so right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One other thing I'd add here is that this also helps showcase decoupling "server upgrades" and "language sdk upgrades" because the server is upgraded here and the language SDKs can all migrated on their own time to newer worlds while Spin will continue to support all of them.
9713a70
to
73b5c5e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the new interface needs to be added here to actually let it work:
spin/crates/trigger-http/src/handler.rs
Lines 280 to 285 in 9e7a68d
const WASI_HTTP_EXPORT: &str = "wasi:http/[email protected]"; | |
impl HandlerType { | |
/// Determine the handler type from the exports | |
fn from_exports(mut exports: wasmtime::component::Exports<'_>) -> Option<HandlerType> { | |
if exports.instance(WASI_HTTP_EXPORT).is_some() { |
And the error message should be updated at
spin/crates/trigger-http/src/handler.rs
Line 53 in 9e7a68d
None => bail!("Expected component to either export `{}` or `fermyon:spin/inbound-http` but it exported neither", WASI_HTTP_EXPORT) |
2023-11-27T20:39:39.722863Z ERROR spin_trigger_http: Error processing request: Expected component to either export `wasi:http/[email protected]` or `fermyon:spin/inbound-http` but it exported neither
Ah good point! Supporting that fully though would probably best be done with a test, which would require updating the SDKs too, so could that perhaps be a follow-up PR? I'm happy to help out with that but I think it'd be useful to try to trial-run decoupling the two too |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK sure; I tried running my own 2023-11-10 thing on this PR and saw it break which I assumed was not intentional but as it shouldn't be breaking any existing components a followup seems fine.
Ok yeah makes sense, and I don't think we'd want to advertise that the latest snapshot works just yet, but following up after this to include that should fix things. |
This commit also provides an implementation of the 0.2.0-rc-2023-10-18 snapshot of WASI interfaces in terms of the latest interfaces of WASI, which is a newer version provided by Wasmtime 15. Signed-off-by: Alex Crichton <[email protected]>
73b5c5e
to
d58dc87
Compare
Adding a runtime test for this sounds like what we want - we should add a test component that is explicitly built with the newest snapshot. |
This builds upon Alex's excellent work in fermyon#2108, adding the ability to run guests which target the `0.2.0-rc-2023-11-10` version of `wasi-http`. I've added a test which uses `wit-bindgen` and `wit-component` directly to ensure it uses the correct snapshot for everything `wasi-http`, `wasi-cli`, etc. Note that we're not yet updating any SDKs to use the new snapshot, since we still want people to be able to use the new SDK to target Spin 2.0. Signed-off-by: Joel Dice <[email protected]>
* add support for `wasi:http/[email protected]` This builds upon Alex's excellent work in #2108, adding the ability to run guests which target the `0.2.0-rc-2023-11-10` version of `wasi-http`. I've added a test which uses `wit-bindgen` and `wit-component` directly to ensure it uses the correct snapshot for everything `wasi-http`, `wasi-cli`, etc. Note that we're not yet updating any SDKs to use the new snapshot, since we still want people to be able to use the new SDK to target Spin 2.0. Signed-off-by: Joel Dice <[email protected]> * use different names for `wasi-http-rust-rc-2023-11-10` module and component Previously, I tried to be clever and overwrite the module with the component, but that led to a filesystem-level race condition and was just generally fragile. Signed-off-by: Joel Dice <[email protected]> --------- Signed-off-by: Joel Dice <[email protected]>
This commit updates the Wasmtime crate to 15.0.0 from the previous fork which was 14.0.4 plus some patches which are all present in the 15.0.0 release. The major consequence of this update, however, is that WASI proposals upstream in the
wasmtime-wasi
crate have moved fromrc-2023-10-18
torc-2023-11-10
. This new release candidate has breaking changes relative to the prior release candidate such as:If this PR "only" updated the Wasmtime dependency then taht would mean that the previously supported snapshot,
rc-2023-10-18
, would no longer work. That would mean that preexisting binaries, plus the preexisting SDKs, would not work. To fix that this PR introduces the additional step of simultaneously supporting multiple WASI snapshots at the same time. This feature is only relevant until WASI reaches a stable release at which point support will likely move into Wasmtime itself for supporting multiple compatible releases. For now though leading up to that point in time Spin is the one which will need to support multiple incompatible releases.The general idea behind supporting multiple snapshots of WASI in this PR is to vendor all the WITs of the old version and generate bindings using the latest
wasmtime::component::bindgen!
macro. This generates a set of bindings and traits all targetting the old version of WASI which are distinct from the bindings in thewasmtime-wasi
crate. These traits are then all implemented, in Rust, in terms of the latest traits in thewasmtime-wasi
crate. This effectively provides a bridge between the two snapshots. Most functionality is a pass-through but more significant changes such as with UDP end up defining a custom resource just for this adaptation. The source of truth for each implementation still lives in thewasmtime-wasi
crate, however. This adaptation is then registered by adding the old snapshot namespace to awasmtime::component::Linker
in addition to the latest supported snapshot.Currently this adaptation layer is housed in one large file and is not exactly the prettiest thing to read or write. It's a pain to get working the first time but the hope is that maintenance over time won't be so bad. It's additionally also expected that this adaptation won't need to continue to be redone between WASI 0.2.0 and 0.2.1 for example, as that would be handled internally by Wasmtime.
Other changes in this PR are:
rc-2023-10-18
are added and vendored to generate full bindings.One major consequence of this PR at this time is that the SDKs are not updated to the newest snapshot of WASI. Not because there's a reason not to do that, moreso this is showcasing how the upgrades to the SDK don't have to happen in lockstep with Wasmtime. A future PR can vendor all the WITs for the latest snapshot and move the SDKs to the new WITs.