Repository navigation
Don't let a wake slip in between a wait's checks - #2
Conversation
wguiInstanceWaitEvents takes the sticky wake flag, then parks. A wake that arrived after that check but before the thread was in the set's list found nobody to unpark, and park looked only for events, so the guest slept through the wake for the rest of the slice: up to 50 ms. park now looks at the wake flag too, after the thread is in the list, without consuming it: either it sees the wake, or the waker finds the thread and unparks it. A SeqCst fence in park, between joining the list and looking, and one in notify, before it looks at the list, make that "either" hold under the memory model, for wakes and for every other reason to stop waiting that notify is called for; Acquire and Release alone don't order a write before a read of another location. Tests: a wake between the outer check and the thread joining the list no longer parks (fails without the fix), and a wake between that and the park itself is still caught by the thread's unpark token. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
syrusakbary
left a comment
syrusakbary
left a comment
There was a problem hiding this comment.
Reviewed the full wait protocol, its instance_wait_events caller, waiter reuse across sets, notification sites, and the interaction with #3. I found no blocking correctness issue in this diff.
The additional check preserves the sticky wake for the outer loop to consume. Once registration has happened, Rust's unpark token semantics cover notification just before parking. Keeping the explicit ordering protocol is preferable to relying on Crossbeam's current internal atomics. This also fits Emscripten's pending-notification approach to precise wakeups, adapted to Rust thread parking.
Validation: all 83 native default-feature unit tests passed on this head; all 86 passed with #2 applied together with #3. One non-blocking test-hardening suggestion is inline. I did not run a weak-memory model checker or browser/Wasm runtime test for this change.
| // Mirrors instance_wait_events' outer-loop check. | ||
| assert!(!set.take_wake()); | ||
| if before_registration { | ||
| assert!(set.waiters.is_empty()); | ||
| set.wake(); | ||
| } | ||
| set.park(Duration::from_secs(30), || { | ||
| if !before_registration { | ||
| // The waiter exists, but park_timeout has not run. | ||
| set.wake(); |
There was a problem hiding this comment.
[P3] Add coverage for the cross-thread ordering claim
Both new boundary cases call set.wake() on the waiting thread itself. They usefully catch the missing sticky-flag check and exercise the unpark token, but cannot expose a publication/observation reordering between threads, which is the other reason for this change. As a follow-up, please add a small Loom-style model of the register → fence → condition-check / publish-condition → fence → inspect-registration protocol, plus a coordinated two-thread boundary test. That would make the intended ordering guarantee reviewable without depending on an ARM stress run or Crossbeam's present implementation. This is test hardening, not a demonstrated defect in the current fences.
The tests of the last change call wake on the waiting thread itself. They catch a missing look at the sticky flag, but can't show how one thread's accesses look to another, which is what the fences are for. - A loom model of the handshake in src/host/wait.rs, built only with `--cfg loom` (`make test-loom`): the waiter takes a pending wake, registers, fences and looks; the notifier publishes, fences and looks for waiters. It fails if either fence or the look after registering is taken out, or if the fences are AcqRel. loom 0.7 implements SeqCst fences, though not SeqCst loads and stores. The model takes the wake with a compare-exchange rather than take_wake's swap: with the swap, loom reports a lost wake that C11 rules out. - The waiting thread held while another thread wakes the set: before it is in the list, where only the look after registering ends the wait; in the list before it looks, where the look or the unpark does; and after its look, where only the unpark does. A test-only hook in park holds it in the list. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
#3 now has main, with #1's wait count and #2's handshake between waiting and waking: - `notify` bumps the counter a suspended wait sleeps on, tells it, then takes #2's fence before it looks at the list of parked threads. - `park` checks a pending wake after the fence, as #2 has it; `wait_step` still takes it. - The parking wait counts the guest's waits once per call, as #1 has it, and the suspending wait now does too, on its first look: otherwise a window redrawn while the guest waits by suspending would be "asked for while drawing" from then on, and `eagerRedraw` would never answer at once. - Cargo.toml and the Makefile keep both sides' test setups (loom and the web wait test). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A wake could be lost between a wait's two checks, so the guest overslept by up to a wait slice (50 ms).
The race
wguiInstanceWaitEventsloops: it takes the sticky wake flag (take_wake), checks for events and timeouts, then parks for a slice. AwguiInstanceWake(or any other wake) that arrived aftertake_wakebut before the thread was in the wait set's list found nobody to unpark.parkthen looked only for events, not the flag, and slept for the rest of the slice.Change
WaitSet::park(src/host/wait.rs) now also looks at the wake flag after the thread is in the list, without consuming it: either it sees the wake, or the waker finds the thread and unparks it.A
SeqCstfence inpark(between joining the list and looking) and one innotify(before it looks at the list) make that "either" hold under the memory model. Acquire and release alone don't order a write before a read of another location; until now this leaned on crossbeam's internals. The fences cover every reasonnotifyis called for, not just wakes.Tests
On the waiting thread (the wake comes from the thread that is about to park):
a_wake_between_outer_check_and_registration_does_not_parka_wake_just_before_the_park_ends_it_at_once(guards the existing unpark-token path)Across threads. Three tests hold the waiting thread at one point while another thread calls
wake(), then let it go on. It parks with a 30 s timeout; the test waits at most 5 s for it, so a wake that's lost fails the test rather than slowing it. A one-line#[cfg(test)]hook inparkholds the thread once it is in the list.ready()only (the bug this fixes)unparkis removed fromnotifyThese order the two threads, so they can't show reordering; removing both fences passes them all.
Reordering, under loom. A model of the handshake (
#[cfg(all(test, loom))] mod modelinsrc/host/wait.rs, run bymake test-loom) explores every interleaving, and every value each load may read. The waiter registers, fences and looks; the notifier publishes, fences and looks for waiters. It checks that either the waiter's look sees what was published or the notifier finds the waiter. Registration is aRelaxedflag, so the fences alone have to be enough, whatever crossbeam's queue does.AcqRelSeqCstfences, but treatsSeqCstloads and stores asAcqRel; the handshake relies on the fences only.take_wake'sswap, loom reports a lost wake that C11 rules out, because it doesn't order the swap before the notifier's store. So the model takes the wake with acompare_exchange(true, false), which returns the same and leaves the flag the same. A comment in the model says so.queuedmark and parking itself are left out.loomis a dev-dependency only undercfg(loom), declared to theunexpected_cfgslint; ordinary builds don't compile it. CI doesn't runmake test-loomyet: my token can't change workflows.Locally (macOS arm64, Rust 1.95; also clippy on nightly 1.100):
make checkmake testmake test-loom🤖 Generated with Claude Code