Skip to content
New issue

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

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

Already on GitHub? # to your account

Implement async witx functions for a Wasmtime host #82

Closed

Conversation

alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Oct 1, 2021

This PR is an implementation of async witx functions for Wasmtime. This is a significantly different design than what's present today for sync bindings or sync-wasm-but-actually-invoked-async bindings. Instead the support for async function in witx will spawn a "reactor task" onto tokio which manages the Store<T> and similarly manages running callbacks. The "callback ABI" is all that's implemented here at this time.

This "reactor task" takes ownership of the Store<T> from the outside and the bindings structure no longer takes &mut StoreContextMut anywhere since the caller won't have it. Instead the bindings structure communicates with the reactor task via a channel, and everything otherwise happens asynchronously. Some properties of this implementation include:

  • Great care is taken to handle "cancellation" correctly in Rust-futures-world where if the original request to execute wasm is dropped (the channel is disconnected) this will immediately abort any executing WebAssembly and propagate the cancellation to all callbacks as well.
  • If wasm is ever "interrupted" for one reason or another -- e.g. via a cancelled future or a trap -- then the entire reactor task is "torn down" which destroys the Store<T>. This cancels any other possible concurrent execution of wasm. This is intended to model kill -9 in a sense but also reflect how after a trap it's not really feasible to resume wasm execution since it's all in an indeterminate state.
  • Some properties of the witx async model are refined, such as:
    • If the completion callback is called and then a trap happens afterwards, that's translate as a trap
    • New "event" intrinsics were added to support cross-coroutine wakeups and the Rust guest is based on this
    • Coroutines are not considered "done" until all of their pending callbacks have been resolved.
  • Support for "synchronous" witx functions remains with Wasmtime, even when async functions are present, but due to the nature of running code in a separate task this is still exposed to the host in an async function with asynchronous communication with the reactor task.
  • There is a period of time that passed when the wasm calls a completion callback until the wasm has actually fully completed. This period of time is not exposed in the APIs generated for the Wasmtime host (or the JS host). Instead functions from wasm are simply exposed as async function() -> T effectively which means that if the completion callback is called significantly further before the coroutine finishes the caller of the API will not know of this.
  • Currently the notion of "async" in the wasmtime code generator is a bit wonky. There's a 2x2 matrix here where one axis is whether the witx function is async or not, and the other axis is whether wasmtime is configured to invoke the function asynchronously or not (using Wasmtime's async fn support natively). One gap here of async witx function invoked synchronously in Wasmtime is not supported, and if configured that way the generated code either won't compile or won't work at runtime, I didn't test this too thoroughly.

This PR continues to have no support for streams. @aturon and I are still discussing a lot of implementation details and implications, so that'll all be a separate follow-up PR.

This commit is the initial implementation of `async function()` in witx
for the Wasmtime host runtime. This integration is unfortunately not as
nice as the previous JS implementation, but the general goal is to hook
up async witx functions to `async` functions in Rust in an at least
somewhat of an idiomatic fashion.

For exported functions not much has changed relative to the preexisting
`async` support for Wasmtime. Functions are still exposed as `async`
functions that take a store and the arguments as input. The major
consequence of this is that only one async method call on a wasm module
can be active at any point in time. The alternative to this design is to
embed the `Store<T>` within the bindings and have some sort of async
mutex around it, but that doesn't feel appropriate at this time since
it's not really how Wasmtime's idioms are designed (where you're
typically given a store rather than embedding it).

For imported functions, however, the implementation is significantly
different from the previous `async` support for Wasmtime. The support
no longer uses `async_trait` for the reason of being able to call
multiple imports simultaneously. This means that imports receive their
arguments and return `'static` futures. This does not map to any
supported mode of `async_trait`. This means that host bindings for these
sorts of functions are likely going to be somewhat unidiomatic since
they'll require cloning state into host callbacks as necessary and/or
using things like `Rc<RefCell<_>>` or similar.

Overall I'm not super happy with how this ended up. I feel that it's
generally pretty un-ergonomic and confusing to use. Unfortunately though
I don't really know how to make it better at this time. On the plus side
it at least all works for the given use cases. There's almost surely a
bug or two with the generated code but this should be at least a
somewhat solid base to build on as we tweak things in the future.
* Associated types need `'static` to live across a captured future
* Caller-source data is rebound when we reenter after the caller is
  available again.
This is a very large number of updates which are borne out of recent
discussions about the async model and how best to map it to Wasmtime.
This resulted in a complete rearchitecture of Wasmtime's support for
`async function` where the store is now owned by a "reactor" task and
bindings are specifically for Tokio. More tests have been added,
particularly around various failure conditions, and the JS async
function bindings were also updated to handle more errors.
Enables cross-coroutine wakeups primarily. Downside is that the Rust
implementation now "bounces" off an import completion callback back onto
an event completion callback.
@alexcrichton alexcrichton requested a review from aturon November 1, 2021 14:46
@alexcrichton
Copy link
Member Author

Ok for reviewing this I'm going to tag in @aturon. We've been talking a lot about this offline and this design has changed significantly from when I first opened this to what it is here today.

@aturon for you and reviewing this I don't think it makes much sense for you to review the code generator but rather the generated code itself is probably more interesting. To that end for reviewing things I'd ask for double-checks on:

  • crates/wasmtime/src/futures.rs - the wasmtime host runtime helper support for async functions
  • crates/rust-wasm/src/futures.rs - the compiled-to-wasm runtime helper support for async functions
  • tests/runtime/{async_functions,async_raw}/* - tests for async functions. The exports.witx files in these directories are what the wasm must export and the imports.witx files are what the host must implement. Both of these directories also have host.ts if you're interested in peeking at the JS host implementations here.

Some other tools which might be helpful in exploring generated code:

  • If you run ./crates/witx-bindgen-demo/build.sh (from the root of the repo) that'll produce a static directory which is the compiled demo but with async support for wasmtime
  • Otherwise you can also run cargo run wasmtime --import path/to/imports.witx or similarly --export path/to/exports.witx to see the generated code. Note that in this context --import is "imported from wasm" so it's wasm-exported fucntions, and --export is "exported to wasm" which is host-provided functions that wasm can export.

Now that I think about it I think I need to update the PR description as well...

@tailhook
Copy link
Contributor

tailhook commented Feb 9, 2022

Any plans to carry on this work?

I want to play with async wasm in my pet project. And I'm thinking I could implement some poll functions at the API boundary to make this work instead of using native async bindings.

But if this could be continued, I probably may be able to test this instead. Unfortunately, I'm not familiar with the codebase enough to review or rebase this branch, though.

@alexcrichton
Copy link
Member Author

There are indeed! I'm still actively working with other folks around the async story and interface types. Most work is internal for now as it's still heavily in the design phase, but I think that we'll be bringing ideas and proposals to the WASI CG in the near-ish future.

For now though the current async support in wit-bindgen is unlikely to get developed further as we have discovered a number of limitations with it and a desire to make it more flexible as well. Ideas from this PR and work have inspired future development but it'll be a bit before all the new support can land here I think.

@alexcrichton
Copy link
Member Author

I'm going to close this since async as-is is being removed in #307 anyway and this is all likely to significantly change as well.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants