Skip to content

Atomics.wait: wake for a termination requested from another thread #392

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.

Sign up for GitHub

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

Open
dylan-conway wants to merge 1 commit into
base: main
Choose a base branch
from
+15 −3
Open

Atomics.wait: wake for a termination requested from another thread #392

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion Source/JavaScriptCore/runtime/AtomicsObject.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,7 +470,12 @@ JSValue atomicsWaitImpl(JSGlobalObject* globalObject, JSArrayType* typedArray, u
case WaiterListManager::WaitSyncResult::TimedOut:
return vm.smallStrings.timedOutString();
case WaiterListManager::WaitSyncResult::Terminated:
vm.throwTerminationException();
// The request may still be an unhandled NeedTermination trap fired from another thread
// while we were blocked; handling it records the request and throws the TerminationException.
if (vm.hasTerminationRequest())
vm.throwTerminationException();
else
vm.traps().handleTraps(VMTraps::NeedTermination);
return { };
}
RELEASE_ASSERT_NOT_REACHED();
Expand Down
11 changes: 9 additions & 2 deletions Source/JavaScriptCore/runtime/WaiterListManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,14 @@
list->addLast(listLocker, syncWaiter);
dataLogLnIf(WaiterListsManagerInternal::verbose, "<WaiterListManager> <Thread:", Thread::currentSingleton(), "> added a new SyncWaiter=", syncWaiter.get(), " to a waiterList for ptr ", RawPointer(ptr));

while (syncWaiter->isOnList() && time.now() < time && !vm.hasTerminationRequest())
// A termination requested from another thread (VM::notifyNeedTermination) is, until this
// thread handles its traps, only a fired NeedTermination trap bit: VMTraps notifies our
// condition for it, but hasTerminationRequest() is set by trap handling on this thread,
// which cannot happen while we block here. Wake for either.
auto terminationRequested = [&] {
return vm.hasTerminationRequest() || vm.traps().needHandling(VMTraps::NeedTermination);
};
while (syncWaiter->isOnList() && time.now() < time && !terminationRequested())
syncWaiter->condition().waitUntil(list->lock, time.approximate<WallTime>());

// At this point, syncWaiter should be either notified (dequeued) or timeout (not dequeued).
Expand All @@ -102,7 +109,7 @@

didGetDequeued = list->findAndRemove(listLocker, syncWaiter);
ASSERT(didGetDequeued);
return vm.hasTerminationRequest() ? WaitSyncResult::Terminated : WaitSyncResult::TimedOut;
return terminationRequested() ? WaitSyncResult::Terminated : WaitSyncResult::TimedOut;

Check failure on line 112 in Source/JavaScriptCore/runtime/WaiterListManager.cpp

View check run for this annotation

Claude / Claude Code Review

Wasm memory.atomic.wait caller not updated for new Terminated semantics 

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).

@claude claude Bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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

  1. A worker executes Wasm memory.atomic.wait32 on shared memory with no timeout. Control reaches waitImpl() → WaiterListManager::waitSync() → waitSyncImpl(), which parks on syncWaiter->condition().waitUntil(...).
  2. Another thread calls worker.terminate() → VM::notifyNeedTermination(). This fires the NeedTermination trap bit and (via VMTraps::requestThreadStopIfNeeded / SignalSender) notifies the sync waiter's condition. Crucially it does not set m_hasTerminationRequest — that only happens in VMTraps::handleTraps() on the target thread.
  3. waitSyncImpl wakes and re-evaluates the loop guard. terminationRequested() returns true because vm.traps().needHandling(VMTraps::NeedTermination) is true, even though vm.hasTerminationRequest() is still false. The loop exits, the waiter is dequeued, and the function returns WaitSyncResult::Terminated.
  4. Back in Wasm waitImpl(), the Terminated case calls vm.throwTerminationException().
  5. VM::throwTerminationException() executes ASSERT(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.

}
}

Expand Down
Loading