Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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
15 changes: 15 additions & 0 deletions src/jsc/bindings/bindings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4899,6 +4899,21 @@ bool JSC__VM__hasTerminationRequest(JSC::VM* vm)
return vm->hasTerminationRequest();
}

// InternalMicrotask::BunPerformMicrotaskJob catches any exception from the
// job callback with an unconditional clearException() and then calls
// Bun__reportUnhandledError with the Exception*. When the caught exception
// is the TerminationException that clear spends the one shot the
// NeedTermination trap fired, so the microtask drain never observes
// termination and a worker in a microtask-bound loop can spin forever.
// Re-establishing the TerminationException here lets MicrotaskQueue's
// runMicrotask see it on return and break out of the drain.
Comment thread
robobun marked this conversation as resolved.
Outdated
[[ZIG_EXPORT(nothrow)]]
void JSC__VM__rethrowTerminationException(JSC::VM* vm)
{
if (vm->hasTerminationRequest() && !vm->hasPendingTerminationException())
vm->throwTerminationException();
}

void JSC__VM__setExecutionForbidden(JSC::VM* arg0, bool arg1)
{
(*arg0).setExecutionForbidden();
Expand Down
12 changes: 7 additions & 5 deletions src/jsc/virtual_machine_exports.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,12 +117,14 @@
pub fn report_unhandled_error(global: &JSGlobalObject, value: JSValue) -> JSValue {
crate::mark_binding!();

if !value.is_termination_exception() {
let _ = global
.bun_vm()
.as_mut()
.uncaught_exception(global, value, false);
if value.is_termination_exception() {
crate::cpp::JSC__VM__rethrowTerminationException(global.vm());
return JSValue::UNDEFINED;
}

Check failure on line 123 in src/jsc/virtual_machine_exports.rs

View check run for this annotation

Claude / Claude Code Review

EventEmitter listener loop can trip assertNoException() after re-throwing TerminationException

Now that `Bun__reportUnhandledError` re-throws the TerminationException, `EventEmitter::innerInvokeEventListeners` (src/jsc/bindings/webcore/EventEmitter.cpp:259) can return from the report call with the exception pending and then continue the loop into the next listener's `JSC::call()`, tripping `executeCallImpl`'s `assertNoException()` and SIGABRTing the process in debug builds. Previously the report call was a no-op for termination, so this is a regression at that one caller — add a `if (vm.i
Comment thread
robobun marked this conversation as resolved.
let _ = global
.bun_vm()
.as_mut()
.uncaught_exception(global, value, false);
JSValue::UNDEFINED
}

Expand Down
13 changes: 10 additions & 3 deletions src/runtime/webcore/Body.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1688,12 +1688,19 @@ pub(crate) trait BodyMixin: BodyOwnerJs + Sized {
/// Migrate any `Locked.readable` strong ref
/// into the GC-traced `js.gc.stream` slot to break the cycle (the JS
/// wrapper owns the stream; native side must not hold it strongly).
///
/// Called from constructors after the native object has been heap-allocated,
/// so it must not reach a VMTraps safepoint (a TerminationException thrown
/// here would leave the generated `construct` holding a non-null ptr with
/// an exception pending, tripping its leak assertion). `Strong::value()`
/// is a pure slot read; `Strong::get()` would re-tag through `from_js`,
/// which is a safepoint.
Comment thread
robobun marked this conversation as resolved.
Outdated
fn check_body_stream_ref(&self, global_object: &JSGlobalObject) {
if let Some(js_value) = self.js_ref() {
if let Value::Locked(locked) = self.get_body_value() {
if let Some(stream) = locked.readable.get(global_object) {
stream.value.ensure_still_alive();
Self::stream_set_cached(js_value, global_object, stream.value);
if let Some(stream_value) = locked.readable.value() {
stream_value.ensure_still_alive();
Self::stream_set_cached(js_value, global_object, stream_value);
locked.readable.downgrade();
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/runtime/webcore/ReadableStream.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ impl Default for Strong {
}

impl Strong {
fn value(&self) -> Option<JSValue> {
pub(crate) fn value(&self) -> Option<JSValue> {
self.held.get().or_else(|| {
if self.weak.is_empty() {
None
Expand Down
67 changes: 67 additions & 0 deletions test/js/web/workers/worker-terminate-lifetime.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,73 @@
timeout,
);

// Regression: InternalMicrotask::BunPerformMicrotaskJob (used by
// queueMicrotask and by the C++ stream start/pull reaction jobs) catches any
// exception from the job callback with an unconditional clearException(), so
// when the caught exception is the TerminationException the microtask drain
// never observes termination. A worker in a microtask-bound loop that
// constructs a JS-source ReadableStream (or calls queueMicrotask) each turn
// would spin forever with terminate() never resolving.
// The Response / Request variants additionally cover a trap safepoint inside
// check_body_stream_ref that fired the TerminationException after the native
// Response/Request had been heap-allocated, tripping the generated
// constructor's "Memory leak detected: new Response()" assertion.
describe("terminate() resolves for a worker in a microtask-bound ReadableStream loop", () => {
const variants: Record<string, string> = {
"new ReadableStream({pull})": `new ReadableStream({ pull(c) { c.enqueue(1); c.close(); } })`,
"new ReadableStream({start})": `new ReadableStream({ start(c) { c.close(); } })`,
"new Response(new ReadableStream)": `new Response(new ReadableStream({ async pull() {} }))`,
"new Request(new ReadableStream)": `new Request("http://x", { method: "POST", body: new ReadableStream({ pull(c) { c.close(); } }), duplex: "half" })`,
"queueMicrotask": `queueMicrotask(() => {})`,
};
// A single hang is deterministic on an unfixed build (the loop is purely
// microtask-bound), so a small sweep of offsets is plenty.
const localRounds = slow ? 3 : 6;
const deadline = slow ? 10_000 : 4_000;

for (const [name, expr] of Object.entries(variants)) {
test.concurrent(
name,
async () => {
await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`
const { Worker } = require("node:worker_threads");
const src = 'require("node:worker_threads").parentPort.postMessage("up");' +
'(async () => { for (;;) { ' + ${JSON.stringify(expr)} + '; await 0; } })();';
for (let r = 0; r < ${localRounds}; r++) {
const w = new Worker(src, { eval: true });
w.on("error", () => {});
await new Promise((res) => w.once("message", res));
await Bun.sleep((r * 23) % 80);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
const winner = await Promise.race([
w.terminate().then(() => "ok"),
Bun.sleep(${deadline}).then(() => "hung"),
]);
if (winner !== "ok") {
console.log("HUNG round " + r);
process.exit(1);
}
}
console.log("PASS");
`,

Check warning on line 231 in test/js/web/workers/worker-terminate-lifetime.test.ts

View check run for this annotation

Claude / Claude Code Review

Dangling Bun.sleep(deadline) timers keep test subprocess alive after PASS

The `Bun.sleep(${deadline})` loser of each `Promise.race` is never cancelled, so after `console.log("PASS")` the subprocess idles until the last-armed timer fires — ~4s on release, ~10s on debug/ASAN — before it can exit. Add `process.exit(0)` after `console.log("PASS")` (or race a clearable `setTimeout` and clear it when `terminate()` wins) to shave that dead time from every CI run.
Comment thread
robobun marked this conversation as resolved.
],
env: bunEnv,
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("PASS\n");
expect(exitCode).toBe(0);
},
timeout,
);
}
});

// Regression: Bun.serve() inside a worker, streaming a JS ReadableStream body,
// then worker.terminate() mid-stream. Worker shutdown stops the server which
// tears down the in-flight HTTP(S)ResponseSink and fires its JS onClose hook
Expand Down
Loading