Skip to content
Closed
Show file tree
Hide file tree
Changes from 6 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
8 changes: 6 additions & 2 deletions src/runtime/bake/DevServer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1247,6 +1247,10 @@ impl Drop for DevServer {
// SAFETY: stored ref from `init_from_any_blob`; no live borrow.
unsafe { StaticRoute::deref_(cached.as_ptr()) };
}
// SAFETY: paired with the `ref_` taken in
// `get_or_put_route_bundle` when this bundle was created; the
// raw `html_bundle` field has no Drop, so release it here.
unsafe { bun_ptr::RefCount::<HTMLBundleRoute>::deref(html.html_bundle) };
}
}

Expand Down Expand Up @@ -5376,8 +5380,8 @@ impl DevServer {
let file = &mut self.client_graph.bundled_files.values_mut()
[incremental_graph_index.get() as usize];
file.html_route_bundle_index = Some(bundle_index);
// Bump the intrusive refcount; matched by
// `RouteBundle::deinit`'s deref of `html_bundle`.
// Bump the intrusive refcount; matched by the
// `html_bundle` deref in `DevServer`'s `Drop`.
// SAFETY: `html` is a live IntrusiveRc-managed allocation.
unsafe { bun_ptr::RefCount::<HTMLBundleRoute>::ref_(html) };
break 'brk route_bundle::Data::Html(route_bundle::Html {
Expand Down
11 changes: 6 additions & 5 deletions src/runtime/bake/dev_server/route_bundle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -164,8 +164,9 @@ impl RouteBundle {
}
}

// `deinit` is fully subsumed by Drop:
// - client_bundle / cached_response: Option<Arc<StaticRoute>> drop = .deref()
// - Framework: StrongOptional fields drop = .deinit()
// - Html: bundled_html_text Box<[u8]> drop = allocator.free()
// html_bundle RefPtr drop = .deref()
// Zig `RouteBundle.deinit` equivalent, split across two mechanisms:
// - Drop: `Framework` StrongOptional fields (= .deinit()) and
// `Html.bundled_html_text` Box<[u8]> (= allocator.free()).
// - `DevServer`'s `Drop` (explicit): `client_bundle`, `Html.cached_response`
// (BackRef, no Drop) and `Html.html_bundle` (raw ptr, no Drop) each hold
// an intrusive ref that is deref'd there.
54 changes: 54 additions & 0 deletions src/runtime/server/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,7 @@ bitflags::bitflags! {
const DEINIT_SCHEDULED = 1 << 0;
const TERMINATED = 1 << 1;
const HAS_HANDLED_ALL_CLOSED_PROMISE = 1 << 2;
const HANDLERS_RELEASED = 1 << 3;
}
}

Expand Down Expand Up @@ -1026,6 +1027,21 @@ impl<const SSL: bool, const DEBUG: bool> NewServer<SSL, DEBUG> {
req: &mut uws_sys::Request,
resp: *mut uws_sys::NewAppResponse<SSL>,
) {
// `deinit_if_we_can` releases the handler Strongs once the server is
// fully idle after a graceful `stop()`. A surviving keep-alive
// connection can still deliver one more request after that; there is
// nothing left to dispatch it to, so close the connection instead of
// calling a released handler.
// SAFETY: `this` is the live server backref registered as the uws
// userdata; no other borrow derived from it is alive here.
if unsafe { (*this).config.on_request.is_none() } {
Comment thread
robobun marked this conversation as resolved.
// S012: `NewAppResponse<SSL>` is a ZST opaque — safe deref.
let resp = bun_opaque::opaque_deref_mut(resp);
resp.write_status(b"503 Service Unavailable");
resp.end_without_body(true);
return;
}
Comment thread
robobun marked this conversation as resolved.
Outdated

let should_deinit_context = core::cell::Cell::new(false);
let Some(prepared) = Self::prepare_js_request_context(
this,
Expand Down Expand Up @@ -1160,6 +1176,19 @@ impl<const SSL: bool, const DEBUG: bool> NewServer<SSL, DEBUG> {
use bun_http_jsc::method_jsc::MethodJsc as _;
use node_http_response::Flags as NhrFlags;

// `deinit_if_we_can` releases the handler Strongs once the server is
Comment thread
robobun marked this conversation as resolved.
Outdated
// fully idle after a graceful `stop()`. A surviving keep-alive
// connection can still deliver one more request after that; there is
// nothing left to dispatch it to, so close the connection instead of
// forwarding a released callback to the C++ request handler.
// SAFETY: `this` is the live server backref registered as the uws
// userdata; no other borrow derived from it is alive here.
if unsafe { (*this).config.on_node_http_request.is_none() } {
Comment thread
robobun marked this conversation as resolved.
resp.write_status(b"503 Service Unavailable");
resp.end_without_body(true);
return;
}

// SAFETY: `this` is the live server backref registered as the uws
// userdata; only one borrow derived from it is alive at a time.
unsafe { (*this).on_pending_request() };
Expand Down Expand Up @@ -1667,6 +1696,31 @@ impl<const SSL: bool, const DEBUG: bool> NewServer<SSL, DEBUG> {
if let Some(ws) = self.config.websocket.as_mut() {
ws.handler.app = None;
}
// No request or upgrade can reach the user's handler callbacks once
// the listener is gone, the in-flight count is zero, and there are
// no live websockets. Release their `Strong` handles now: a handler
// defined in a scope that closes over the JS `Server` value
// otherwise forms a native↔JS cycle (box → Strong(handler) →
// lexical environment → Server wrapper → m_ctx → box) that the GC
// cannot see through, so the wrapper never finalizes and the box is
// never freed.
//
// The `Option<Strong>` / `StrongOptional` releases are idempotent,
// so they run on every idle pass (a reload on a stopped server may
// have installed fresh handlers since the last one). The websocket
// unprotect is counted per value, so it runs at most once per
// protected context: HANDLERS_RELEASED is set here and cleared by
// `on_reload_from_zig` when it installs a new protected context.
self.config.on_request = None;
Comment thread
robobun marked this conversation as resolved.
self.config.on_node_http_request = None;
self.config.on_error = None;
self.on_clienterror.deinit();
if !self.flags.contains(ServerFlags::HANDLERS_RELEASED) {
self.flags.insert(ServerFlags::HANDLERS_RELEASED);
if let Some(ws) = self.config.websocket.as_mut() {
Comment thread
robobun marked this conversation as resolved.
Outdated
ws.handler.unprotect();
Comment thread
robobun marked this conversation as resolved.
Comment thread
robobun marked this conversation as resolved.
}
}
self.unref();

// Detach DevServer. This is needed because there are aggressive
Expand Down
18 changes: 17 additions & 1 deletion src/runtime/server/server_body.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2191,10 +2191,19 @@
.set(super::web_socket_server_context::HandlerFlags::SSL, SSL);
if !ws.handler.on_message.is_empty() || !ws.handler.on_open.is_empty() {
if let Some(old_ws) = self.config.websocket.as_ref() {
old_ws.unprotect();
// Skip when an idle `deinit_if_we_can` already released the
// old context's protections (stopped server being
// reloaded); a second unprotect would unbalance the
// per-value protect counts.
if !self.flags.contains(ServerFlags::HANDLERS_RELEASED) {
old_ws.unprotect();
}
}
ws.global_object = bun_ptr::BackRef::new(global);
self.config.websocket = Some(ws);
// The newly installed context holds fresh protections; let the
// next idle pass release them.
self.flags.remove(ServerFlags::HANDLERS_RELEASED);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
} else {
// Not adopting it: release the protections taken in
// `WebSocketServerContext::on_create` so the handlers don't leak.
Expand Down Expand Up @@ -2238,6 +2247,13 @@
));
}
}

// A stopped (idle) server can never invoke the handlers this reload
// just installed, and they would otherwise pin the JS wrapper through
// the native↔JS cycle the idle release exists to break. Re-run the
// idle pass so they are released immediately. No-op while the server
// is still listening or has work in flight.
self.deinit_if_we_can();

Check failure on line 2256 in src/runtime/server/server_body.rs

View check run for this annotation

Claude / Claude Code Review

reload() after graceful stop with live websockets unprotects the active handler

The comment here ("a stopped (idle) server can never invoke the handlers this reload just installed") is not true when `stop()` was graceful and websockets opened before the reload are still connected: the swap at line 2203 resets `active_connections` to 0 at the same address the live `ServerWebSocket` `BackRef`s point into, so `has_active_web_sockets()` reads the new zero counter and this `deinit_if_we_can()` unprotects the freshly installed handler while those sockets are still dispatching to
Comment thread
robobun marked this conversation as resolved.
Outdated
Comment thread
robobun marked this conversation as resolved.
Outdated
}

pub fn reload_static_routes(&mut self) -> Result<bool, bun_core::Error> {
Expand Down
4 changes: 3 additions & 1 deletion test/bake/deinitialization.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,6 @@ test("dev server deinitializes itself", () => {
});
expect(result.signalCode).toBeUndefined();
expect(result.exitCode).toBe(0);
});
// The child runs a whole `bun test` suite (nine GC-heavy cases plus leak
// reporting at exit), which takes longer than the 5s default under ASAN.
}, 60_000);
50 changes: 47 additions & 3 deletions test/bake/fixtures/deinitialization/test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { getDevServerDeinitCount } from "bun:internal-for-testing";
import html from "./index.html";
import { expect, test } from "bun:test";
import { fullGC } from "bun:jsc";
import { afterAll, expect, test } from "bun:test";
import { fullGC, heapStats } from "bun:jsc";

expect(process.cwd()).toBe(import.meta.dir);

Expand Down Expand Up @@ -71,7 +71,15 @@ async function run({ closeActiveConnections = false, sendAnyRequests = true, web
expect(fetch(server.url.origin, { keepalive: false })).rejects.toThrow("Unable to connect");
}

await main();
try {
await main();
} finally {
// The closure assigned to `globalThis.callback` inside `main()` captures
// `server`; left in place it roots the JS Server wrapper through every GC
// below, so the wrapper never finalizes and the native NewServer box (and
// everything its config owns) is still live at process exit.
globalThis.callback = undefined;
}

if (closeActiveConnections) {
await promise;
Expand Down Expand Up @@ -109,6 +117,42 @@ const cases = [
{ closeActiveConnections: true, sendAnyRequests: false, websocket: 8 },
];

function liveServerWrappers() {
const c = heapStats().objectTypeCounts;
return (c.HTTPServer ?? 0) + (c.DebugHTTPServer ?? 0) + (c.HTTPSServer ?? 0) + (c.DebugHTTPSServer ?? 0);
}
// `objectTypeCounts` includes the (lazily created) prototype object once the
// first server has been constructed. Create-and-stop one trivial server here
// so the prototype is materialized but the instance is freed; the afterAll
// check then asserts every dev-server case returns to this baseline (i.e. zero
// live wrapper instances and the native boxes were actually freed).
let serverWrapperBaseline: number;
test("baseline: stopped server wrapper collects", async () => {
await (async () => {
const server = Bun.serve({ port: 0, fetch: () => new Response("ok") });
server.stop(true);
})();
for (let i = 0; i < 10 && liveServerWrappers() > 1; i++) {
Comment thread
robobun marked this conversation as resolved.
Outdated
Bun.gc(true);
fullGC();
await new Promise(resolve => setTimeout(resolve, 100));
}
serverWrapperBaseline = liveServerWrappers();
expect(serverWrapperBaseline).toBeLessThanOrEqual(1);
});

afterAll(async () => {
// Drain any deferred deinit task scheduled during the final case's GC, then
// assert every JS Server wrapper has actually been collected — i.e. the
// native NewServer boxes are freed, not just the embedded dev servers.
for (let i = 0; i < 10 && liveServerWrappers() > serverWrapperBaseline; i++) {
Comment thread
robobun marked this conversation as resolved.
Outdated
Bun.gc(true);
fullGC();
await new Promise(resolve => setTimeout(resolve, 100));
}
expect(liveServerWrappers()).toBe(serverWrapperBaseline);
});

for (const { closeActiveConnections, sendAnyRequests, websocket } of cases) {
test(
"flags: " +
Expand Down
63 changes: 63 additions & 0 deletions test/js/bun/http/bun-server.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -613,6 +613,69 @@ test("should be able to await server.stop(true) with keep alive", async () => {
expect(async () => await fetch(server.url)).toThrow();
});

test("request on a surviving keep-alive connection after stop() closes instead of crashing", async () => {
// A graceful stop() releases the handler references once nothing is in
// flight, but an already-accepted keep-alive connection can still deliver
// one more request afterwards. That request must close the connection, not
// dispatch into the released handler.
const script = /* js */ `
const server = Bun.serve({
port: 0,
hostname: "127.0.0.1",
fetch() { return new Response("ok"); },
});

const firstResponse = Promise.withResolvers();
const outcome = Promise.withResolvers();
let buffered = "";
let stopped = false;
const socket = await Bun.connect({
hostname: "127.0.0.1",
port: server.port,
socket: {
data(_socket, chunk) {
buffered += chunk.toString();
if (!stopped) {
if (buffered.includes("\\r\\n\\r\\nok")) {
buffered = "";
firstResponse.resolve();
}
return;
}
if (buffered.includes("HTTP/1.1 200")) outcome.resolve("served");
},
close() { outcome.resolve("closed"); },
error() { outcome.resolve("closed"); },
},
});

socket.write("GET / HTTP/1.1\\r\\nHost: localhost\\r\\nConnection: keep-alive\\r\\n\\r\\n");
await firstResponse.promise;

// Nothing is in flight: the handler release runs inside stop().
server.stop();
stopped = true;

socket.write("GET / HTTP/1.1\\r\\nHost: localhost\\r\\n\\r\\n");
const result = await outcome.promise;
if (result !== "closed") throw new Error("late request was served after stop(): " + result);
console.log("survived");
`;

await using proc = Bun.spawn({
cmd: [bunExe(), "-e", script],
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("survived\n");
expect(exitCode).toBe(0);
});

test("should be able to async upgrade using custom protocol", async () => {
const { promise, resolve } = Promise.withResolvers<{ code: number; reason: string } | boolean>();
using server = Bun.serve<unknown>({
Expand Down
Loading
Loading