Atomics.wait: wake for a termination requested from another thread - #392
Atomics.wait: wake for a termination requested from another thread#392dylan-conway wants to merge 1 commit into
Conversation
VM::notifyNeedTermination() from another thread fires the NeedTermination trap and VMTraps notifies the blocked VM's syncWaiter condition, but waitSync()'s loop only re-checked vm.hasTerminationRequest(), which is set by trap *handling* on the blocked thread itself — impossible while it sits in waitUntil(). The waiter woke, saw no request, and slept again, so a worker parked in Atomics.wait could never be terminated. waitSync() now also treats a pending NeedTermination trap bit as a termination, and the Atomics.wait host function handles that trap (which records the request and throws the TerminationException) when the request has not been recorded yet.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughSynchronous atomic waits now detect both recorded termination requests and pending ChangesTermination handling
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| didGetDequeued = list->findAndRemove(listLocker, syncWaiter); | ||
| ASSERT(didGetDequeued); | ||
| return vm.hasTerminationRequest() ? WaitSyncResult::Terminated : WaitSyncResult::TimedOut; | ||
| return terminationRequested() ? WaitSyncResult::Terminated : WaitSyncResult::TimedOut; |
There was a problem hiding this comment.
🔴 This widens WaitSyncResult::Terminated to cover the "only a NeedTermination trap bit is set" state, but the second caller of waitSync() — Wasm's waitImpl() in WasmOperationsInlines.h:750 — was not updated and still calls vm.throwTerminationException() directly, which ASSERT(hasTerminationRequest())s. Terminating a worker blocked in memory.atomic.wait32/64 will now hit that debug assertion (and in release, throw with m_hasTerminationRequest unset and the trap bit still pending). The Wasm caller needs the same hasTerminationRequest() / handleTraps(NeedTermination) branch you added in AtomicsObject.cpp, or waitSyncImpl could handle the trap itself before returning Terminated.
Extended reasoning...
What changed and why it breaks the Wasm path
Before this PR, waitSyncImpl() only returned WaitSyncResult::Terminated when vm.hasTerminationRequest() was already true. Both callers therefore safely responded with a bare vm.throwTerminationException().
This PR broadens the condition: terminationRequested() now also fires when vm.traps().needHandling(VMTraps::NeedTermination) is true — i.e. when another thread has set the trap bit via VM::notifyNeedTermination() but this thread has not yet run VMTraps::handleTraps() to record m_hasTerminationRequest. The PR description explicitly calls out that throwTerminationException() "asserts hasTerminationRequest()", and correctly updates the JS Atomics.wait caller in AtomicsObject.cpp:472-478 to branch: if hasTerminationRequest() is already set, throw directly; otherwise call vm.traps().handleTraps(VMTraps::NeedTermination), which sets the request and throws.
However, WaiterListManager::waitSync() has exactly one other caller: Wasm's waitImpl() in Source/JavaScriptCore/wasm/WasmOperationsInlines.h:738-755, reached from memory.atomic.wait32/wait64 in every Wasm tier (memoryAtomicWait32/64, operationMemoryAtomicWait32/64, ipint_extern_memory_atomic_wait32/64). That caller was not touched:
case WaiterListManager::WaitSyncResult::Terminated:
vm.throwTerminationException();
return -1;And VM::throwTerminationException() (VM.cpp:1102) begins with ASSERT(hasTerminationRequest()); VM::setException() (VM.cpp:1093) asserts the same invariant.
Step-by-step proof
- A worker executes Wasm
memory.atomic.wait32on shared memory with no timeout. Control reacheswaitImpl()→WaiterListManager::waitSync()→waitSyncImpl(), which parks onsyncWaiter->condition().waitUntil(...). - Another thread calls
worker.terminate()→VM::notifyNeedTermination(). This fires theNeedTerminationtrap bit and (viaVMTraps::requestThreadStopIfNeeded/ SignalSender) notifies the sync waiter's condition. Crucially it does not setm_hasTerminationRequest— that only happens inVMTraps::handleTraps()on the target thread. waitSyncImplwakes and re-evaluates the loop guard.terminationRequested()returns true becausevm.traps().needHandling(VMTraps::NeedTermination)is true, even thoughvm.hasTerminationRequest()is still false. The loop exits, the waiter is dequeued, and the function returnsWaitSyncResult::Terminated.- Back in Wasm
waitImpl(), theTerminatedcase callsvm.throwTerminationException(). VM::throwTerminationException()executesASSERT(hasTerminationRequest())→ debug-build assertion failure.
In release builds the asserts compile out and the TerminationException is thrown, but m_hasTerminationRequest is never set and the NeedTermination trap bit is never cleared — violating the VM invariant that hasTerminationRequest() is set whenever a TerminationException is pending, and leaving a stale trap bit to be re-handled at the next trap check.
Why nothing else prevents it
The only guard is at the call sites. The PR added the guard to one of the two call sites and left the other unchanged. There is no fallback in throwTerminationException() that promotes the trap bit to a request; it simply asserts.
Fix
Apply the same treatment to WasmOperationsInlines.h:749-751:
case WaiterListManager::WaitSyncResult::Terminated:
if (vm.hasTerminationRequest())
vm.throwTerminationException();
else
vm.traps().handleTraps(VMTraps::NeedTermination);
return -1;Alternatively, centralize this: have waitSyncImpl call vm.traps().handleTraps(VMTraps::NeedTermination) (or otherwise record the request) before returning Terminated, so both callers can keep the bare throwTerminationException() and future callers can't make the same mistake.
Preview Builds
|
1 participant
VM::notifyNeedTermination()called from another thread (a dedicated worker'sterminate()) fires theNeedTerminationtrap, andVMTraps::requestThreadStopIfNeeded()/ the SignalSender notify the target VM'ssyncWaiter()condition so a thread blocked inAtomics.waitwakes up. ButWaiterListManager::waitSync()only re-checksvm.hasTerminationRequest()— andm_hasTerminationRequestis set byVMTraps::handleTraps()on the blocked thread itself, which cannot run while that thread is insidewaitUntil(). So the waiter wakes, sees no request, and goes back to sleep: a worker parked inAtomics.wait(…)with no timeout can never be terminated (its thread never exits; whoever joins it hangs). Upstreammainhas the same code: https://github.com/WebKit/WebKit/blob/9ef04dabf52d7432a3e7ab13f44f9d8b4bd933a0/Source/JavaScriptCore/runtime/WaiterListManager.cpp#L95-L105 (loop) and https://github.com/WebKit/WebKit/blob/9ef04dabf52d7432a3e7ab13f44f9d8b4bd933a0/Source/JavaScriptCore/runtime/VMTraps.cpp#L419-L420 (the notify).Change:
waitSync()treats a pendingNeedTerminationtrap bit (atomic, safe to read here) as a termination as well ashasTerminationRequest().atomicsWaitImpl'sTerminatedcase handles that trap when the request has not been recorded yet —handleTraps(NeedTermination)sets the request and throws the TerminationException — instead of callingthrowTerminationException()directly, which assertshasTerminationRequest().waitAsyncis unaffected. Observed in Bun as oven-sh/bun#32802 (worker.terminate()never completes for a worker inAtomics.wait); Node/V8 interrupt the wait in the same situation.