-
Notifications
You must be signed in to change notification settings - Fork 1.6k
trap on blocking call in sync task before return #12043
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
c6c26c4 to
3547452
Compare
|
Talked about this with @dicej today, current blockers are:
|
|
Regarding the Eventually, we'll want a proper way for host functions to "cheat" and block the whole guest without monopolizing the |
|
New thought: I think we're going to be required to implement "component-model-sync function can do async things on the host" in Wasmtime. Ignoring p3 for a moment, that's exactly how all async works in WASIp2. All WASIp2 functions are "sync" functions which leverage host superpowers to suspend the guest, and we can't break WASIp2, so I don't think that "just solve tcp/udp for wasip3" is a viable solution after all. |
1dfed8a to
3d14cbf
Compare
Agreed, but I don't see how that's relevant to this PR. Sync-typed exports are still permitted to call sync-typed imports, and Certainly we'll need to confront p2 head-on once we remove or replace |
|
Er right, yes, wires crossed again. That means though that #12085 is possible which "fixes" tcp/udp. |
3d14cbf to
c3fb452
Compare
|
Some of the WAST tests are hanging and/or busy looping. I'll investigate this morning. |
This anticipates the forthcoming changes to WIT/CM async and helps us get CI green for bytecodealliance/wasmtime#12043 Signed-off-by: Joel Dice <[email protected]>
|
CI is green now; I'm temporarily pointing the |
|
I believe we're just waiting on a spec PR (hopefully accompanied by WAST tests covering the changes) at this point. |
This implements a spec change (PR pending) such that tasks created for calls to
synchronous exports may not call potentially-blocking imports or return `wait`
or `poll` callback codes prior to returning a value. Specifically, the
following are prohibited in that scenario:
- returning callback-code.{wait,poll}
- sync calling an async import
- sync calling subtask.cancel
- sync calling {stream,future}.{read,write}
- sync calling {stream,future}.cancel-{read,write}
- calling waitable-set.{wait,poll}
- calling thread.suspend
This breaks a number of tests, which will be addressed in follow-up commits:
- The `{tcp,udp}-socket.bind` implementation in `wasmtime-wasi` is implemented
using `Linker::func_wrap_concurrent` and thus assumed to be async, whereas the
WIT interface says they're sync, leading to a type mismatch error at runtime.
Alex and I have discussed this and have a general plan to address it.
- A number of tests in the tests/component-model submodule that points to the
spec repo are failing. Those will presumably be fixed as part of the upcoming
spec PR (although some could be due to bugs in this implementation, in which
case I'll fix them).
- A number of tests in tests/misc_testsuite are failing. I'll address those in
a follow-up commit.
Signed-off-by: Joel Dice <[email protected]>
`check_blocking` needs access to the current task, but that's not set for post-return functions since those should not be calling _any_ imports at all, so first check for that. Signed-off-by: Joel Dice <[email protected]>
This amounts to adding `async` to any exported component functions that might need to block. Signed-off-by: Joel Dice <[email protected]>
Signed-off-by: Joel Dice <[email protected]>
Per the proposed spec changes, `thread.yield` should return control to the guest immediately without allowing any other thread to run. Similarly, when an async-lifted export or callback returns `CALLBACK_CODE_YIELD`, we should call the callback again immediately without allowing another thread to run. Signed-off-by: Joel Dice <[email protected]>
Signed-off-by: Joel Dice <[email protected]>
Signed-off-by: Joel Dice <[email protected]>
Note that this temporarily updates the `tests/component-model` submodule to the branch for WebAssembly/component-model#577 until that PR is merged. Signed-off-by: Joel Dice <[email protected]>
This clarifies that such a task cannot block prior to returning. Signed-off-by: Joel Dice <[email protected]>
4408972 to
497ba53
Compare
Signed-off-by: Joel Dice <[email protected]>
sorry again how is that not function coloring? |
I somehow forgot to address this earlier. Thanks to Luke for catching this. Note that this commit doesn't include test coverage, but Luke's forthecoming tests in the `component-model` repo will cover it, and we'll pull that in with the next submodule update. Signed-off-by: Joel Dice <[email protected]>
|
@pannous if you have technical concerns about this change, mind bringing it up on WebAssembly/component-model#578? That'll be a better forum for discussing the design than here |
|
@dicej did you have anything else you wanted to sort out with this or is it ready to go? |
Currently the |
|
Could this ignore the upstream tests temporarily and/or vendor working copies of them? Behavior-wise though there's nothing more you're looking to add here? |
alexcrichton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two extra thoughts in addition to the ones below:
- Could an
assert!be done in a single "core" location that suspension happens that the task is allowed to block? That would be a final check that we did indeed sprinkle all the checks correctly. - We'll definitely want a test, in repo, that traps happen as opposed to just updating all existing tests to not trap. I believe Luke has one upstream, but for landing this ahead of the upstream PR could this vendor the test in this repo to ensure we're running it here?
| // | ||
| // TODO: Where does async checking happen? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This'll want to get filled out I believe
| ty: TypeFuncIndex, | ||
| options: OptionsIndex, | ||
| storage: &mut [MaybeUninit<ValRaw>], | ||
| async_function: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be plumbed through with S as a type parameter rather than an extra argument to follow the same idioms of the other constructors in this file?
| if async_function { | ||
| concurrent::check_blocking(store.0)?; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you leave a comment on checks like this to indicate what the trap/check represents?
| if !self.options(store, options).async_ { | ||
| store.concurrent_state_mut().check_blocking()?; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to host.rs, mind leaving a comment on these checks throughout this file explaining what's going on?
Yeah; I'll switch the submodule back and temporarily skip the offending tests.
Correct; nothing left behavior-wise. |
I'll look into that.
Yup, I'll just copy the test from his PR into e.g. |
alexcrichton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh meant to do this too
This implements a spec change (PR pending) such that tasks created for calls to synchronous exports may not call potentially-blocking imports or return
waitorpollcallback codes prior to returning a value. Specifically, the following are prohibited in that scenario:This breaks a number of tests, which will be addressed in follow-up commits:
TheEDIT: addressed by Relax required host capabilities in wasip3 tcp/udp bindings #12085{tcp,udp}-socket.bindimplementation inwasmtime-wasiis implemented usingLinker::func_wrap_concurrentand thus assumed to be async, whereas the WIT interface says they're sync, leading to a type mismatch error at runtime. Alex and I have discussed this and have a general plan to address it.A number of tests in the tests/component-model submodule that points to the spec repo are failing. Those will presumably be fixed as part of the upcoming spec PR (although some could be due to bugs in this implementation, in which case I'll fix them).EDIT: addressed by addasyncto funcs that need to block in WAST tests WebAssembly/component-model#577A number of tests in tests/misc_testsuite are failing. I'll address those in a follow-up commit.