Trap on sync stream/future cancel and subtask.cancel when in a waitable set#13708
Trap on sync stream/future cancel and subtask.cancel when in a waitable set#13708GodlyDonuts wants to merge 1 commit into
Conversation
…le set
Synchronous `{stream,future}.{read,write}` already trap when their waitable
has been added to a waitable set, but the corresponding `cancel-{read,write}`
and `subtask.cancel` intrinsics did not, even though component-model#647 added
the same trap for them.
The existing trap is enforced in `wait_for_event`, which every blocking sync
read/write funnels through. The sync cancel paths don't always reach it: when
there is nothing pending to wait on, `cancel_read`/`cancel_write` return a
`Cancelled` code directly and `subtask.cancel` resolves without suspending, so
the "in a waitable set" check was simply never performed.
Hoist that check into a small `Waitable::trap_if_in_waitable_set` helper and
call it from the three sync cancel entry points (guarded by `!async_`, like the
neighbouring `check_blocking` calls). `wait_for_event` now uses the same helper
so the single source of truth is shared.
Adds a regression test covering all five operations, mirroring the upstream
`trap-if-sync-and-waitable-set.wast` spec test.
|
I'm going to redirect review to someone who knows much more about the CM async machinery (hope that's ok @dicej!). |
dicej
left a comment
There was a problem hiding this comment.
LGTM, thanks!
CI is failing, and it seems to point to wit_bindgen::rt's internal machinery for handling inter-task communication. I suspect a change will be needed in that project in order to get those tests passing. I'm on vacation at the moment (and should not have opened my computer at all 😅 ), but I can take a closer look when I get back if nobody beats me to it.
|
Thanks @dicej! That matches what I found, the trap fires in I opened bytecodealliance/wit-bindgen#1638 to fix that: it reorders the two so the stream leaves the waitable set before the synchronous cancel, matching the unregister-then-cancel ordering the general So this just needs a |
Fixes #13690.
component-model#647 tightened the trap rules for synchronous stream/future
operations: a synchronous
read/writemust trap if the waitable it operateson has already been added to a waitable set. That PR extended the same rule to
{stream,future}.cancel-{read,write}andsubtask.cancel, but Wasmtime onlyimplemented it for
read/write. As a result the upstreamtrap-if-sync-and-waitable-set.wasttest hits anunreachableon the cancelcases today instead of trapping.
Why it was missing
The check lives in
wait_for_event, the single point every blockingsynchronous read/write funnels through before suspending:
The cancel paths don't reliably reach it. When there is nothing pending to wait
on,
cancel_read/cancel_writetake an early return with aCancelledcode,and
subtask.cancelcan resolve a starting/finished subtask without eversuspending. In those cases the waitable-set check was never run, so the cancel
succeeded where the spec requires a trap.
Fix
Pull the check into a small helper on
Waitable:wait_for_eventnow calls it (no behavior change), and the three synchronouscancel entry points —
cancel_read,cancel_write, andsubtask_cancel—call it up front, guarded by
!async_to match the neighbouringcheck_blockingguards. The async variants are deliberately untouched: being amember of a waitable set is the whole point of an asynchronous cancel.
Testing
Added
tests/misc_testsuite/component-model/async/cancel-sync-and-waitable.wastcovering all five operations (
future.cancel-{read,write},stream.cancel-{read,write}, andsubtask.cancel). Each starts a pendingoperation, joins the waitable to a fresh set, and asserts the synchronous
cancel traps; this mirrors the upstream
trap-if-sync-and-waitable-set.wast. Iverified each case regresses (returns a cancel code / completes normally rather
than trapping) when its individual guard is removed.
Depends on bytecodealliance/wit-bindgen#1638
Enforcing this trap surfaces a latent bug in wit-bindgen's async runtime:
cancel_inter_task_stream_readissues a synchronousstream.cancel-readon itsinter-task wakeup stream while that stream is still in the task's waitable set
(it calls
remove_waitableonly afterwards). That is exactly the pattern thistrap now (correctly) rejects, so every
wasi:http@0.3.0(p3) program built withthe current
wit-bindgentraps during ordinary operation — which is what thered p3 CI jobs here are.
bytecodealliance/wit-bindgen#1638 reorders that to leave the set before
cancelling, matching the unregister-then-cancel ordering the general
WaitableOperation::cancelpath already uses. With that change applied locally(via
[patch.crates-io]), the full p3 suites pass again:wasmtime-wasi-httpp3: 25 passedwasmtime-wasip3: 27 passedcomponent-model/asyncwast (incl. the new test): 192 passedSo this PR needs a
wit-bindgenrelease containing #1638 and a correspondingversion bump before its p3 jobs go green. Happy to fold the bump in here once
that's available, or sequence it however you prefer.