Skip to content
Closed
Show file tree
Hide file tree
Changes from 8 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.
68 changes: 68 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 @@ -1077,6 +1093,20 @@ impl<const SSL: bool, const DEBUG: bool> NewServer<SSL, DEBUG> {
let server = user_route.server.cast_mut();
let index = user_route.id;

// Same stopped-server guard as the `on_request` trampoline: once the
Comment thread
robobun marked this conversation as resolved.
Outdated
// idle pass after a graceful `stop()` has run, the JS wrapper (which
// owns the route list this dispatch reads) may already be collected,
// so close late requests on surviving keep-alive connections instead
// of dispatching them.
// SAFETY: `server` is the live backref stored in `user_route`.
if unsafe { (*server).flags.contains(ServerFlags::HANDLERS_RELEASED) } {
// 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;
}

let should_deinit_context = core::cell::Cell::new(false);
let Some(mut prepared) = Self::prepare_js_request_context(
server,
Expand Down Expand Up @@ -1160,6 +1190,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 +1710,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
47 changes: 46 additions & 1 deletion src/runtime/server/server_body.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2191,10 +2191,30 @@ where
.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();
}
// Sockets opened under the old context keep dispatching
// through the same inline storage after the swap (each
// `ServerWebSocket` holds a `BackRef` to
// `config.websocket.handler`), so the live-socket count
// must follow them. A fresh zero would make
// `has_active_web_sockets()` report an idle server and let
// `deinit_if_we_can` release the new handlers while those
// sockets can still invoke them.
ws.handler
Comment thread
robobun marked this conversation as resolved.
Outdated
.active_connections
.set(old_ws.handler.active_connections.get());
}
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 +2258,13 @@ where
));
}
}

// 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();
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 Expand Up @@ -2806,6 +2833,15 @@ where
let server = unsafe { &mut *server_ptr };
let index = user_route.id;

// Same stopped-server guard as the H1 `on_user_route_request`
// trampoline: after the idle release the JS wrapper that owns the
// route list may already be collected.
if server.flags.contains(ServerFlags::HANDLERS_RELEASED) {
resp.write_status(b"503 Service Unavailable");
resp.end_without_body(true);
return;
}

let should_deinit_context = core::cell::Cell::new(false);
let Some(mut prepared) = server.prepare_js_request_context_for::<Ctx>(
req,
Expand Down Expand Up @@ -3189,6 +3225,15 @@ where
let server_ptr = server_ref.as_ptr();
let index = this.id;

// Same stopped-server guard as the request trampolines: after the
// idle release the JS wrapper that owns the route list may already be
// collected, so refuse late upgrades on surviving connections.
if server_ref.flags.contains(ServerFlags::HANDLERS_RELEASED) {
resp.write_status(b"503 Service Unavailable");
resp.end_without_body(true);
return;
}

let should_deinit_context = core::cell::Cell::new(false);
// SAFETY: `server_ptr` is the live heap server registered for this route;
// `req`/`resp` are the live uWS handles passed to the route handler.
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
Loading
Loading