deprecate [Typed]Func::post_return[_async] and make them no-ops
#12498
Conversation
With the advent of the Component Model concurrency ABI and it's `task.return` intrinsic, post-return functions have been informally deprecated and are expected to be removed for WASI 1.0 and the corresponding stable edition of the Component Model. Consequently, it does not make sense anymore to require embedders to explicitly call the post-return function after using `[Typed]Func::call[_async]`. As of this commit, `[Typed]Func::post_return[_async]` are no-ops. Instead, the post-return function is called automatically as part of `[Typed]Func::call[_async]` if present, which is how `[Typed]Func::call_concurrent` has worked all along. In addition, this commit fixes and tests a couple of cases where the task and/or thread was being disposed of before the post-return function was called.
github-actions
bot
added
fuzzing
Subscribe to Label Actioncc @fitzgen DetailsThis issue or pull request has been labeled: "fuzzing", "wasmtime:api", "wasmtime:c-api", "wizer"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
alexcrichton
left a comment
alexcrichton
left a comment
There was a problem hiding this comment.
For the "Monolith checks" CI job if you rebase on main that'll resolve that error
| }) | ||
| } | ||
|
|
||
| #[deprecated(note = "no longer has any effect")] |
There was a problem hiding this comment.
Can this percolate up into the C API documentation to indicate that the function is a noop?
| let result = func.post_return(&mut context); | ||
|
|
||
| crate::handle_result(result, |_| {}) | ||
| crate::handle_result(Ok(()), |_| {}) |
There was a problem hiding this comment.
It's ok to just return None from this function, no need to use the handle_result helper
| let ret = func.call_async(&mut *self.store, ()).await.unwrap().0; | ||
| let ret = use_ret(&mut *self.store, ret); | ||
| func.post_return_async(&mut *self.store).await.unwrap(); | ||
| ret | ||
| use_ret(&mut *self.store, ret) |
There was a problem hiding this comment.
This reminds me that this was actually intentional that it's inserted between call_async and post_return_async. This specifically uses WasmList<u8> to avoid copying the entire memory. That being said though there's no post-return on this function so it's fine either way. Mostly just a quirk I figured I'd point out
|
|
||
| /// See [`Func::post_return_async`] | ||
| #[doc(hidden)] | ||
| #[deprecated(note = "no longer has any effect")] |
There was a problem hiding this comment.
Could this be clarified to indicate that calls can be removed? Something like "no longer needed to call this, this function has no effect" or something like that. Mostly something to at-a-glance ensure users see that the solution is to just delete the call to this function.
| store.0.validate_sync_call()?; | ||
| self.call_impl(store, params) | ||
| let result = self.call_impl(store.as_context_mut(), params)?; | ||
| self.func.post_return_impl(store)?; |
There was a problem hiding this comment.
Can this get folded into call_impl? It looks like, for example, that below call_async may not be calling post-return when concurrency is disabled. Mind expanding the test coverage to catch that as well? In theory it's mostly a matter of finding the case already testing this and then also do the same thing for Config::concurrency_support(false)
| // Unset the "needs post return" flag now that post-return is being | ||
| // processed. This will cause future invocations of this method to | ||
| // panic, even if the function call below traps. | ||
| flags.set_needs_post_return(false); |
There was a problem hiding this comment.
In theory we can probably jettison all this handling since the only purpose of this flag was for the API of a dedicated post_return function itself. That's fine to defer to later though
| call_post_return_automatically, | ||
| move |func, store, params_out| { | ||
| func.with_lower_context(store, call_post_return_automatically, |cx, ty| { | ||
| func.with_lower_context(store, true, |cx, ty| { |
There was a problem hiding this comment.
Should the boolean parameter to with_lower_context get removed as well? I think it's always true now, right?
| store.0.validate_sync_call()?; | ||
| self.call_impl(&mut store.as_context_mut(), params, results) | ||
| self.call_impl(store.as_context_mut(), params, results)?; | ||
| self.post_return_impl(store)?; |
There was a problem hiding this comment.
Similar to TypedFunc, I think this'd be good to merge into call_impl to ensure that no-concurrency call_async also calls post-return (I think it's missing that currently)
| self.id() | ||
| .get_mut(store) | ||
| .post_return_arg_set(function_index, post_return_arg); |
There was a problem hiding this comment.
I think this function on vm::ComponentInstance, as well as the internal slot for this, may all be deletable now too
2 participants
With the advent of the Component Model concurrency ABI and it's
task.returnintrinsic, post-return functions have been informally deprecated and are expected to be removed for WASI 1.0 and the corresponding stable edition of the Component Model. Consequently, it does not make sense anymore to require embedders to explicitly call the post-return function after using[Typed]Func::call[_async].As of this commit,
[Typed]Func::post_return[_async]are no-ops. Instead, the post-return function is called automatically as part of[Typed]Func::call[_async]if present, which is how[Typed]Func::call_concurrenthas worked all along. In addition, this commit fixes and tests a couple of cases where the task and/or thread was being disposed of before the post-return function was called.