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
rockwotj requested wasmtime-core-reviewers for a review on PR #7106.
rockwotj requested alexcrichton for a review on PR #7106.
rockwotj requested wasmtime-default-reviewers for a review on PR #7106.
rockwotj updated PR #7106.
rockwotj updated PR #7106.
rockwotj updated PR #7106.
rockwotj updated PR #7106.
rockwotj updated PR #7106.
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
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!
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!
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++
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 generatingwasmtime/__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.
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 theresults
andargs
raw pointers but that can be mitigated one layer up wherewasmtime_func_call_async
would perform theslice_from_raw_parts
calls, then passing safe slices to theasync
pieces here.I ask mainly because mixing
async
andunsafe
is a recipe for bad things to happen, particularly around the lifetimes of arguments and such.
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.
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 havingwasmtime_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 wasOutput = ()
and then APIs such aswasmtime_linker_instantiate_async
take awasmtime_error_t**
as a pointer to fill in with the error, if any.
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 aswasmtime_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 bycb
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.
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 thepoll
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 withwasmtime_async_continuation_t
. (e.g. you may be able to get away withimpl Future for wasmtime_async_continuation_t
or something like that.- Moving the bulk of this function to an
async fn
. Theasync 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
onwasmtime_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.
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.
rockwotj submitted PR review.
rockwotj created PR review comment:
Yeah happy to move all this to a dedicated header and source file.
rockwotj submitted PR review.
rockwotj created PR review comment:
To be clear, you're talking about moving the function and linker methods into this new header too?
rockwotj submitted PR review.
rockwotj created PR review comment:
Sounds good.
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
alexcrichton submitted PR review.
rockwotj submitted PR review.
rockwotj submitted PR review.
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);
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Yeah that looks good to me :+1:
alexcrichton edited PR review comment.
rockwotj submitted PR review.
rockwotj created PR review comment:
I actually wanted to do this at first so happy to update :)
rockwotj updated PR #7106.
rockwotj submitted PR review.
rockwotj created PR review comment:
Done!
rockwotj edited PR review comment.
rockwotj submitted PR review.
rockwotj created PR review comment:
Thanks yes this is better as I'm wrapping my head around async + unsafe in Rust :big_smile: done.
rockwotj submitted PR review.
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 thecaller
parameter, but maybe that should follow suit of the results and params args and just make the caller keep a reference to that too.
rockwotj submitted PR review.
rockwotj created PR review comment:
Okay I did that and see what you were saying. Nice suggestion!
rockwotj submitted PR review.
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).
rockwotj updated PR #7106.
rockwotj updated PR #7106.
rockwotj submitted PR review.
rockwotj created PR review comment:
Correct - I've updated these docs a bit.
rockwotj requested alexcrichton for a review on PR #7106.
rockwotj updated PR #7106.
rockwotj updated PR #7106.
rockwotj submitted PR review.
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.
rockwotj updated PR #7106.
rockwotj submitted PR review.
rockwotj created PR review comment:
I went ahead and did this - I quite like how it turned out.
rockwotj updated PR #7106.
rockwotj updated PR #7106.
rockwotj updated PR #7106.
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
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
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 inwasmtime_async_continuation_t
and then must fill intrap_ret
andresults
by the time the continuation yields "I'm done"?
alexcrichton created PR review comment:
As a small rust-ism the
mem::take
isn't required here and you can sayhostcall_val_storage.truncate(0)
and put it back into the store
alexcrichton created PR review comment:
Mind combining this clause with the above one so the three cases of suspension are enumerated as a set?
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 asnew
uses in C++. By using a retptr it would help sidestep such issues by taking allocators out of the picture.
alexcrichton created PR review comment:
If you'd like, to avoid the
CallbackData
structure here, which I'm assuming is only required to satisfySend
, theinvoke_c_async_callback
function could take&ForeignData
which I think is alreadySend
rockwotj submitted PR review.
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?
rockwotj submitted PR review.
rockwotj created PR review comment:
Thanks! This was vestigial from the refactor :)
rockwotj submitted PR review.
rockwotj created PR review comment:
Done.
rockwotj submitted PR review.
rockwotj updated PR #7106.
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.
rockwotj requested alexcrichton for a review on PR #7106.
rockwotj updated PR #7106.
rockwotj updated PR #7106.
alexcrichton submitted PR review.
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
alexcrichton submitted PR review.
alexcrichton created PR review comment:
er, sorry, hit send to soon!
I take this back, I think this is fine as-is.
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!
alexcrichton submitted PR review.
alexcrichton submitted PR review.
rockwotj submitted PR review.
rockwotj edited PR review comment.
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:
alexcrichton submitted PR review.
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!
github-merge-queue[bot] updated PR #7106.
rockwotj updated PR #7106.
rockwotj submitted PR review.
rockwotj created PR review comment:
I added the dummy function pointer - it's better to give guardrails.
rockwotj updated PR #7106.
rockwotj updated PR #7106.
rockwotj updated PR #7106.
rockwotj requested alexcrichton for a review on PR #7106.
alexcrichton merged PR #7106.
Last updated: Dec 23 2024 at 12:05 UTC