async: remove inter-task wakeup stream from waitable set before cancelling#1638
Open
GodlyDonuts wants to merge 1 commit into
Open
Conversation
…ancelling
`cancel_inter_task_stream_read` issued a synchronous `stream.cancel-read` on
the inter-task wakeup stream while that stream was still a member of the task's
waitable set, only calling `remove_waitable` afterwards.
Per the Component Model (component-model#647), a synchronous
`{stream,future}.cancel-{read,write}` traps if the waitable is still in a
waitable set, for the same reason synchronous reads/writes do. Runtimes that
enforce this trap (e.g. recent Wasmtime) therefore fault here during ordinary
async operation.
Reorder so the stream leaves the waitable set before the synchronous cancel,
matching the unregister-then-cancel ordering already used by the general
`WaitableOperation::cancel` path. The cancel result is discarded as before, so
behavior is otherwise unchanged.
Author
|
Companion to bytecodealliance/wasmtime#13708, which adds the matching "in a waitable set" trap for synchronous |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
cancel_inter_task_stream_readissues a synchronousstream.cancel-readon theinter-task wakeup stream while that stream is still a member of the task's
waitable set, calling
remove_waitableonly afterwards:Per component-model#647, a synchronous
{stream,future}.cancel-{read,write}must trap if the waitable is still in a waitable set, for the same reason the
synchronous read/write operations do (the cancel may need to block on the very
waitable the set is watching). The canonical ABI codifies this as
trap_if(e.in_waitable_set() and not async_).The general cancellation path in
WaitableOperation::cancelalready respectsthis — it calls
unregister_waker(removing the waitable from the set) beforeinvoking the synchronous cancel intrinsic.
cancel_inter_task_stream_readisthe one place that doesn't follow that ordering, so on a runtime that enforces
the trap it faults during ordinary async operation.
This reorders the two operations so the stream leaves the waitable set before
the synchronous cancel, matching the established unregister-then-cancel pattern.
The cancel's return code is discarded exactly as before, so behavior is
otherwise unchanged.
Context / testing
This surfaced while implementing the corresponding cancel traps in Wasmtime
(bytecodealliance/wasmtime#13690): once Wasmtime traps on a synchronous cancel
of an in-set waitable, every
wasi:http@0.3.0(p3) program built with thisruntime traps in
cancel_inter_task_stream_read. With this change applied(via a local
[patch.crates-io]), Wasmtime's full p3 HTTP/CLI suites passagain (52 previously-failing tests across
wasmtime-wasi/wasmtime-wasi-http).