Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions src/jsc/bindings/bindings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5191,6 +5191,10 @@
void JSC__VM__notifyNeedTermination(JSC::VM* arg0)
{
JSC::VM& vm = *arg0;
// Firing NeedTermination wakes this VM's blocked Atomics.wait (VMTraps
// notifies vm.syncWaiter()), but WaiterListManager::waitForSync only exits
// on hasTerminationRequest(), so set it first or the waiter re-parks forever.
Comment thread
robobun marked this conversation as resolved.
vm.setHasTerminationRequest();

Check warning on line 5197 in src/jsc/bindings/bindings.cpp

View check run for this annotation

Claude / Claude Code Review

SigintWatcher::signalAll shares the same bug: breakOnSigint cannot interrupt Atomics.wait

Same-class sibling not covered: `SigintWatcher::signalAll()` (src/jsc/bindings/vm/SigintWatcher.cpp:210) calls `globalObject->vm().notifyNeedTermination()` directly from its background thread — the identical off-thread pattern fixed here, but bypassing this wrapper, so it does not get `setHasTerminationRequest()`. Consequence: `vm.runInThisContext('Atomics.wait(...)', { breakOnSigint: true })` + Ctrl+C wakes the waiter, which sees `hasTerminationRequest()` still false and re-parks forever (Node
Comment thread
robobun marked this conversation as resolved.
Comment thread
robobun marked this conversation as resolved.
Comment thread
robobun marked this conversation as resolved.
Comment thread
robobun marked this conversation as resolved.
bool didEnter = vm.currentThreadIsHoldingAPILock();
if (didEnter)
vm.apiLock().unlock();
Expand Down
64 changes: 64 additions & 0 deletions test/js/node/worker_threads/worker_threads.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,70 @@ test("all worker_threads worker instance properties are present", async () => {
await worker.terminate();
});

// JSC's Atomics.wait loop only exits on vm.hasTerminationRequest(), which used
// to be set only at a JS safepoint on the waiting thread itself, so the
// NeedTermination wakeup re-parked forever and terminate() never completed.
test("terminate() interrupts a worker blocked in Atomics.wait", async () => {
const sab = new SharedArrayBuffer(4);
const ia = new Int32Array(sab);
const worker = new Worker(
`const { parentPort, workerData } = require("node:worker_threads");
const ia = new Int32Array(workerData);
parentPort.postMessage("ready");
for (;;) Atomics.wait(ia, 0, 0);`,
{ eval: true, workerData: sab },
);
await once(worker, "message");
// Atomics.notify returns how many agents it woke; spinning until it returns
// 1 proves the worker thread is parked inside Atomics.wait.
while (Atomics.notify(ia, 0, 1) === 0) {}
expect(await worker.terminate()).toBe(1);
});

// A worker's own exit (process.exit or an uncaught throw) joins its child
// workers during teardown, so a grandchild parked in Atomics.wait must be
// interruptible or the middle worker never finishes exiting and its parent
// never receives 'exit'.
test("process.exit() in a worker completes while its own child worker is parked in Atomics.wait", async () => {
using dir = tempDir("worker-nested-atomics-exit", {
"nested-fixture.mjs": `
import { Worker, parentPort, workerData } from "node:worker_threads";
const role = workerData?.role ?? "main";
if (role === "main") {
const child = new Worker(new URL(import.meta.url), { workerData: { role: "child" } });
child.on("message", m => console.log(m));
child.on("exit", code => console.log("child exit-event", code));
} else if (role === "child") {
const sab = new SharedArrayBuffer(4);
const ia = new Int32Array(sab);
const grand = new Worker(new URL(import.meta.url), { workerData: { role: "grand", sab } });
grand.on("message", () => {
// Spin until notify reports one woken agent: the grandchild is
// provably parked in the futex (it re-enters it right away).
while (Atomics.notify(ia, 0, 1) === 0) {}
parentPort.postMessage("child exiting");
process.exit(5);
});
} else {
parentPort.postMessage("parked");
const ia = new Int32Array(workerData.sab);
for (;;) Atomics.wait(ia, 0, 0);
}
`,
});
await using proc = Bun.spawn({
cmd: [bunExe(), "nested-fixture.mjs"],
env: bunEnv,
cwd: String(dir),
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stderr).toBe("");
expect(stdout).toBe("child exiting\nchild exit-event 5\n");
expect(exitCode).toBe(0);
});

test("threadId module and worker property is consistent", async () => {
const worker1 = new Worker(new URL("./worker-thread-id.ts", import.meta.url));
expect(threadId).toBe(0);
Expand Down