Skip to content
Closed
Show file tree
Hide file tree
Changes from 4 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
7 changes: 4 additions & 3 deletions src/jsc/web_worker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1325,9 +1325,10 @@ impl WebWorker {
}

// The finalizers JSC just ran close the sockets that `close_all_socket_groups` leaves
// alone (a Listener owns its listen socket and closes it in `finalize`). `us_socket_close`
// only queues onto `loop->data.closed_head`; step 5's `on_thread_exit()` frees the loop
// out from under whatever is still queued, so drain it now, while the loop is alive.
// alone (a `Listener` / `NewServer` owns its listen socket and closes it in `finalize`).
// `us_socket_close` only queues onto `loop->data.closed_head`; step 5's `on_thread_exit()`
// frees the loop out from under whatever is still queued, so drain it now, while the loop
// is alive.
Comment thread
robobun marked this conversation as resolved.
if !vm_ptr.is_null() {
// SAFETY: `vm_ptr` was unpublished under `vm_lock`; sole owner, `destroy()` is below.
unsafe { (*vm_ptr).uws_loop_mut().drain_closed_sockets() };
Expand Down
21 changes: 21 additions & 0 deletions src/runtime/server/server_body.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2751,7 +2751,28 @@
// hold a ref), so hand ownership back to the raw teardown path.
let this = bun_core::heap::release(self);
this.js_value.finalize();
// `listener` set here ⇒ VM teardown (`lastChanceToFinalize`): the
// Strong `js_value` root kept the wrapper alive through every ordinary
// GC. `close_all_socket_groups` skipped listen sockets for us to close
// (matching `Listener::finalize`); not doing so leaks the fd/port past
// worker exit.
Comment thread
robobun marked this conversation as resolved.
if let Some(listener) = this.listener.take() {
if let server_config::Address::Unix(path) = &this.config.address {
let bytes = path.as_bytes();
if !bytes.is_empty() && bytes[0] != 0 {
let _ = bun_sys::unlink(path.as_zstr());
}
}
bun_opaque::opaque_deref_mut(listener).close();
}
Comment thread
robobun marked this conversation as resolved.
// Not `h3_listener.close()`: `us_quic_listen_socket_close` runs
// `lsquic_conn_abort` + `us_quic_process` → JS `on_abort` for any live
// QUIC conn (which `close_all_socket_groups` never drained), and JS is
// unsound inside `lastChanceToFinalize`.
Comment thread
robobun marked this conversation as resolved.
if Self::HAS_H3 {
this.h3_listener = None;
}
Comment thread
robobun marked this conversation as resolved.
this.deinit_if_we_can();

Check warning on line 2775 in src/runtime/server/server_body.rs

View check run for this annotation

Claude / Claude Code Review

schedule_deinit() now reachable from lastChanceToFinalize enqueues never-drained tasks

Nit: now that `finalize()` clears `listener`/`h3_listener` before `deinit_if_we_can()`, the block at `mod.rs:1750` is entered from `lastChanceToFinalize()` with `js_value == Finalized` and reaches `schedule_deinit()` (mod.rs:1774), which `vm.enqueue_task()`s two `ManagedTask`s that no event-loop tick will ever drain — `EventLoop::deinit()` drops them without running the callbacks, so `app.close()` / `Self::deinit(this)` still never run. Not a regression (the `NewServer`/`App` leaked pre-PR too;
Comment thread
robobun marked this conversation as resolved.
}

pub fn get_all_closed_promise(&mut self, global: &JSGlobalObject) -> JSValue {
Expand Down
81 changes: 80 additions & 1 deletion test/js/web/workers/worker-terminate-lifetime.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { expect, test } from "bun:test";
import { bunEnv, bunExe, isASAN, isDebug } from "harness";
import { bunEnv, bunExe, isASAN, isDebug, isLinux } from "harness";
import { join } from "path";

// Worker VM startup/teardown is much slower under debug and/or ASAN; these
Expand Down Expand Up @@ -176,3 +176,82 @@ test.skipIf(!isASAN)(
},
timeout,
);

// Regression: NewServer::finalize() never closed its listen socket. Worker
// shutdown's close_all_socket_groups() deliberately skips listen sockets on
// the assumption the owner closes them in finalize (Bun.listen's Listener
// does), so a Bun.serve() server in a terminated worker leaked its listen fd
// and the port stayed bound for the life of the process. All three exit modes
// below converge on WebWorker::shutdown() -> lastChanceToFinalize().
test.each([
["parent terminate()", "", true],
["worker process.exit()", " setImmediate(() => process.exit(0));", false],
["worker unref() + drain", " s.unref();", false],
])(
"Bun.serve() in a worker releases the listening port on %s",
async (_name, tail, parentTerminates) => {
await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`
const serveWorker =
"const s = Bun.serve({ port: 0, fetch: () => new Response('ok') });" +
"postMessage(s.port);" + ${JSON.stringify(tail)};

async function cycle() {
const w = new Worker("data:text/javascript," + encodeURIComponent(serveWorker));
const port = await new Promise(r => w.addEventListener("message", e => r(e.data), { once: true }));
const closed = new Promise(r => w.addEventListener("close", r, { once: true }));
${parentTerminates ? "w.terminate();" : ""}
await closed;
Comment thread
robobun marked this conversation as resolved.
// Worker has fully exited; its listen socket must be gone. Rebind
// fails with EADDRINUSE if the old listener is still alive.
const s = Bun.serve({ port, reusePort: false, fetch: () => new Response("x") });
s.stop(true);
return port;
}

// One warm-up cycle so lazily-created per-process fds (DNS, event loop
// timers, module cache) do not count against the leak delta below.
await cycle();

const fdCount = ${isLinux}
? () => require("fs").readdirSync("/proc/self/fd").length
: () => 0;
const before = fdCount();

const ports = [];
for (let i = 0; i < 5; i++) ports.push(await cycle());

// The 'close' event fires from WebWorker__dispatchExit (shutdown step
// 4); the detached worker thread then still runs step 5 (vm.destroy +
// on_thread_exit) which closes the per-worker epoll/eventfd. Poll the
// fd count with a bounded deadline so the last cycle's worker has
// reached that point instead of relying on gc() as an implicit sleep.
let fdDelta = fdCount() - before;
for (let i = 0; fdDelta > 1 && i < ${slow ? 500 : 100}; i++) {
await Bun.sleep(10);
fdDelta = fdCount() - before;
Comment thread
robobun marked this conversation as resolved.
}
console.log(JSON.stringify({ ports: ports.length, fdDelta }));
`,
],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stderr).toBe("");
const { ports, fdDelta } = JSON.parse(stdout.trim());
// Every cycle rebound its port: EADDRINUSE would have thrown before here.
expect(ports).toBe(5);
if (isLinux) {
// Unfixed: one listen fd per cycle (>= 5).
expect(fdDelta).toBeLessThanOrEqual(1);
}
Comment thread
robobun marked this conversation as resolved.
expect(exitCode).toBe(0);
},
timeout,
);
Loading