Repository navigation
add (off-by-default) task-group-hook feature - #14416
Conversation
github-actions
Bot
added
the
wasmtime:api
alexcrichton
left a comment
alexcrichton
left a comment
There was a problem hiding this comment.
Could this perhaps be something where a task_group_hook.rs submodule could be made to help split out code and reduce the #[cfg] traffic in concurrent.rs? (also in the spirit of keeping the file a bit smaller...)
Also, would it make sense to revert other work from #13510 such as the FuncCallConcurrent abstraction? (as that's sort of no longer relevant)
This addresses bytecodealliance#14247 and roughly resembles the API sketch Alex posted as a comment on that issue. Although the Component Model has no notion of a "task group", we define one here in order to enable embedders to associate guest->host calls with corresponding host->guest calls in a predictable way. See the doc comments on `TaskGroupHook` for details on what it means for a task to be part of a task group and what notifications a hook will receive for a given group. Note that, unlike with `CallHookHandler`, the functions in `TaskGroupHook` do not have a `StoreContextMut<T>` parameter. Supporting such a parameter is technically possible, but would require converting a _lot_ of functions so that they accept `&mut dyn VMStore` parameters instead of `&mut StoreOpaque` parameters, including parts of the public API. This is something we can revisit later, if desired. Meanwhile, types implementing `TaskGroupHook` may need to use e.g. `Arc` to share state with the `T` in `Store<T>`. This feature supersedes `StoreContextMut::async_call_stack`, which I've removed given that it is prone to use-after-free of a table slot as described in the aforementioned issue. Fixes bytecodealliance#14247
96c195b to
204053b
Compare
19a59a6 to
9e7aba7
Compare
- Ensure `TaskGroupHook` events are delivered even if the guest traps and/or leaks subtasks - Expand doc comments per review feedback - Move most `TaskGroupHook`-related code into its own module - Add `task_group_hook` function to `StoreContextMut` - Deduplicate code and make it harder to forget to increment or decrement `TaskGroup` ref counts
9e7aba7 to
67c201f
Compare
|
I believe I've addressed all the feedback so far. |
alexcrichton
left a comment
alexcrichton
left a comment
There was a problem hiding this comment.
A few minor things, mostly I think a *_disabled.rs file would work well here, but otherwise lgtm
| state.debug_assert_deferred_host_invariant(); | ||
| let old_thread = mem::replace(&mut state.unforced_current_thread, thread); | ||
|
|
||
| #[cfg(feature = "task-group-hook")] |
There was a problem hiding this comment.
FWIW this file still has quite a lot of #[cfg] which IMO defeats most of the purpose of outlining it into task_group_hook.rs as the goal is to generally have everything in one location with little #[cfg] necessary. That being said I might recommend some changes here to assist with that and reduce the #[cfg] this file. Notably a task_group_hook_disabled.rs would be a location to have stubs that do nothing, and that would enable this file to ideally have far less #[cfg] to assume that task group management should always be done. If the feature is disabled the functions would be noops, but when enabled it'd route into task_group_hooks.rs.
There was a problem hiding this comment.
To clarify as well, I don't think that this file should have zero #[cfg] in it, but I think it could still be reduced quite a bit by having enabled/disabled files, and that I think is beneficial to have a file that contains 99%+ of the implementation as opposed to as-is where you've got to cross-reference a few file simultaneously to find the body of the implementation.
There was a problem hiding this comment.
I just pushed an update; please let me know what you think.
There was a problem hiding this comment.
Err, hold on, let me address the CI issues first.
There was a problem hiding this comment.
Ok, really this time.
629646c to
9bfa670
Compare
9bfa670 to
9671657
Compare
alexcrichton
left a comment
alexcrichton
left a comment
There was a problem hiding this comment.
Just some minor nits, but lgtm 👍
| #[cfg(feature = "task-group-hook")] | ||
| pub use task_group_hook::TaskGroupHook; | ||
| pub use task_group_hook::TaskGroupId; |
There was a problem hiding this comment.
This might be able to pub use task_group_hook::* to avoid individually listing each name
There was a problem hiding this comment.
True, but I tend to avoid wildcard imports; they make it harder for the reader to determine where a given symbol is defined.
| ready_for_concurrent_call_waker: _, | ||
| event_loop_running: _, | ||
| #[cfg(feature = "task-group-hook")] | ||
| task_group_hook: _, |
There was a problem hiding this comment.
I know, but rustfmt insists 🤷
This addresses #14247 and roughly resembles the API sketch Alex posted as a comment on that issue.
Although the Component Model has no notion of a "task group", we define one here in order to enable embedders to associate guest->host calls with corresponding host->guest calls in a predictable way. See the doc comments on
TaskGroupHookfor details on what it means for a task to be part of a task group and what notifications a hook will receive for a given group.Note that, unlike with
CallHookHandler, the functions inTaskGroupHookdo not have aStoreContextMut<T>parameter. Supporting such a parameter is technically possible, but would require converting a lot of functions so that they accept&mut dyn VMStoreparameters instead of&mut StoreOpaqueparameters, including parts of the public API. This is something we can revisit later, if desired. Meanwhile, types implementingTaskGroupHookmay need to use e.g.Arcto share state with theTinStore<T>.This feature supersedes
StoreContextMut::async_call_stack, which I've removed given that it is prone to use-after-free of a table slot as described in the aforementioned issue.Fixes #14247