diff --git a/src/runtime/bake/DevServer.rs b/src/runtime/bake/DevServer.rs index f697101b5d75..d8d062018474 100644 --- a/src/runtime/bake/DevServer.rs +++ b/src/runtime/bake/DevServer.rs @@ -1247,6 +1247,8 @@ 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_` in `get_or_put_route_bundle`. + unsafe { bun_ptr::RefCount::::deref(html.html_bundle) }; } } @@ -5376,8 +5378,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::::ref_(html) }; break 'brk route_bundle::Data::Html(route_bundle::Html { diff --git a/src/runtime/bake/dev_server/route_bundle.rs b/src/runtime/bake/dev_server/route_bundle.rs index 3736176cc045..b7ebd41fb5e2 100644 --- a/src/runtime/bake/dev_server/route_bundle.rs +++ b/src/runtime/bake/dev_server/route_bundle.rs @@ -164,8 +164,6 @@ impl RouteBundle { } } -// `deinit` is fully subsumed by Drop: -// - client_bundle / cached_response: Option> drop = .deref() -// - Framework: StrongOptional fields drop = .deinit() -// - Html: bundled_html_text Box<[u8]> drop = allocator.free() -// html_bundle RefPtr drop = .deref() +// Drop covers the `Framework` Strongs and `bundled_html_text`; +// `client_bundle`, `cached_response`, and `html_bundle` hold intrusive refs +// released in `DevServer`'s `Drop`. diff --git a/src/runtime/server/ServerWebSocket.rs b/src/runtime/server/ServerWebSocket.rs index 5fb0c0e297b4..9d996a8cb967 100644 --- a/src/runtime/server/ServerWebSocket.rs +++ b/src/runtime/server/ServerWebSocket.rs @@ -388,8 +388,10 @@ impl ServerWebSocket { let handler = self.handler(); let vm = handler.vm(); - // The handler is shared (&), so mutate via the interior-mutability helper. - handler.active_connections_saturating_add(1); + let server = handler.server; + if let Some(server) = server { + server.on_websocket_opened(); + } let global_object = handler.global_object(); let on_open_handler = handler.on_open; if vm.is_shutting_down() { @@ -426,17 +428,25 @@ impl ServerWebSocket { if let Some(err_value) = result.to_error() { bun_output::scoped_log!(WebSocketServer, "onOpen exception"); + let mut closed_here = false; if !self.flags.get().closed() { self.update_flags(|f| f.set_closed(true)); // we un-gracefully close the connection if there was an exception // we don't want any event handlers to fire after this for anything other than error() // https://github.com/oven-sh/bun/issues/1480 self.websocket().close(); - handler.active_connections_saturating_sub(1); + closed_here = true; this_value.unprotect(); } handler.run_error_callback(vm, global_object, err_value); + if closed_here { + // The re-entrant `on_close` above skipped its accounting + // because the closed flag was already set. + if let Some(server) = server { + server.on_websocket_closed(); + } + } } } @@ -633,11 +643,16 @@ impl ServerWebSocket { bun_output::scoped_log!(WebSocketServer, "onClose"); // TODO: Can this called inside finalize? let handler = self.handler(); + // Copied out: the deferred call may form `&mut NewServer`, which owns + // the handler storage. + let server = handler.server; let was_closed = self.is_closed(); self.update_flags(|f| f.set_closed(true)); scopeguard::defer! { if !was_closed { - handler.active_connections_saturating_sub(1); + if let Some(server) = server { + server.on_websocket_closed(); + } } } let signal = self.signal.take(); @@ -1349,8 +1364,13 @@ impl ServerWebSocket { break 'brk args.ptr[1].to_slice_or_null(global_this)?; }; + let server = self.handler().server; self.update_flags(|f| f.set_closed(true)); self.websocket().end(code, message_value.slice()); + // The re-entrant `on_close` skipped its accounting (closed was set). + if let Some(server) = server { + server.on_websocket_closed(); + } Ok(JSValue::UNDEFINED) } @@ -1368,8 +1388,13 @@ impl ServerWebSocket { return Ok(JSValue::UNDEFINED); } + let server = self.handler().server; self.update_flags(|f| f.set_closed(true)); self.websocket().close(); + // The re-entrant `on_close` skipped its accounting (closed was set). + if let Some(server) = server { + server.on_websocket_closed(); + } Ok(JSValue::UNDEFINED) } diff --git a/src/runtime/server/WebSocketServerContext.rs b/src/runtime/server/WebSocketServerContext.rs index 2548f670af85..3b1ed6518734 100644 --- a/src/runtime/server/WebSocketServerContext.rs +++ b/src/runtime/server/WebSocketServerContext.rs @@ -34,10 +34,8 @@ pub struct Handler { // LIFETIMES.tsv = STATIC (vm) / JSC_BORROW (global_object) — both outlive the handler. pub vm: bun_ptr::BackRef, pub global_object: bun_ptr::BackRef, - /// Mutated through `&Handler` (the field is owned by - /// `ServerConfig.websocket` and only ever touched on the JS thread), so - /// it's a `Cell`. - pub active_connections: core::cell::Cell, + /// Backref to the owning `NewServer`; set in `set_routes` alongside `app`. + pub server: Option, /// used by publish() pub flags: HandlerFlags, @@ -68,19 +66,6 @@ impl Handler { self.vm.get() } - #[inline] - pub fn active_connections_saturating_add(&self, n: usize) { - self.active_connections - .set(self.active_connections.get().saturating_add(n)); - } - - /// See `active_connections_saturating_add`. - #[inline] - pub fn active_connections_saturating_sub(&self, n: usize) { - self.active_connections - .set(self.active_connections.get().saturating_sub(n)); - } - pub fn run_error_callback( &self, vm: &VirtualMachine, @@ -119,7 +104,7 @@ impl Handler { app: None, vm: bun_ptr::BackRef::new(VirtualMachine::get()), global_object: bun_ptr::BackRef::new(global_object), - active_connections: core::cell::Cell::new(0), + server: None, flags: HandlerFlags::empty(), }; @@ -172,18 +157,26 @@ impl Handler { self.on_pong.protect(); } - pub fn unprotect(&self) { + pub fn unprotect(&mut self) { if self.vm.is_shutting_down() { return; } - self.on_open.unprotect(); - self.on_message.unprotect(); - self.on_close.unprotect(); - self.on_drain.unprotect(); - self.on_error.unprotect(); - self.on_ping.unprotect(); - self.on_pong.unprotect(); + // Zero the slots so in-flight dispatches (e.g. a `message` handler + // that closed the last socket of a stopped server) see empty values + // instead of unrooted cells. + for field in [ + &mut self.on_open, + &mut self.on_message, + &mut self.on_close, + &mut self.on_drain, + &mut self.on_error, + &mut self.on_ping, + &mut self.on_pong, + ] { + field.unprotect(); + *field = JSValue::ZERO; + } } } @@ -206,7 +199,7 @@ impl WebSocketServerContext { self.handler.protect(); } - pub fn unprotect(&self) { + pub fn unprotect(&mut self) { self.handler.unprotect(); } } diff --git a/src/runtime/server/mod.rs b/src/runtime/server/mod.rs index 9a5e5f90b6f4..38685b748e33 100644 --- a/src/runtime/server/mod.rs +++ b/src/runtime/server/mod.rs @@ -220,6 +220,9 @@ bitflags::bitflags! { const DEINIT_SCHEDULED = 1 << 0; const TERMINATED = 1 << 1; const HAS_HANDLED_ALL_CLOSED_PROMISE = 1 << 2; + const HANDLERS_RELEASED = 1 << 3; + /// Transient: held across the abrupt-stop `app.close()` drain. + const WEBSOCKETS_DRAINING = 1 << 4; } } @@ -265,6 +268,9 @@ pub struct NewServer { pub base_url_string_for_joining: Box<[u8]>, pub config: ServerConfig, pub pending_requests: usize, + /// Live `ServerWebSocket` count; `Cell` because updates arrive through + /// shared `AnyServer` handles. + pub active_websocket_count: core::cell::Cell, pub request_pool: *mut request_context::RequestContextStackAllocator, /// Null until the H3 listen path runs (`HAS_H3 && config.http3`); never /// allocated when `!SSL`. Kept as a raw nullable pointer rather than a @@ -1026,6 +1032,16 @@ impl NewServer { req: &mut uws_sys::Request, resp: *mut uws_sys::NewAppResponse, ) { + // A keep-alive connection can deliver a request after the idle pass + // released the handlers; close it instead of calling a freed 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() } { + // S012: `NewAppResponse` is a ZST opaque — safe deref. + server_body::reject_stopped(bun_opaque::opaque_deref_mut(resp)); + return; + } + let should_deinit_context = core::cell::Cell::new(false); let Some(prepared) = Self::prepare_js_request_context( this, @@ -1040,13 +1056,7 @@ impl NewServer { // SAFETY: `this` is the live server backref for this request. let server = unsafe { &*this }; - let on_request = server - .config - .on_request - .as_ref() - .map(|s| s.get()) - .unwrap_or(JSValue::ZERO); - debug_assert!(!on_request.is_empty()); + let on_request = server.config.on_request.as_ref().unwrap().get(); let global = server.global_this(); let js_value = server.js_value_assert_alive(); @@ -1077,6 +1087,14 @@ impl NewServer { let server = user_route.server.cast_mut(); let index = user_route.id; + // Same late-503 path as `on_request`. + // SAFETY: `server` is the live backref stored in `user_route`. + if unsafe { (*server).flags.contains(ServerFlags::HANDLERS_RELEASED) } { + // S012: `NewAppResponse` is a ZST opaque — safe deref. + server_body::reject_stopped(bun_opaque::opaque_deref_mut(resp)); + return; + } + let should_deinit_context = core::cell::Cell::new(false); let Some(mut prepared) = Self::prepare_js_request_context( server, @@ -1160,6 +1178,14 @@ impl NewServer { use bun_http_jsc::method_jsc::MethodJsc as _; use node_http_response::Flags as NhrFlags; + // Same late-503 path as `on_request`. + // 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() } { + server_body::reject_stopped(resp); + 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() }; @@ -1205,12 +1231,7 @@ impl NewServer { }, None => JSValue::UNDEFINED, }; - let callback = this_ref - .config - .on_node_http_request - .as_ref() - .map(|s| s.get()) - .unwrap_or(JSValue::ZERO); + let callback = this_ref.config.on_node_http_request.as_ref().unwrap().get(); // C++ forwards `any_server` to `NodeHTTPResponse::create`, which // unpacks it via `any_server_from_packed` (bits 49..64 = variant tag); // a raw `*mut Self` would zero those bits and trip the dispatch @@ -1462,10 +1483,19 @@ impl NewServer { } pub fn active_sockets_count(&self) -> u32 { - self.config - .websocket - .as_ref() - .map_or(0, |ws| ws.handler.active_connections.get() as u32) + self.active_websocket_count.get() + } + + pub(crate) fn note_websocket_opened(&self) { + self.active_websocket_count + .set(self.active_websocket_count.get().saturating_add(1)); + } + + /// Returns true when this close drained the last live websocket. + pub(crate) fn note_websocket_closed(&self) -> bool { + let remaining = self.active_websocket_count.get().saturating_sub(1); + self.active_websocket_count.set(remaining); + remaining == 0 } pub fn has_active_web_sockets(&self) -> bool { @@ -1577,8 +1607,12 @@ impl NewServer { ws.handler.app = None; } self.flags.insert(ServerFlags::TERMINATED); + // Keep `on_websocket_closed` from re-entering `deinit_if_we_can` + // while `app.close()` drains sockets under this `&mut self`. + self.flags.insert(ServerFlags::WEBSOCKETS_DRAINING); // S012: `NewApp` is a ZST opaque — safe `*mut → &mut` deref. bun_opaque::opaque_deref_mut(self.app.unwrap()).close(); + self.flags.remove(ServerFlags::WEBSOCKETS_DRAINING); } } @@ -1664,9 +1698,21 @@ impl NewServer { ); } if self.pending_requests == 0 && !self.has_listener() && !self.has_active_web_sockets() { + // Release the handler refs: a handler closing over the JS `Server` + // forms a cycle (box → Strong → closure env → wrapper → box) the + // GC cannot see through. The counted websocket unprotect runs once + // via the terminal HANDLERS_RELEASED flag. + self.config.on_request = None; + self.config.on_node_http_request = None; + self.config.on_error = None; + self.on_clienterror.deinit(); if let Some(ws) = self.config.websocket.as_mut() { ws.handler.app = None; + if !self.flags.contains(ServerFlags::HANDLERS_RELEASED) { + ws.handler.unprotect(); + } } + self.flags.insert(ServerFlags::HANDLERS_RELEASED); self.unref(); // Detach DevServer. This is needed because there are aggressive @@ -1905,6 +1951,7 @@ impl NewServer { h3_alt_svc: Box::<[u8]>::default(), js_value: jsc::JsRef::empty(), pending_requests: 0, + active_websocket_count: core::cell::Cell::new(0), request_pool: >::request_pool(), // Plain HTTP servers never allocate the ~816 KB H3 pool; defer to // the H3-listen path (`listen()` below) so HTTPS servers that @@ -2047,6 +2094,7 @@ impl NewServer { websocket.global_object = bun_ptr::BackRef::new(bun_opaque::opaque_deref(self.global_this)); websocket.handler.app = Some(std::ptr::from_mut(app).cast::()); + websocket.handler.server = Some(any_server); websocket .handler .flags @@ -3453,6 +3501,23 @@ impl AnyServer { any_server_dispatch!(self, |s| s.inspector_server_id) } + pub(crate) fn on_websocket_opened(&self) { + any_server_dispatch!(self, |s| s.note_websocket_opened()); + } + + /// Decrement the live-socket count; draining the last socket of a + /// stopped server runs the idle pass that was held back by the open + /// sockets. Skipped during the synchronous abrupt-stop drain, which + /// holds `&mut self` (`stop()` follows with its own pass). + pub(crate) fn on_websocket_closed(&self) { + let drained = any_server_dispatch!(self, |s| { + s.note_websocket_closed() && !s.flags.contains(ServerFlags::WEBSOCKETS_DRAINING) + }); + if drained { + any_server_dispatch_mut!(self, |s| s.deinit_if_we_can()); + } + } + pub fn set_inspector_server_id(&mut self, id: jsc::DebuggerId) { any_server_dispatch_mut!(self, |s| { s.inspector_server_id = id; diff --git a/src/runtime/server/server_body.rs b/src/runtime/server/server_body.rs index 6182510d5cd2..334aedb661ae 100644 --- a/src/runtime/server/server_body.rs +++ b/src/runtime/server/server_body.rs @@ -328,6 +328,13 @@ impl ReqLike for uws_sys::h3::Request { } } +/// 503-and-close for requests that arrive after the idle pass released the +/// server's handlers. +pub(crate) fn reject_stopped(resp: &mut impl RespLike) { + resp.write_status(b"503 Service Unavailable"); + resp.end_without_body(true); +} + pub trait RespLike { fn write_status(&mut self, status: &[u8]); fn end_without_body(&mut self, close_connection: bool); @@ -2150,6 +2157,19 @@ where pub fn on_reload_from_zig(&mut self, new_config: &mut ServerConfig, global: &JSGlobalObject) { httplog!("onReload"); + // HANDLERS_RELEASED is terminal: nothing this reload installs could + // ever dispatch, and the idle pass will never unprotect it. Release + // the unadopted websocket protections (no Drop) and skip the route + // swap. Not the idle predicate: a late static/file request on a + // surviving keep-alive connection transiently bumps pending_requests + // after the release. + if self.flags.contains(ServerFlags::HANDLERS_RELEASED) { + if let Some(ws) = new_config.websocket.as_mut() { + ws.unprotect(); + } + return; + } + // SAFETY: `on_reload` is only reachable while the server is running // (`self.app` set in `listen()`). self.app_mut().clear_routes(); @@ -2190,7 +2210,7 @@ where .flags .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() { + if let Some(old_ws) = self.config.websocket.as_mut() { old_ws.unprotect(); } ws.global_object = bun_ptr::BackRef::new(global); @@ -2301,11 +2321,15 @@ where jsc::mark_binding!(); if self.config.on_request.is_none() { + let message: &[u8] = if self.flags.contains(ServerFlags::HANDLERS_RELEASED) { + b"fetch() cannot be used after the server has been stopped" + } else { + b"fetch() requires the server to have a fetch handler" + }; return Ok( JSPromise::dangerously_create_rejected_promise_value_without_notifying_vm( ctx, - ZigString::init(b"fetch() requires the server to have a fetch handler") - .to_error_instance(ctx), + ZigString::init(message).to_error_instance(ctx), ), ); } @@ -2806,6 +2830,12 @@ where let server = unsafe { &mut *server_ptr }; let index = user_route.id; + // Same late-503 path as `on_request`. + if server.flags.contains(ServerFlags::HANDLERS_RELEASED) { + reject_stopped(resp); + return; + } + let should_deinit_context = core::cell::Cell::new(false); let Some(mut prepared) = server.prepare_js_request_context_for::( req, @@ -3189,6 +3219,12 @@ where let server_ptr = server_ref.as_ptr(); let index = this.id; + // Same late-503 path as `on_request`. + if server_ref.flags.contains(ServerFlags::HANDLERS_RELEASED) { + reject_stopped(resp); + 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. diff --git a/test/bake/deinitialization.test.ts b/test/bake/deinitialization.test.ts index 3a6f6772903f..9622fa7973bf 100644 --- a/test/bake/deinitialization.test.ts +++ b/test/bake/deinitialization.test.ts @@ -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); diff --git a/test/bake/fixtures/deinitialization/test.ts b/test/bake/fixtures/deinitialization/test.ts index b6b41533a15c..f83f9822bce6 100644 --- a/test/bake/fixtures/deinitialization/test.ts +++ b/test/bake/fixtures/deinitialization/test.ts @@ -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, beforeAll, expect, test } from "bun:test"; +import { fullGC, heapStats } from "bun:jsc"; expect(process.cwd()).toBe(import.meta.dir); @@ -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; @@ -109,6 +117,48 @@ 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); +} + +async function drainServerWrappers(target: number) { + for (let i = 0; i < 10 && liveServerWrappers() > target; i++) { + Bun.gc(true); + fullGC(); + await new Promise(resolve => setTimeout(resolve, 100)); + } +} + +// `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). Captured +// in beforeAll so the baseline exists even when a name filter skips the +// baseline test. +let serverWrapperBaseline = 0; +beforeAll(async () => { + await (async () => { + const server = Bun.serve({ port: 0, fetch: () => new Response("ok") }); + server.stop(true); + })(); + await drainServerWrappers(1); + serverWrapperBaseline = liveServerWrappers(); +}); + +test("baseline: stopped server wrapper collects", () => { + 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. + await drainServerWrappers(serverWrapperBaseline); + expect(liveServerWrappers()).toBe(serverWrapperBaseline); +}); + for (const { closeActiveConnections, sendAnyRequests, websocket } of cases) { test( "flags: " + diff --git a/test/js/bun/http/bun-server.test.ts b/test/js/bun/http/bun-server.test.ts index 26b8d80e2bd2..3923983cf28b 100644 --- a/test/js/bun/http/bun-server.test.ts +++ b/test/js/bun/http/bun-server.test.ts @@ -613,6 +613,145 @@ test("should be able to await server.stop(true) with keep alive", async () => { expect(async () => await fetch(server.url)).toThrow(); }); +test("fetch() on a stopped server rejects with a stopped-server error", async () => { + using server = Bun.serve({ + port: 0, + fetch() { + return new Response("ok"); + }, + }); + expect(await (await server.fetch("/")).text()).toBe("ok"); + // Idle at stop: the handler release runs inside stop(), so the rejection + // names the stopped state rather than a missing fetch handler. + server.stop(true); + await expect(server.fetch("/")).rejects.toThrow("fetch() cannot be used after the server has been stopped"); +}); + +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("request to a declared route on a surviving keep-alive connection after stop() closes instead of dispatching", async () => { + // Same scenario as above, but through the per-route uws trampoline + // (routes: { "/path": fn }) instead of the catch-all fetch dispatch. + const script = /* js */ ` + const server = Bun.serve({ + port: 0, + hostname: "127.0.0.1", + routes: { "/foo": () => new Response("foo") }, + fetch() { return new Response("catch-all"); }, + }); + + 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\\nfoo")) { + 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 /foo 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 /foo HTTP/1.1\\r\\nHost: localhost\\r\\n\\r\\n"); + const result = await outcome.promise; + if (result !== "closed") throw new Error("late route 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({ diff --git a/test/js/bun/websocket/websocket-server-reload-leak.test.ts b/test/js/bun/websocket/websocket-server-reload-leak.test.ts index c014698d5303..f1b4ee87cc91 100644 --- a/test/js/bun/websocket/websocket-server-reload-leak.test.ts +++ b/test/js/bun/websocket/websocket-server-reload-leak.test.ts @@ -1,5 +1,29 @@ import { expect, test } from "bun:test"; -import { bunEnv, bunExe } from "harness"; +import { bunEnv, bunExe, isWindows } from "harness"; + +// Each test runs its scenario in a child process (gcProtect bookkeeping is +// process-global) and reports measurements as one JSON line on stdout. +async function runProbe(script: string): Promise { + 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(exitCode).toBe(0); + return JSON.parse(stdout.trim()); +} + +// Counts protected AsyncFunction cells so the probe is insensitive to every +// other gcProtected plain Function in the process. +const protectedAsyncFnsPrelude = /* js */ ` + const { heapStats } = require("bun:jsc"); + const protectedAsyncFns = () => heapStats().protectedObjectTypeCounts.AsyncFunction ?? 0; +`; // server.reload({ websocket: { close() {} } }) — i.e. a websocket config // without `open` or `message` — is silently discarded by onReloadFromZig. @@ -46,6 +70,418 @@ test("server.reload() with websocket config lacking open/message does not leak p console.log(JSON.stringify({ before, after, iters: ITERS })); `; + const { before, after, iters } = await runProbe(script); + // With the leak, `after - before` is ~iters * 4 (one per close/drain/ping/pong). + // Without it, the delta should be a small constant independent of `iters`. + expect(after - before).toBeLessThan(iters); +}); + +// A handler defined in a scope that closes over the JS Server value forms a +// native↔JS cycle (server box → protected handler → closure environment → +// Server wrapper → box) that the GC cannot see through. Once the server is +// idle (stopped, no in-flight requests, no live websockets), deinitIfWeCan +// releases the config's handler references, including the gcProtects taken on +// the websocket handlers by WebSocketServerContext.onCreate. +test("stopping an idle server releases its websocket handler protections", async () => { + const script = /* js */ ` + ${protectedAsyncFnsPrelude} + const base = protectedAsyncFns(); + const server = Bun.serve({ + port: 0, + fetch() { return new Response("ok"); }, + websocket: { async open(ws) {}, async message(ws, m) {} }, + }); + const afterServe = protectedAsyncFns(); + // stop(true) with nothing in flight goes idle synchronously and runs the + // handler release in the same call. + server.stop(true); + const afterStop = protectedAsyncFns(); + console.log(JSON.stringify({ base, afterServe, afterStop })); + `; + + const { base, afterServe, afterStop } = await runProbe(script); + expect({ afterServe, afterStop }).toEqual({ afterServe: base + 2, afterStop: base }); +}); + +// reload() on a stopped (idle) server is skipped entirely: the server can +// never dispatch the handlers it would install, so nothing new may end up +// protected (which would reinstate the cycle the idle release breaks). +test("a stopped server's reload does not leave newly installed websocket handler protections behind", async () => { + const script = /* js */ ` + ${protectedAsyncFnsPrelude} + const base = protectedAsyncFns(); + const server = Bun.serve({ + port: 0, + fetch() { return new Response("ok"); }, + websocket: { async open(ws) {}, async message(ws, m) {} }, + }); + server.stop(true); + const afterStop = protectedAsyncFns(); + server.reload({ + fetch() { return new Response("reloaded"); }, + websocket: { async open(ws) {}, async message(ws, m) {} }, + }); + const afterReload = protectedAsyncFns(); + console.log(JSON.stringify({ base, afterStop, afterReload })); + `; + + const { base, afterStop, afterReload } = await runProbe(script); + expect({ afterStop, afterReload }).toEqual({ afterStop: base, afterReload: base }); +}); + +// Static/file routes dispatch without JS and bump pendingRequests with no +// released-handlers guard, so a late request on a surviving keep-alive +// connection makes the released server transiently non-idle. A reload() in +// that window must still be skipped: the release already ran, so nothing +// the reload installs could ever be unprotected again. +// +// Skipped on Windows: the late request there never reaches the router (the +// precondition check below trips with "late static request never became +// pending"), so the pending window this race needs cannot open. The +// short-circuit under test is platform-independent. +test.skipIf(isWindows)( + "reload() during a late static-route response on a released server does not leave handler protections behind", + async () => { + const script = /* js */ ` + ${protectedAsyncFnsPrelude} + const net = require("node:net"); + const base = protectedAsyncFns(); + const server = Bun.serve({ + port: 0, + // Big enough that a paused client forces backpressure, holding the + // request pending while reload() runs. + routes: { "/big": new Response(Buffer.alloc(8 << 20, 65)) }, + fetch() { return new Response("ok"); }, + websocket: { async open(ws) {}, async message(ws, m) {} }, + }); + const afterServe = protectedAsyncFns(); + + const socket = net.connect(server.port, "127.0.0.1"); + await new Promise((resolve, reject) => { + socket.on("connect", resolve); + socket.on("error", reject); + }); + + // Serve one request first: the listener uses deferred accept, so a + // connection that has not sent data yet dies with the listener instead + // of surviving stop(). + const firstDone = Promise.withResolvers(); + let buffered = ""; + socket.on("data", d => { + buffered += d.toString("latin1"); + if (buffered.includes("\\r\\n\\r\\nok")) firstDone.resolve(); + }); + socket.write("GET / HTTP/1.1\\r\\nHost: localhost\\r\\nConnection: keep-alive\\r\\n\\r\\n"); + await firstDone.promise; + + // Graceful: only the listener closes; the established idle connection + // does not keep the server from releasing its handlers. + server.stop(); + const afterStop = protectedAsyncFns(); + + // The static route still serves on the surviving connection. The paused + // client never drains, so the response backpressures and the request + // stays pending. + socket.pause(); + socket.write("GET /big HTTP/1.1\\r\\nHost: localhost\\r\\n\\r\\n"); + for (let i = 0; server.pendingRequests === 0; i++) { + if (i > 400) throw new Error("late static request never became pending"); + await Bun.sleep(5); + } + + server.reload({ + fetch() { return new Response("reloaded"); }, + websocket: { async open(ws) {}, async message(ws, m) {} }, + }); + const afterReload = protectedAsyncFns(); + + // Abort the response; the completion that drops pendingRequests to zero + // runs the idle pass synchronously in the same call. + socket.destroy(); + for (let i = 0; server.pendingRequests > 0; i++) { + if (i > 400) throw new Error("aborted static response never completed"); + await Bun.sleep(5); + } + const final = protectedAsyncFns(); + console.log(JSON.stringify({ base, afterServe, afterStop, afterReload, final })); + `; + + const { base, afterServe, afterStop, afterReload, final } = await runProbe(script); + expect({ afterServe, afterStop, afterReload, final }).toEqual({ + afterServe: base + 2, + afterStop: base, + afterReload: base, + final: base, + }); + }, +); + +// gcProtect is counted per value. When two servers share the same handler +// functions, each onCreate protects them once. The idle release of a stopped +// server drops that server's count; a later reload() of the stopped server +// must not unprotect them again, or it strips the other server's protection +// of the same values. +test("reloading a stopped server does not release another server's shared websocket handler protections", async () => { + const script = /* js */ ` + ${protectedAsyncFnsPrelude} + const base = protectedAsyncFns(); + // Both servers protect the same two async handler function values. + const shared = { + async open(ws) {}, + async message(ws, m) { ws.send("pong:" + m); }, + }; + const keeper = Bun.serve({ + port: 0, + fetch(req, server) { + if (server.upgrade(req)) return; + return new Response("keeper"); + }, + websocket: shared, + }); + const stopped = Bun.serve({ + port: 0, + fetch() { return new Response("stopped"); }, + websocket: shared, + }); + + // Goes idle synchronously; releases the stopped server's handler refs. + stopped.stop(true); + // The reload's new websocket context uses plain functions so it cannot + // affect the AsyncFunction count either way. + stopped.reload({ + fetch() { return new Response("reloaded"); }, + websocket: { open(ws) {}, message(ws, m) {} }, + }); + const afterReload = protectedAsyncFns(); + + // The keeper must still serve websockets with the shared handlers. + const { promise, resolve, reject } = Promise.withResolvers(); + const ws = new WebSocket(keeper.url.href.replace("http", "ws")); + ws.onmessage = e => resolve(e.data); + ws.onerror = () => reject(new Error("keeper websocket errored")); + ws.onclose = e => reject(new Error("keeper websocket closed early: " + e.code)); + ws.onopen = () => ws.send("hi"); + const echoed = await promise; + ws.onclose = null; + ws.close(); + keeper.stop(true); + console.log(JSON.stringify({ base, afterReload, echoed })); + `; + + const { base, afterReload, echoed } = await runProbe(script); + expect({ afterReload, echoed }).toEqual({ afterReload: base + 2, echoed: "pong:hi" }); +}); + +// A graceful stop() closes only the listener; websockets upgraded earlier +// stay connected and dispatch through whichever context a reload() installs, +// so the idle release must wait for them. Once the last one closes, the +// release has to fire right then: no request or finalizer is coming to +// trigger it later. Covers both orderings of stop() and reload(). +test.each([ + ["stop() then reload()", /* js */ `server.stop(); doReload();`], + ["reload() then stop()", /* js */ `doReload(); server.stop();`], +])("handlers stay protected while a websocket survives %s, then release when it closes", async (_order, sequence) => { + const script = /* js */ ` + ${protectedAsyncFnsPrelude} + const base = protectedAsyncFns(); + const server = Bun.serve({ + port: 0, + fetch(req, s) { + if (s.upgrade(req)) return; + return new Response("ok"); + }, + // Plain functions so only the reload's async handlers are counted. + websocket: { open(ws) {}, message(ws, m) {} }, + }); + + const opened = Promise.withResolvers(); + const echoedMessage = Promise.withResolvers(); + const closed = Promise.withResolvers(); + const client = new WebSocket(server.url.href.replace("http", "ws")); + client.onopen = () => opened.resolve(); + client.onmessage = e => echoedMessage.resolve(e.data); + client.onerror = () => { + opened.reject(new Error("client websocket errored")); + echoedMessage.reject(new Error("client websocket errored")); + }; + client.onclose = () => closed.resolve(); + await opened.promise; + + const doReload = () => + server.reload({ + fetch() { return new Response("reloaded"); }, + websocket: { + async open(ws) {}, + async message(ws, m) { ws.send("pong:" + m); }, + }, + }); + // The connected socket keeps the server from going idle throughout. + ${sequence} + + // The socket now dispatches to the reload's handlers; they must still be + // protected, and still work. + const whileConnected = protectedAsyncFns(); + Bun.gc(true); + client.send("hi"); + const echoed = await echoedMessage.promise; + client.close(); + await closed.promise; + + // The server-side close drains the last live socket, which must run the + // deferred idle release. The server event can lag the client's close + // event by a beat, so poll for it. + let afterClose = protectedAsyncFns(); + for (let i = 0; i < 200 && afterClose !== base; i++) { + await Bun.sleep(10); + afterClose = protectedAsyncFns(); + } + console.log(JSON.stringify({ base, whileConnected, echoed, afterClose })); + `; + + const { base, whileConnected, echoed, afterClose } = await runProbe(script); + expect({ whileConnected, echoed, afterClose }).toEqual({ + whileConnected: base + 2, + echoed: "pong:hi", + afterClose: base, + }); +}); + +// Server-side ws.close()/ws.terminate() set the closed flag and then +// synchronously re-enter on_close, which skips its own accounting; the +// methods balance the count themselves. If they did not, the live-socket +// count would stay above zero forever and the idle release could never run. +test.each(["close", "terminate"])("server-side ws.%s() keeps the live-socket count balanced", async method => { + const script = /* js */ ` + ${protectedAsyncFnsPrelude} + const base = protectedAsyncFns(); + const server = Bun.serve({ + port: 0, + fetch(req, s) { + if (s.upgrade(req)) return; + return new Response("ok"); + }, + websocket: { + async open(ws) { ws.KICK_METHOD(); }, + async message(ws, m) {}, + }, + }); + + const closed = Promise.withResolvers(); + const client = new WebSocket(server.url.href.replace("http", "ws")); + client.onclose = () => closed.resolve(); + client.onerror = () => closed.resolve(); + await closed.promise; + + // The kicked socket must have been fully uncounted: this graceful stop + // finds the server idle and releases the handlers. + server.stop(); + let afterStop = protectedAsyncFns(); + for (let i = 0; i < 200 && afterStop !== base; i++) { + await Bun.sleep(10); + afterStop = protectedAsyncFns(); + } + console.log(JSON.stringify({ base, afterStop })); + `.replace("KICK_METHOD", method); + + const { base, afterStop } = await runProbe(script); + expect(afterStop).toBe(base); +}); + +// stop(true) called from inside a websocket close handler: the calling +// socket's own decrement lands only after stop() returns, so the idle pass +// must still fire then (the transient draining flag is already cleared). +test("stop(true) from inside a websocket close handler still releases the handlers", async () => { + const script = /* js */ ` + ${protectedAsyncFnsPrelude} + const base = protectedAsyncFns(); + const server = Bun.serve({ + port: 0, + fetch(req, s) { + if (s.upgrade(req)) return; + return new Response("ok"); + }, + websocket: { + open(ws) {}, + message(ws, m) {}, + // The only async handler, so the probe tracks exactly this closure, + // which also captures \`server\` (the cycle the release breaks). + async close(ws) { + server.stop(true); + }, + }, + }); + + const clientClosed = Promise.withResolvers(); + const client = new WebSocket(server.url.href.replace("http", "ws")); + const opened = Promise.withResolvers(); + client.onopen = () => opened.resolve(); + client.onclose = () => clientClosed.resolve(); + client.onerror = () => { + opened.resolve(); + clientClosed.resolve(); + }; + await opened.promise; + + client.close(); + await clientClosed.promise; + + let afterStop = protectedAsyncFns(); + for (let i = 0; i < 200 && afterStop !== base; i++) { + await Bun.sleep(10); + afterStop = protectedAsyncFns(); + } + console.log(JSON.stringify({ base, afterStop })); + `; + + const { base, afterStop } = await runProbe(script); + expect(afterStop).toBe(base); +}); + +// ws.close() from inside a message handler can drain the last socket of a +// stopped server, running the release while the dispatch is still on the +// stack. The released slots are zeroed, so the dispatch tail (the error +// callback for a throw after close) sees empty values instead of unrooted +// cells. +test("release during an in-flight dispatch does not call released handlers", async () => { + const script = /* js */ ` + const server = Bun.serve({ + port: 0, + fetch(req, s) { + if (s.upgrade(req)) return; + return new Response("ok"); + }, + websocket: { + open(ws) {}, + message(ws, m) { + ws.close(); + throw new Error("boom"); + }, + error(e) { + console.log("error-handler-ran"); + }, + }, + }); + + const opened = Promise.withResolvers(); + const closed = Promise.withResolvers(); + const client = new WebSocket(server.url.href.replace("http", "ws")); + client.onopen = () => opened.resolve(); + client.onclose = () => closed.resolve(); + client.onerror = () => { + opened.resolve(); + closed.resolve(); + }; + await opened.promise; + + // Graceful: the connected socket is the only thing keeping the server + // from the idle release. + server.stop(); + + client.send("hi"); + await closed.promise; + console.log("done"); + `; + await using proc = Bun.spawn({ cmd: [bunExe(), "-e", script], env: bunEnv, @@ -55,10 +491,10 @@ test("server.reload() with websocket config lacking open/message does not leak p const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); - expect(stderr).toBe(""); - const { before, after, iters } = JSON.parse(stdout.trim()); - // With the leak, `after - before` is ~iters * 4 (one per close/drain/ping/pong). - // Without it, the delta should be a small constant independent of `iters`. - expect(after - before).toBeLessThan(iters); - expect(exitCode).toBe(0); + // The throw after close() surfaces through the default uncaught reporter + // (stderr plus a nonzero exit, the same as a server that never had an + // error handler); the released error handler must not run. + expect(stdout).toBe("done\n"); + expect(stderr).toContain("boom"); + expect(exitCode).toBe(1); });