Skip to content
Merged
5 changes: 5 additions & 0 deletions src/runtime/server/WebSocketServerContext.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,11 @@
global_object: &JSGlobalObject,
error_value: JSValue,
) {
// Termination raised inside the preceding callback.call() cannot be
// cleared; entering JS again trips executeCallImpl's assertNoException.
if global_object.has_exception() {
return;
}

Check failure on line 92 in src/runtime/server/WebSocketServerContext.rs

View check run for this annotation

Claude / Claude Code Review

Same-class sibling: termination in websocket open() handler still aborts via synchronous on_close re-entry before this guard

Same-class sibling not covered: when termination lands inside the websocket `open()` handler, `on_open`'s error branch calls `self.websocket().close()` (ServerWebSocket.rs:445) *before* reaching this guard, and that `close()` synchronously re-enters `on_close`, which then calls `on_close_handler.call(...)` at :744 with the termination still pending — the same `assertNoException` abort, just via native-close re-entry instead of the error-handler dispatch. A matching `has_exception()` guard is nee
Comment thread
claude[bot] marked this conversation as resolved.
if !on_error.is_empty_or_undefined_or_null() {
let _ = on_error
.call(global_object, JSValue::UNDEFINED, &[error_value])
Expand Down
5 changes: 5 additions & 0 deletions src/runtime/socket/Handlers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,11 @@ impl Handlers {
}

let global_object = self.global_object;
// Termination raised inside the preceding callback.call() cannot be
// cleared; entering JS again trips executeCallImpl's assertNoException.
if global_object.has_exception() {
return false;
}
let on_error = self.on_error();

if on_error.is_empty() {
Expand Down
155 changes: 155 additions & 0 deletions test/js/bun/net/socket-handler-worker-terminate.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,155 @@
// worker.terminate() firing while a Bun.listen socket handler is mid-call must
// not re-enter JS with the termination exception still pending. The socket
// dispatch path calls the error handler when the primary handler throws, and
// termination cannot be cleared: entering JS again trips Interpreter::
// executeCallImpl's `assertNoException`. Repro for the
// test/js/node/test/parallel/test-http2-reset-flood.js SIGABRT.
import { describe, expect, test } from "bun:test";
import { bunEnv, bunExe, isWindows } from "harness";

// The handler body shared by both the Bun.listen close handler and the
// Bun.serve websocket message handler: a two-phase Atomics handshake so the
// safepoint spin starts only once the parent is already calling terminate(),
// making the window build-speed-independent.
// shared[0]: worker -> parent ("I am inside the handler"; 2 = spin ran out)
// shared[1]: parent -> worker ("terminate() is in flight")
const handlerBody = `
Atomics.store(shared, 0, 1);
Atomics.notify(shared, 0);
Atomics.wait(shared, 1, 0, 5000);
// terminate() is now in flight; spin on safepoints so the termination
// exception is raised inside this handler frame. Bounded so a missed
// terminate cannot hang.
let sink = 0;
for (let i = 0; i < 10_000_000; i++) sink += Atomics.load(shared, 1);
Atomics.store(shared, 0, 2);
`;

const workerSource = `
const { parentPort } = require("worker_threads");
const shared = new Int32Array(require("worker_threads").workerData);

const server = Bun.listen({
hostname: "127.0.0.1",
port: 0,
socket: {
open(socket) { socket.write("hello"); },
data() {},
// socket_body.rs:on_close: a termination raised here returns Err to the
// native caller, which then invokes Handlers::call_error_handler.
close() {${handlerBody}},
error() {},
},
});
parentPort.postMessage(server.port);
`;

const wsWorkerSource = `
const { parentPort } = require("worker_threads");
const shared = new Int32Array(require("worker_threads").workerData);

const server = Bun.serve({
port: 0,
fetch(req, server) {
if (server.upgrade(req)) return;
return new Response("no upgrade", { status: 400 });
},
websocket: {
open(ws) { ws.send("hello"); },
// ServerWebSocket.rs:on_message: a termination raised here returns Err to
// the native caller, which then invokes WebSocketServerContext::
// run_error_callback.
message() {${handlerBody}},
close() {},
error() {},
},
});
parentPort.postMessage(server.port);
`;

function parentSource(workerSrc: string, driveHandler: string, cleanup: string) {
return `
const { Worker } = require("worker_threads");
const net = require("net");

(async () => {
// Each iteration is an independent opportunity for terminate() to land inside
// the handler. The two-phase handshake makes it land on the first try; a few
// repeats leave headroom for CI scheduling jitter.
let hits = 0;
for (let i = 0; i < 6; i++) {
const sab = new SharedArrayBuffer(8);
const shared = new Int32Array(sab);
const worker = new Worker(${JSON.stringify(workerSrc)}, { eval: true, workerData: sab });
const port = await new Promise(resolve => worker.once("message", resolve));
${driveHandler}
// Wait until the worker is inside the handler, then release it from its
// own wait and terminate so the termination exception lands mid-handler.
if (Atomics.wait(shared, 0, 0, 5000) === "timed-out") throw new Error("handler never fired");
Atomics.store(shared, 1, 1);
Atomics.notify(shared, 1);
await worker.terminate();
${cleanup}
// shared[0] === 1 means termination landed mid-handler (the spin loop was
// interrupted); 2 means the handler returned normally first.
if (Atomics.load(shared, 0) === 1) hits++;
}
if (hits === 0) throw new Error("terminate() never landed inside the handler (0/6)");
console.log("ok", hits);
})();
`;
}

const listenDriver = `
const conn = net.connect({ port, host: "127.0.0.1" });
await new Promise(resolve => conn.once("data", resolve));
// Closing the client drives on_close on the server's per-connection socket.

Check warning on line 106 in test/js/bun/net/socket-handler-worker-terminate.test.ts

View check run for this annotation

Claude / Claude Code Review

nit: worker-message and net.connect awaits do not wire error → reject (REVIEW.md convention; wsDriver already does)

nit: per REVIEW.md ("Wire EVERY failure event … to reject the awaited promise"), the `worker.once("message", resolve)` await at line 84 and the `conn.once("data", resolve)` await at line 105 could add `worker.once("error", reject)` / `conn.once("error", reject)`. In practice an unhandled `'error'` on these EventEmitters already crashes the subprocess with the message on stderr (which is in the `toMatchObject` diff), so this is convention-only — not a hang risk.
Comment thread
robobun marked this conversation as resolved.
conn.destroy();
`;

const wsDriver = `
const ws = new WebSocket("ws://127.0.0.1:" + port);
await new Promise((resolve, reject) => { ws.onopen = resolve; ws.onerror = reject; });
// Terminating the worker ends the connection under the client; swallow the
// resulting ErrorEvent so it does not surface as an uncaught error.
ws.onerror = () => {};
ws.send("go");
`;

// On Windows the usockets close path and Atomics.wait scheduling differ enough
// that the window does not open; the bug is platform-agnostic and is exercised
// on the POSIX lanes.
describe.skipIf(isWindows)(
"worker.terminate() mid-handler does not re-enter JS with a pending termination exception",
() => {
for (const [name, src] of [
["Bun.listen close handler (socket Handlers::call_error_handler)", parentSource(workerSource, listenDriver, "")],
[
"Bun.serve websocket message handler (WebSocketServerContext::run_error_callback)",
parentSource(wsWorkerSource, wsDriver, "ws.close();"),
],
] as const) {
test.concurrent(
name,
Comment thread
robobun marked this conversation as resolved.
async () => {
await using proc = Bun.spawn({
cmd: [bunExe(), "-e", src],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
// The unpatched build aborts inside an iteration (exit 134,
// "ASSERTION FAILED: !exception()" on stderr, no stdout). Assert on
// stdout/exitCode so benign debug-build stderr noise cannot cause a
// false positive; the crash's stderr is in the diff either way.
expect({ stdout: stdout.trim(), stderr, exitCode }).toMatchObject({
stdout: expect.stringMatching(/^ok [1-6]$/),
exitCode: 0,
});
},
60_000,
Comment thread
robobun marked this conversation as resolved.
);
}
},
);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Loading