wasmtime / PR #7106 Async support in the C API · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #7106 Async support in the C API

Wasmtime GitHub notifications bot (Sep 28 2023 at 19:20):

rockwotj opened PR #7106 from rockwotj:async to bytecodealliance:main:

This patch set extends the C API to allow for async execution of WebAssembly as well as async host calls to suspend the VM.

The APIs where mostly follow the rust implementation in places, but I am happy to take suggestions for alternatives in places.

Additionally, I wrote the async example in C++ because that is easier to get working cross platform and do things like spawn another thread to execute the host functions easier, I'm not sure if having C++ in the examples is desirable, but I could possibly simplify the example if we wanted to keep things in C.

Fixes: #3111
Fixes: #6277

Wasmtime GitHub notifications bot (Sep 28 2023 at 19:20):

rockwotj requested wasmtime-core-reviewers for a review on PR #7106.

Wasmtime GitHub notifications bot (Sep 28 2023 at 19:20):

rockwotj requested alexcrichton for a review on PR #7106.

Wasmtime GitHub notifications bot (Sep 28 2023 at 19:20):

rockwotj requested wasmtime-default-reviewers for a review on PR #7106.

Wasmtime GitHub notifications bot (Sep 28 2023 at 19:24):

rockwotj updated PR #7106.

Wasmtime GitHub notifications bot (Sep 28 2023 at 19:28):

rockwotj updated PR #7106.

Wasmtime GitHub notifications bot (Sep 28 2023 at 19:46):

rockwotj updated PR #7106.

Wasmtime GitHub notifications bot (Sep 28 2023 at 19:52):

rockwotj updated PR #7106.

Wasmtime GitHub notifications bot (Sep 29 2023 at 03:29):

rockwotj updated PR #7106.

Wasmtime GitHub notifications bot (Sep 29 2023 at 04:07):

rockwotj edited PR #7106:

This patch set extends the C API to allow for async execution of WebAssembly as well as async host calls to suspend the VM.

The APIs here mostly follow the rust implementation, but I am happy to take suggestions for alternatives.

Additionally, I wrote the async example in C++ because that is easier to get working cross platform and do things like spawn another thread to execute the host functions easier, I'm not sure if having C++ in the examples is desirable, but I could possibly simplify the example if we wanted to keep things in C.

Fixes: #3111
Fixes: #6277

Wasmtime GitHub notifications bot (Sep 29 2023 at 14:52):

alexcrichton submitted PR review:

Thanks for this! I've written down some thoughts on the API and documentation as well, but please let me know if anything is confusing or doesn't feel actionable!

Wasmtime GitHub notifications bot (Sep 29 2023 at 14:52):

alexcrichton submitted PR review:

Thanks for this! I've written down some thoughts on the API and documentation as well, but please let me know if anything is confusing or doesn't feel actionable!

Wasmtime GitHub notifications bot (Sep 29 2023 at 14:52):

alexcrichton created PR review comment:

Given that all this function does is box up the above structure which already has public fields, I think it'd be ok to drop this function entirely. Instead it can be replaced with manual management of the structure in C/C++

Wasmtime GitHub notifications bot (Sep 29 2023 at 14:52):

alexcrichton created PR review comment:

From an organizational perspective, what do you think about putting all of these functions in a new wasmtime/async.h header? Ideally we'd go as far as generating wasmtime/__config__.h or something which has #define WASMTIME_ASYNC 0 in non-async builds of the C API, but we don't need to get that fancy yet. In the meantime though having what is almost a conditional header might be a good way to mitigate that in the meantime.

Wasmtime GitHub notifications bot (Sep 29 2023 at 14:52):

alexcrichton created PR review comment:

Could this, and other async functions, be listed as safe throughout the Rust crate? For example the main reason for the unsfaety here is the results and args raw pointers but that can be mitigated one layer up where wasmtime_func_call_async would perform the slice_from_raw_parts calls, then passing safe slices to the async pieces here.

I ask mainly because mixing async and unsafe is a recipe for bad things to happen, particularly around the lifetimes of arguments and such.

Wasmtime GitHub notifications bot (Sep 29 2023 at 14:52):

alexcrichton created PR review comment:

This isn't quite right since the main result is transmitting through the instance pointer I think? Given my suggestion above though it would mean that all results are transmitted through provided pointers, too.

Wasmtime GitHub notifications bot (Sep 29 2023 at 14:52):

alexcrichton created PR review comment:

To bikeshed this API slightly, I think it might help using this to perhaps refactor this slightly. What do you think about removing wasmtime_call_future_get_results and only having wasmtime_call_future_poll? That way the polling method is basically a test of "is this done yet".

The main purpose of wasmtime_call_future_get_results it seems is to communicate the error, but I think this would work well if the future in Rust was Output = () and then APIs such as wasmtime_linker_instantiate_async take a wasmtime_error_t** as a pointer to fill in with the error, if any.

Wasmtime GitHub notifications bot (Sep 29 2023 at 14:52):

alexcrichton created PR review comment:

This I think warrants a bit of expansion and more explanation. This might be another good use case for an async.h header file which could have a dedicated comment at the top discussing how async works.

All arguments to async functions are "captured" for the entire duration of the future. They must stay valid and no other modifications are allowed during the lifetime of the future, or it's UB. This is a guarantee to wasmtime_func_async_callback_t provided by Wasmtime, but also something embedders must guarantee when calling APIs such as wasmtime_linker_instantiate_async

One point to emphasize is that this includes store, meaning that concurrent invocations of futures are not allowed. Instead only one future per store can be active at any one point in time. My guess is that many people may get this wrong by accident so it's definitely something I want called out in the documentation. Ideally there'd be a double-check in the C API which sets/clears a flag and panics if the flag is cleared indicating that a future is active, but that's ok to do as a follow-up.

For wasmtime_linker_define_async_func it's worth noting that the native code defined by cb will not be invoked on the current stack but will be invoked on a separate stack (and a separate fiber on Windows). This can possibly be important for some embeddings.

Basically I think it's ok for the docs on individual functions to be a bit terse, but there are a huge number of gotchas with getting async right in C. There's no protection lifetime-wise when it's most critically required for async so I'd want to make sure that we at least have some longer-form documentation explaining some of the hazards and what embedders need to consider when designing their own async support. If you're up for starting the documentation I can try to help fill it in too.

Wasmtime GitHub notifications bot (Sep 29 2023 at 14:52):

alexcrichton created PR review comment:

One thing that worries me about this is that there's an implicit contract that while this future is alive it cannot access hostcall_val_storage because that's been handed out to C and it is expected to retain pointers to it. Within the poll method, however, there's nothing stopping the impementation from accessing this variable.

I'm trying to think of how to best restructure this to best handle this, but nothing feels like a slam dunk to me. That being said, how would you feel about:

Minmizing the size fo this CHostCallFuture to basically dealing with wasmtime_async_continuation_t. (e.g. you may be able to get away with impl Future for wasmtime_async_continuation_t or something like that.

Moving the bulk of this function to an async fn. The async fn would take as arguments most of the struct fields here, which would guarantee they're valid for the lifetime of the function.

Within the async fn there'd be a .await on wasmtime_async_continuation_t from above (or the equivalent thereof). This would prevent access to the local variables while the function is awaiting.

That feels enough to me to prevent footguns for now, despite it not being 100% bulletproof.

Wasmtime GitHub notifications bot (Sep 29 2023 at 14:52):

alexcrichton created PR review comment:

And additionally I think it might be good to place all the Rust code in a crates/c-api/src/async.rs or similar.

Wasmtime GitHub notifications bot (Sep 29 2023 at 15:03):

rockwotj submitted PR review.

Wasmtime GitHub notifications bot (Sep 29 2023 at 15:03):

rockwotj created PR review comment:

Yeah happy to move all this to a dedicated header and source file.

Wasmtime GitHub notifications bot (Sep 29 2023 at 15:03):

rockwotj submitted PR review.

Wasmtime GitHub notifications bot (Sep 29 2023 at 15:03):

rockwotj created PR review comment:

To be clear, you're talking about moving the function and linker methods into this new header too?

Wasmtime GitHub notifications bot (Sep 29 2023 at 15:03):

rockwotj submitted PR review.

Wasmtime GitHub notifications bot (Sep 29 2023 at 15:03):

rockwotj created PR review comment:

Sounds good.

Wasmtime GitHub notifications bot (Sep 29 2023 at 15:15):

alexcrichton created PR review comment:

Indeed yeah I think that'd be a good way to organize, although if you feel differently I don't feel strongly so it's ok to leave as-is too

Wasmtime GitHub notifications bot (Sep 29 2023 at 15:15):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Sep 29 2023 at 15:45):

rockwotj submitted PR review.

Wasmtime GitHub notifications bot (Sep 29 2023 at 15:45):

rockwotj submitted PR review.

Wasmtime GitHub notifications bot (Sep 29 2023 at 15:45):

rockwotj created PR review comment:

Sounds good, so the API for the new async functions would be as follows?

WASM_API_EXTERN wasmtime_call_future_t* wasmtime_func_call_async(
    wasmtime_context_t *context,
    const wasmtime_func_t *func,
    const wasmtime_val_t *args,
    size_t nargs,
    wasmtime_val_t *results,
    size_t nresults,
    wasmtime_error_t** error,
    wasm_trap_t** trap);

WASM_API_EXTERN wasmtime_call_future_t *wasmtime_linker_instantiate_async(
    const wasmtime_linker_t *linker,
    wasmtime_context_t *store,
    const wasmtime_module_t *module,
    wasmtime_instance_t *instance,
    wasmtime_error_t** error,
    wasm_trap_t** trap);

Wasmtime GitHub notifications bot (Sep 29 2023 at 15:59):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Sep 29 2023 at 15:59):

alexcrichton created PR review comment:

Yeah that looks good to me :+1:

Wasmtime GitHub notifications bot (Sep 29 2023 at 15:59):

alexcrichton edited PR review comment.

Wasmtime GitHub notifications bot (Sep 29 2023 at 16:25):

rockwotj submitted PR review.

Wasmtime GitHub notifications bot (Sep 29 2023 at 16:25):

rockwotj created PR review comment:

I actually wanted to do this at first so happy to update :)

Wasmtime GitHub notifications bot (Sep 29 2023 at 17:00):

rockwotj updated PR #7106.

Wasmtime GitHub notifications bot (Sep 29 2023 at 17:00):

rockwotj submitted PR review.

Wasmtime GitHub notifications bot (Sep 29 2023 at 17:00):

rockwotj created PR review comment:

Done!

Wasmtime GitHub notifications bot (Sep 29 2023 at 17:01):

rockwotj edited PR review comment.

Wasmtime GitHub notifications bot (Sep 29 2023 at 17:01):

rockwotj submitted PR review.

Wasmtime GitHub notifications bot (Sep 29 2023 at 17:01):

rockwotj created PR review comment:

Thanks yes this is better as I'm wrapping my head around async + unsafe in Rust :big_smile: done.

Wasmtime GitHub notifications bot (Sep 29 2023 at 17:02):

rockwotj submitted PR review.

Wasmtime GitHub notifications bot (Sep 29 2023 at 17:02):

rockwotj created PR review comment:

Okay I took a stab at this - I wasn't able to figure out how to make wasmtime_async_continuation_t a future without remove the caller parameter, but maybe that should follow suit of the results and params args and just make the caller keep a reference to that too.

Wasmtime GitHub notifications bot (Sep 29 2023 at 17:10):

rockwotj submitted PR review.

Wasmtime GitHub notifications bot (Sep 29 2023 at 17:10):

rockwotj created PR review comment:

Okay I did that and see what you were saying. Nice suggestion!

Wasmtime GitHub notifications bot (Sep 29 2023 at 17:36):

rockwotj submitted PR review.

Wasmtime GitHub notifications bot (Sep 29 2023 at 17:36):

rockwotj created PR review comment:

Ideally there'd be a double-check in the C API which sets/clears a flag and panics if the flag is cleared indicating that a future is active, but that's ok to do as a follow-up.

Not that my embedding does this - but is it safe to call back into the VM from a async host function? That maybe a reason to not add this check.

I took a stab at the documentation. I will admit documentation is not my strong suit so help is appreciated (or I'm happy to rubber duck/review a followup PR on improving the documentation).

Wasmtime GitHub notifications bot (Sep 29 2023 at 17:37):

rockwotj updated PR #7106.

Wasmtime GitHub notifications bot (Sep 29 2023 at 17:39):

rockwotj updated PR #7106.

Wasmtime GitHub notifications bot (Sep 29 2023 at 17:40):

rockwotj submitted PR review.

Wasmtime GitHub notifications bot (Sep 29 2023 at 17:40):

rockwotj created PR review comment:

Correct - I've updated these docs a bit.

Wasmtime GitHub notifications bot (Sep 29 2023 at 17:40):

rockwotj requested alexcrichton for a review on PR #7106.

Wasmtime GitHub notifications bot (Sep 29 2023 at 17:43):

rockwotj updated PR #7106.

Wasmtime GitHub notifications bot (Sep 29 2023 at 18:02):

rockwotj updated PR #7106.

Wasmtime GitHub notifications bot (Sep 30 2023 at 16:11):

rockwotj submitted PR review.

Wasmtime GitHub notifications bot (Sep 30 2023 at 16:11):

rockwotj created PR review comment:

Does it make more sense for this error_ret to be a parameter of the initial host function call? Then this becomes the mirror to poll which just returns a boolean.

Wasmtime GitHub notifications bot (Oct 01 2023 at 01:02):

rockwotj updated PR #7106.

Wasmtime GitHub notifications bot (Oct 01 2023 at 01:03):

rockwotj submitted PR review.

Wasmtime GitHub notifications bot (Oct 01 2023 at 01:03):

rockwotj created PR review comment:

I went ahead and did this - I quite like how it turned out.

Wasmtime GitHub notifications bot (Oct 01 2023 at 01:04):

rockwotj updated PR #7106.

Wasmtime GitHub notifications bot (Oct 01 2023 at 02:40):

rockwotj updated PR #7106.

Wasmtime GitHub notifications bot (Oct 02 2023 at 00:56):

rockwotj updated PR #7106.

Wasmtime GitHub notifications bot (Oct 02 2023 at 17:32):

alexcrichton submitted PR review:

Looks great to me, thank you again for working on this!

I've got some minor comments but otherwise looks good to go

Wasmtime GitHub notifications bot (Oct 02 2023 at 17:32):

alexcrichton submitted PR review:

Looks great to me, thank you again for working on this!

I've got some minor comments but otherwise looks good to go

Wasmtime GitHub notifications bot (Oct 02 2023 at 17:32):

alexcrichton created PR review comment:

Instead of returning wasmtime_async_continuation_t could that perhaps be an out-ptr? That way the contract of this function is that it always fills in wasmtime_async_continuation_t and then must fill in trap_ret and results by the time the continuation yields "I'm done"?

Wasmtime GitHub notifications bot (Oct 02 2023 at 17:32):

alexcrichton created PR review comment:

As a small rust-ism the mem::take isn't required here and you can say hostcall_val_storage.truncate(0) and put it back into the store

Wasmtime GitHub notifications bot (Oct 02 2023 at 17:32):

alexcrichton created PR review comment:

Mind combining this clause with the above one so the three cases of suspension are enumerated as a set?

Wasmtime GitHub notifications bot (Oct 02 2023 at 17:32):

alexcrichton created PR review comment:

In lieu of this though there are other issues - for example in Rust this is represented in the return value as a Box<T> but that's not guaranteed to use the same allocator as new uses in C++. By using a retptr it would help sidestep such issues by taking allocators out of the picture.

Wasmtime GitHub notifications bot (Oct 02 2023 at 17:32):

alexcrichton created PR review comment:

If you'd like, to avoid the CallbackData structure here, which I'm assuming is only required to satisfy Send, the invoke_c_async_callback function could take &ForeignData which I think is already Send

Wasmtime GitHub notifications bot (Oct 02 2023 at 18:02):

rockwotj submitted PR review.

Wasmtime GitHub notifications bot (Oct 02 2023 at 18:02):

rockwotj created PR review comment:

I can't get that to work, which maybe means I'm doing something unsafe here...

The compiled tells me that the reference to the captured variables can't escape the closure. I'm not sure if Wasmtime ensures that the futures are completed before deleting the host function closures, from my debugging it seems the order is correct. Additionally c_void is not Send so I need the wrapper type. Other suggestions or am I doing something dangerous here?

Wasmtime GitHub notifications bot (Oct 02 2023 at 18:02):

rockwotj submitted PR review.

Wasmtime GitHub notifications bot (Oct 02 2023 at 18:02):

rockwotj created PR review comment:

Thanks! This was vestigial from the refactor :)

Wasmtime GitHub notifications bot (Oct 02 2023 at 18:03):

rockwotj submitted PR review.

Wasmtime GitHub notifications bot (Oct 02 2023 at 18:03):

rockwotj created PR review comment:

Done.

Wasmtime GitHub notifications bot (Oct 02 2023 at 18:20):

rockwotj submitted PR review.

Wasmtime GitHub notifications bot (Oct 02 2023 at 18:20):

rockwotj updated PR #7106.

Wasmtime GitHub notifications bot (Oct 02 2023 at 18:20):

rockwotj created PR review comment:

Ah this is why I originally had the async_continuation_new function... I can make that an out ptr instead, seems better and allows for removing a heap allocation.

Wasmtime GitHub notifications bot (Oct 02 2023 at 18:20):

rockwotj requested alexcrichton for a review on PR #7106.

Wasmtime GitHub notifications bot (Oct 02 2023 at 18:25):

rockwotj updated PR #7106.

Wasmtime GitHub notifications bot (Oct 02 2023 at 18:25):

rockwotj updated PR #7106.

Wasmtime GitHub notifications bot (Oct 02 2023 at 20:13):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Oct 02 2023 at 20:13):

alexcrichton created PR review comment:

Due to this containing a function pointer I don't think that uninit is actually safe to do here, could this instead

Wasmtime GitHub notifications bot (Oct 02 2023 at 20:13):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Oct 02 2023 at 20:13):

alexcrichton created PR review comment:

er, sorry, hit send to soon!

I take this back, I think this is fine as-is.

Wasmtime GitHub notifications bot (Oct 02 2023 at 20:14):

alexcrichton created PR review comment:

Ah no I see why rustc would say that, and makes sense. Sorry for the wild goose chase, this is good as-is!

Wasmtime GitHub notifications bot (Oct 02 2023 at 20:14):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Oct 02 2023 at 20:14):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Oct 02 2023 at 20:37):

rockwotj submitted PR review.

Wasmtime GitHub notifications bot (Oct 02 2023 at 20:37):

rockwotj edited PR review comment.

Wasmtime GitHub notifications bot (Oct 02 2023 at 20:37):

rockwotj created PR review comment:

Yeah I wanted to initialize a value, but you can't assign null to a function ptr in rust so this was the best I could come up with... :big_smile:

Wasmtime GitHub notifications bot (Oct 02 2023 at 20:44):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Oct 02 2023 at 20:44):

alexcrichton created PR review comment:

Yeah the risk of this is that it triggers UB if C accidentally doesn't fill in the structure, so the alternative would be to have a dummy function pointer which panics. I don't think it's worth it to go that far though, hence my reconsideration this is fine as is!

Wasmtime GitHub notifications bot (Oct 03 2023 at 00:10):

github-merge-queue[bot] updated PR #7106.

Wasmtime GitHub notifications bot (Oct 03 2023 at 00:18):

rockwotj updated PR #7106.

Wasmtime GitHub notifications bot (Oct 03 2023 at 00:19):

rockwotj submitted PR review.

Wasmtime GitHub notifications bot (Oct 03 2023 at 00:19):

rockwotj created PR review comment:

I added the dummy function pointer - it's better to give guardrails.

Wasmtime GitHub notifications bot (Oct 03 2023 at 00:20):

rockwotj updated PR #7106.

Wasmtime GitHub notifications bot (Oct 03 2023 at 02:00):

rockwotj updated PR #7106.

Wasmtime GitHub notifications bot (Oct 03 2023 at 02:26):

rockwotj updated PR #7106.

Wasmtime GitHub notifications bot (Oct 03 2023 at 09:55):

rockwotj requested alexcrichton for a review on PR #7106.

Wasmtime GitHub notifications bot (Oct 03 2023 at 15:00):

alexcrichton merged PR #7106.

Last updated: Nov 22 2024 at 16:03 UTC