diff --git a/src/jsc/bindings/ZigGlobalObject.cpp b/src/jsc/bindings/ZigGlobalObject.cpp index 567404092abb..a59ef87ba7bc 100644 --- a/src/jsc/bindings/ZigGlobalObject.cpp +++ b/src/jsc/bindings/ZigGlobalObject.cpp @@ -4132,7 +4132,17 @@ extern "C" void Zig__GlobalObject__destructOnExit(Zig::GlobalObject* globalObjec gcUnprotect(globalObject); globalObject = nullptr; vm.heap.collectNow(JSC::Sync, JSC::CollectionScope::Full); - vm.derefSuppressingSaferCPPChecking(); + // The two refs that exist when this runs at event-loop top level are + // Zig__GlobalObject__create's manual ref and the boot-scope JSLockHolder. + // When process.exit() is called from inside a JS callback, every nested + // JSLockHolder still on the native stack (e.g. JSEventListener::handleEvent) + // holds a RefPtr, so a fixed two derefs leave the count > 0 and ~VM + // (and with it Heap::lastChanceToFinalize, which clears all marks and + // sweeps every cell) is skipped. Those holders never destruct because this + // path never returns, so release on their behalf. + for (uint32_t n = vm.refCount(); n > 1; --n) + vm.derefSuppressingSaferCPPChecking(); + // refCount 1 -> 0 runs ~VM; `vm` is dead past this line. vm.derefSuppressingSaferCPPChecking(); runLoop->threadWillExit(); } diff --git a/src/runtime/api/BunObject.rs b/src/runtime/api/BunObject.rs index c592c826915e..7f0af49447df 100644 --- a/src/runtime/api/BunObject.rs +++ b/src/runtime/api/BunObject.rs @@ -1596,6 +1596,19 @@ pub(crate) fn serve(global_object: &JSGlobalObject, callframe: &CallFrame) -> Js break 'brk config; }; + // `init()` below `mem::take`s `config` into a heap-boxed `NewServer`, so + // past that point the raw-`JSValue` handler shadows have no GC root until + // `wrap_handler_slot` writes them into the wrapper's WriteBarrier slots. + // For a data-property options object the user's `{ fetch: fn }` on this + // stack still retains them, but a Proxy- or accessor-backed options + // object returns a fresh fn that nothing else holds. `compute_id`, + // `listen()`'s `set_routes`, and the `ptr_to_js` wrapper allocation can + // all trigger a GC in that window, so gcProtect each handler for its + // duration. `Protected`'s `Drop` unprotects on every exit path (including + // a thrown `listen()` and the hot-reload early return). + let _handler_pins: [bun_jsc::js_value::Protected; 10] = + crate::server::protect_handler_shadows(&config); + // SAFETY: same VM pointer; re-borrow after `args` is dropped. let vm = global_object.bun_vm().as_mut(); @@ -1658,7 +1671,40 @@ pub(crate) fn serve(global_object: &JSGlobalObject, callframe: &CallFrame) -> Js // `server_body` until per-type codegen externs land. <$ServerType>::js_gc_route_list_set(obj, global_object, route_list_object); } + // Mirror the handler callbacks into the wrapper's WriteBarrier + // slots — the wrapper is the sole GC root for these; `ServerConfig` + // / `Handler` only hold raw `JSValue` shadows for hot-path dispatch. + // The async-context wrap is applied here (not in `from_js`) so the + // freshly-allocated wrapper fn is rooted by the slot immediately. + crate::server::wrap_handler_slot( + &mut server_ref.config.on_request, + obj, + global_object, + <$ServerType>::js_gc_on_request_set, + ); + crate::server::wrap_handler_slot( + &mut server_ref.config.on_error, + obj, + global_object, + <$ServerType>::js_gc_on_error_set, + ); + crate::server::wrap_handler_slot( + &mut server_ref.config.on_node_http_request, + obj, + global_object, + <$ServerType>::js_gc_on_node_http_request_set, + ); + // Skip the 7-slot write when there's no websocket config: the + // slots default ZERO so `write_ws_handler_slots`'s clear path + // would be 7 wasted FFI calls. + if server_ref.config.websocket.is_some() { + server_ref.write_ws_handler_slots(obj, global_object); + } server_ref.js_value.set_strong(obj, global_object); + // Slots are rooted; release the scoped gcProtects and run the + // "server just started" GC nudge split out of `listen()`. + drop(_handler_pins); + server_ref.gc_hint_after_listen(); if global_object.bun_vm().test_isolation_enabled { if let Some(handles) = crate::jsc_hooks::isolation_handles() { diff --git a/src/runtime/bake/DevServer.rs b/src/runtime/bake/DevServer.rs index 4e2646dc4e57..d4c2222e7029 100644 --- a/src/runtime/bake/DevServer.rs +++ b/src/runtime/bake/DevServer.rs @@ -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::::deref(html.html_bundle) }; } } @@ -5249,13 +5253,13 @@ fn on_request(dev: &mut DevServer, req: &mut Request, mut resp: AnyResponse) { return; } - if dev + if !dev .server .as_ref() .expect("infallible: server bound") .config() .on_request - .is_some() + .is_empty() { dev.server .as_mut() @@ -5388,8 +5392,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 a16410af9b79..95a3b46f488f 100644 --- a/src/runtime/bake/dev_server/route_bundle.rs +++ b/src/runtime/bake/dev_server/route_bundle.rs @@ -163,8 +163,9 @@ 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() +// 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. diff --git a/src/runtime/server/RequestContext.rs b/src/runtime/server/RequestContext.rs index 43a8b24f750a..23f5fb67e3c1 100644 --- a/src/runtime/server/RequestContext.rs +++ b/src/runtime/server/RequestContext.rs @@ -3517,12 +3517,10 @@ where if let Some(server) = self.server { // SAFETY: BACKREF let server = &*server; - if let Some(on_error) = server.config().on_error.as_ref() - && !self.flags.has_called_error_handler() - { + let on_error = server.config().on_error; + if !on_error.is_empty() && !self.flags.has_called_error_handler() { self.flags.set_has_called_error_handler(true); let result = on_error - .get() .call( server.global_this(), server.js_value().try_get().unwrap_or(JSValue::UNDEFINED), diff --git a/src/runtime/server/ServerConfig.rs b/src/runtime/server/ServerConfig.rs index f9c57edb29c6..4f8ef5c3b1c4 100644 --- a/src/runtime/server/ServerConfig.rs +++ b/src/runtime/server/ServerConfig.rs @@ -45,9 +45,12 @@ pub struct ServerConfig { /// If HMR is not enabled, then this field is ignored. pub enable_chrome_devtools_automatic_workspace_folders: bool, - pub on_error: Option, - pub on_request: Option, - pub on_node_http_request: Option, + /// Raw shadow of the wrapper's `onError`/`onRequest`/`onNodeHTTPRequest` + /// WriteBarrier slots. The wrapper JSCell is the GC root; these are + /// `JSValue::ZERO` when unset and copied for hot-path dispatch reads. + pub on_error: JSValue, + pub on_request: JSValue, + pub on_node_http_request: JSValue, pub websocket: Option, @@ -81,9 +84,9 @@ impl Default for ServerConfig { development: DevelopmentOption::Development, broadcast_console_log_from_browser_to_server_for_bake: false, enable_chrome_devtools_automatic_workspace_folders: true, - on_error: None, - on_request: None, - on_node_http_request: None, + on_error: JSValue::ZERO, + on_request: JSValue::ZERO, + on_node_http_request: JSValue::ZERO, websocket: None, reuse_port: false, id: Box::default(), @@ -274,9 +277,9 @@ impl ServerConfig { .broadcast_console_log_from_browser_to_server_for_bake, enable_chrome_devtools_automatic_workspace_folders: self .enable_chrome_devtools_automatic_workspace_folders, - on_error: self.on_error.take(), - on_request: self.on_request.take(), - on_node_http_request: self.on_node_http_request.take(), + on_error: self.on_error, + on_request: self.on_request, + on_node_http_request: self.on_node_http_request, websocket: self.websocket.take(), reuse_port: self.reuse_port, id: core::mem::take(&mut self.id), @@ -1313,8 +1316,10 @@ impl ServerConfig { global.throw_invalid_arguments(format_args!("Expected error to be a function")) ); } - let on_error_snapshot = on_error.with_async_context_if_needed(global); - args.on_error = Some(Strong::create(on_error_snapshot, global)); + // Raw value — async-context wrapping is deferred to the slot-write + // site (`serve_with!` / `on_reload_from_zig`) so the wrapped fn is + // rooted by the wrapper's WriteBarrier slot the moment it exists. + args.on_error = on_error; } if global.has_exception() { return Err(JsError::Thrown); @@ -1326,8 +1331,7 @@ impl ServerConfig { "Expected onNodeHTTPRequest to be a function", ))); } - let on_request = on_request_.with_async_context_if_needed(global); - args.on_node_http_request = Some(Strong::create(on_request, global)); + args.on_node_http_request = on_request_; } if let Some(on_request_) = arg.get_truthy(global, "fetch")? { @@ -1335,10 +1339,9 @@ impl ServerConfig { return Err(global .throw_invalid_arguments(format_args!("Expected fetch() to be a function"))); } - let on_request = on_request_.with_async_context_if_needed(global); - args.on_request = Some(Strong::create(on_request, global)); + args.on_request = on_request_; } else if args.bake.is_none() - && args.on_node_http_request.is_none() + && args.on_node_http_request.is_empty() && ((args.static_routes.len() + args.user_routes_to_build.len()) == 0 && !opts.has_user_routes) && opts.is_fetch_required diff --git a/src/runtime/server/ServerWebSocket.rs b/src/runtime/server/ServerWebSocket.rs index 39f126212084..5225c12379fb 100644 --- a/src/runtime/server/ServerWebSocket.rs +++ b/src/runtime/server/ServerWebSocket.rs @@ -147,9 +147,9 @@ impl Flags { // (`generate-classes.ts` → `${T}__data{Get,Set}Cached`). #[allow(non_snake_case)] pub mod js { - // Emits `data_{get,set}_cached`. Getter maps `JSValue::ZERO` → `None`; + // Emits `{data,server}_{get,set}_cached`. Getter maps `JSValue::ZERO` → `None`; // setter forwards through the JSC `WriteBarrier` slot. - ::bun_jsc::codegen_cached_accessors!("ServerWebSocket"; data); + ::bun_jsc::codegen_cached_accessors!("ServerWebSocket"; data, server); } /// Maps a uWS `SendStatus` to the JS-visible number contract shared by every @@ -356,6 +356,14 @@ impl ServerWebSocket { .this_value .set(JsRef::init_strong(this_value, global_object)); js::data_set_cached(this_value, global_object, data_value); + // Both callers route through `on_upgrade`'s `handler.server.is_none()` + // refusal, so this is normally `Some`; keep the `and_then` as + // defense-in-depth (option getters between that guard and here can + // re-enter JS and `stop(true)`, and `js_value_for_dispatch` still + // returns `None` on `Finalized`). + if let Some(server_js) = handler.server.and_then(|s| s.js_value_for_dispatch()) { + js::server_set_cached(this_value, global_object, server_js); + } this } @@ -383,10 +391,15 @@ 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); + // Live-socket accounting lives on the server (`Cell`), reached + // through the type-erased backref so the shared `&Handler` suffices. + 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; + let on_error = handler.on_error; if vm.is_shutting_down() { bun_output::scoped_log!(WebSocketServer, "onOpen called after script execution"); ws.close(); @@ -421,17 +434,26 @@ 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 - // close() dispatches on_close, which owns the accounting decrement. + // (`close()` re-enters `on_close`, which skips its own + // accounting because the closed flag is already set.) self.websocket().close(); + closed_here = true; this_value.unprotect(); } - handler.run_error_callback(vm, global_object, err_value); + handler.run_error_callback(on_error, vm, global_object, err_value); + if closed_here { + if let Some(server) = server { + // May run the idle pass; no `&Handler` borrow is live here. + server.on_websocket_closed(); + } + } } } @@ -444,6 +466,7 @@ impl ServerWebSocket { bstr::BStr::new(message) ); let on_message_handler = self.handler().on_message; + let on_error = self.handler().on_error; if on_message_handler.is_empty_or_undefined_or_null() { return; } @@ -490,7 +513,7 @@ impl ServerWebSocket { if let Some(err_value) = result.to_error() { self.handler() - .run_error_callback(vm, global_object, err_value); + .run_error_callback(on_error, vm, global_object, err_value); return; } @@ -523,7 +546,9 @@ impl ServerWebSocket { return; } - if !handler.on_drain.is_empty() { + let on_drain = handler.on_drain; + let on_error = handler.on_error; + if !on_drain.is_empty() { let global_object = handler.global_object(); let args = [self @@ -535,7 +560,7 @@ impl ServerWebSocket { args: &args, global_object, this_value: JSValue::ZERO, - callback: handler.on_drain, + callback: on_drain, result: JSValue::ZERO, }; let _loop_guard = vm.enter_event_loop_scope(); @@ -543,7 +568,7 @@ impl ServerWebSocket { let result = corker.result; if let Some(err_value) = result.to_error() { - handler.run_error_callback(vm, global_object, err_value); + handler.run_error_callback(on_error, vm, global_object, err_value); } } } @@ -563,6 +588,7 @@ impl ServerWebSocket { bun_output::scoped_log!(WebSocketServer, "onPing: {}", bstr::BStr::new(data)); let handler = self.handler(); let cb = handler.on_ping; + let on_error = handler.on_error; let vm = handler.vm(); if cb.is_empty_or_undefined_or_null() || vm.is_shutting_down() { return; @@ -583,7 +609,7 @@ impl ServerWebSocket { if let Err(e) = cb.call(global_this, JSValue::UNDEFINED, &args) { let err = global_this.take_exception(e); bun_output::scoped_log!(WebSocketServer, "onPing error"); - handler.run_error_callback(vm, global_this, err); + handler.run_error_callback(on_error, vm, global_this, err); } } @@ -592,6 +618,7 @@ impl ServerWebSocket { bun_output::scoped_log!(WebSocketServer, "onPong: {}", bstr::BStr::new(data)); let handler = self.handler(); let cb = handler.on_pong; + let on_error = handler.on_error; if cb.is_empty_or_undefined_or_null() { return; } @@ -617,7 +644,7 @@ impl ServerWebSocket { if let Err(e) = cb.call(global_this, JSValue::UNDEFINED, &args) { let err = global_this.take_exception(e); bun_output::scoped_log!(WebSocketServer, "onPong error"); - handler.run_error_callback(vm, global_this, err); + handler.run_error_callback(on_error, vm, global_this, err); } } @@ -628,12 +655,21 @@ impl ServerWebSocket { bun_output::scoped_log!(WebSocketServer, "onClose"); // TODO: Can this called inside finalize? let handler = self.handler(); + // Copy the erased server handle out now: the guard below runs after + // every `handler` borrow has expired, and `on_websocket_closed` may + // form `&mut NewServer` (which owns the handler storage) to run the + // idle pass when this was the last live socket. + let server = handler.server; + let was_closed = self.is_closed(); self.update_flags(|f| f.set_closed(true)); - // uws fires the close callback exactly once per opened socket, so this - // balances on_open's increment even when close()/terminate() already - // set the closed flag (which used to skip it and leak the count). + // Whoever set the closed flag owns the decrement; close()/terminate() + // and on_open's error path each decrement themselves when they flip it. scopeguard::defer! { - handler.on_connection_closed(); + if !was_closed { + if let Some(server) = server { + server.on_websocket_closed(); + } + } } let signal = self.signal.take(); @@ -647,6 +683,7 @@ impl ServerWebSocket { .try_get() .unwrap_or(JSValue::UNDEFINED); let this_value_cell: &JsCell = &self.this_value; + let global_object_ref = handler.global_object; let _cleanup = scopeguard::guard(signal, move |sig| { if let Some(sig) = sig { // `sig` was stored with a +1 ref by the upgrade caller; it @@ -657,6 +694,9 @@ impl ServerWebSocket { sig.unref(); } if was_not_empty { + // Drop the server-wrapper traced edge: once closed, this socket + // no longer needs to pin the server (and its handler slots). + js::server_set_cached(cached_this, global_object_ref.get(), JSValue::ZERO); // R-2: closure-scoped `&mut JsRef` via `JsCell::with_mut` — // no raw `*mut` projection needed. this_value_cell.with_mut(|v| v.downgrade()); @@ -668,7 +708,11 @@ impl ServerWebSocket { return; } - if !handler.on_close.is_empty_or_undefined_or_null() { + // Copy to a stack local before `sig.signal()` re-enters JS: a GC + // between the test and the `.call(...)` could otherwise collect it. + let on_close_handler = handler.on_close; + let on_error = handler.on_error; + if !on_close_handler.is_empty_or_undefined_or_null() { let global_object = handler.global_object(); let _loop_guard = vm.enter_event_loop_scope(); @@ -691,19 +735,16 @@ impl ServerWebSocket { "onClose error (message) {}", was_not_empty ); - handler.run_error_callback(vm, global_object, err); + handler.run_error_callback(on_error, vm, global_object, err); return; } }; let call_args = [cached_this, JSValue::js_number(code as f64), message_js]; - if let Err(e) = handler - .on_close - .call(global_object, JSValue::UNDEFINED, &call_args) - { + if let Err(e) = on_close_handler.call(global_object, JSValue::UNDEFINED, &call_args) { let err = global_object.take_exception(e); bun_output::scoped_log!(WebSocketServer, "onClose error {}", was_not_empty); - handler.run_error_callback(vm, global_object, err); + handler.run_error_callback(on_error, vm, global_object, err); return; } } else if let Some(sig) = signal { @@ -1344,8 +1385,22 @@ impl ServerWebSocket { break 'brk args.ptr[1].to_slice_or_null(global_this)?; }; + // `to_slice_or_null` can run user `toString()`, which may re-entrantly + // `ws.close()` and already decrement the count; re-check the guard. + if self.is_closed() { + return Ok(JSValue::UNDEFINED); + } + + // Copy the server backref BEFORE end(): on_close re-enters and the + // user's close handler may call stop(true), which clears handler.server. + let server = self.handler().server; self.update_flags(|f| f.set_closed(true)); self.websocket().end(code, message_value.slice()); + // on_close re-entered with was_closed=true so it skipped the + // accounting; balance the count here. + if let Some(server) = server { + server.on_websocket_closed(); + } Ok(JSValue::UNDEFINED) } @@ -1363,8 +1418,16 @@ impl ServerWebSocket { return Ok(JSValue::UNDEFINED); } + // Copy the server backref BEFORE close(): on_close re-enters and the + // user's close handler may call stop(true), which clears handler.server. + let server = self.handler().server; self.update_flags(|f| f.set_closed(true)); self.websocket().close(); + // on_close re-entered with was_closed=true so it skipped the + // accounting; balance the count here. + 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 dc8713fc02c9..41d8be5ac37d 100644 --- a/src/runtime/server/WebSocketServerContext.rs +++ b/src/runtime/server/WebSocketServerContext.rs @@ -29,20 +29,20 @@ pub struct Handler { pub on_pong: JSValue, pub app: Option<*mut c_void>, - - /// Set alongside `app` in `set_routes`; lets a closing connection - /// re-evaluate the server's deinit gate (graceful stop holds the - /// server's ref until pending work drains). + /// Type-erased backref to the owning `NewServer`, set alongside `app` + /// in `set_routes` (so it is in place before any socket can upgrade and + /// refreshed whenever a reload installs a new context). + /// `ServerWebSocket::init` reads it to write the server JS wrapper into the + /// per-socket `m_server` traced slot (keeping the wrapper, and the `m_ws*` + /// handler slots it carries, reachable while any socket is connected), and + /// `ServerWebSocket` open/close events route the live-socket accounting + /// through it. pub server: Option, // Always set manually. // 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, /// used by publish() pub flags: HandlerFlags, @@ -73,38 +73,18 @@ 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)); - } - - /// A connection fully closed: decrement and, when it was the last one, - /// re-evaluate the server's deinit gate so a graceful stop's deferred - /// unref/all-closed promise (deinit_if_we_can) can complete. - pub fn on_connection_closed(&self) { - self.active_connections_saturating_sub(1); - if self.active_connections.get() == 0 { - if let Some(mut server) = self.server { - server.deinit_if_we_can(); - } - } - } - + /// `on_error` must be copied to a stack local by the caller before any + /// user JS runs: a re-entrant `ws.close()` on the last socket of a stopped + /// server can downgrade the wrapper (the sole GC root for `wsOnError`) + /// mid-handler, so a fresh `self.on_error` read after user JS could be a + /// freed cell. pub fn run_error_callback( &self, + on_error: JSValue, vm: &VirtualMachine, global_object: &JSGlobalObject, error_value: JSValue, ) { - let on_error = self.on_error; if !on_error.is_empty_or_undefined_or_null() { let _ = on_error .call(global_object, JSValue::UNDEFINED, &[error_value]) @@ -137,7 +117,6 @@ impl Handler { server: None, vm: bun_ptr::BackRef::new(VirtualMachine::get()), global_object: bun_ptr::BackRef::new(global_object), - active_connections: core::cell::Cell::new(0), flags: HandlerFlags::empty(), }; @@ -161,9 +140,10 @@ impl Handler { key ))); } - let cb = value.with_async_context_if_needed(global_object); - *field = cb; - cb.ensure_still_alive(); + // Raw value — async-context wrapping is deferred to + // `NewServer::write_ws_handler_slots` so the wrapped fn is + // rooted by the wrapper's WriteBarrier slot immediately. + *field = value; if i > 0 { // anything other than "error" is considered valid. valid = true; @@ -179,30 +159,6 @@ impl Handler { "WebSocketServerContext expects a message handler" ))) } - - pub fn protect(&self) { - self.on_open.protect(); - self.on_message.protect(); - self.on_close.protect(); - self.on_drain.protect(); - self.on_error.protect(); - self.on_ping.protect(); - self.on_pong.protect(); - } - - pub fn unprotect(&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(); - } } impl WebSocketServerContext { @@ -219,14 +175,6 @@ impl WebSocketServerContext { ..Default::default() } } - - pub fn protect(&self) { - self.handler.protect(); - } - - pub fn unprotect(&self) { - self.handler.unprotect(); - } } bun_core::comptime_string_map! { @@ -446,6 +394,5 @@ pub(crate) fn on_create( } } - server.protect(); Ok(server) } diff --git a/src/runtime/server/mod.rs b/src/runtime/server/mod.rs index 9e3f744692bb..abd2d3816635 100644 --- a/src/runtime/server/mod.rs +++ b/src/runtime/server/mod.rs @@ -262,11 +262,27 @@ pub struct NewServer { pub base_url_string_for_joining: Box<[u8]>, pub config: ServerConfig, pub pending_requests: usize, + /// Live `ServerWebSocket` count. Lives on the server (not the websocket + /// context) so a reload's context swap cannot reset it, and sits in a + /// `Cell` because the open/close accounting arrives through shared + /// `AnyServer` handles on the JS thread. + pub active_websocket_count: core::cell::Cell, + /// Set across [`NewServer::deinit_if_we_can`] and the synchronous + /// `app.close()` drain in `stop_listening`; lets a nested call (reached + /// via a callback the body fires) early-return instead of re-running the + /// downgrade/teardown while the outer frame still holds `&mut self`. + deinit_running: 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 /// conditional field so the struct stays uniform across monomorphizations. pub h3_request_pool: *mut request_context::RequestContextStackAllocator, + /// Authoritative GC root for the `server.stop()` promise. Lazily filled by + /// `get_all_closed_promise`; read in `deinit_if_we_can` (which can run + /// after the wrapper is collected, so a wrapper-traced slot would not + /// suffice — this Strong is what keeps the cell live across that window). + /// The cycle through a user `.then(cb)` is broken by resolving the + /// promise in `deinit_if_we_can`, after which the Strong is dropped. pub all_closed_promise: jsc::JSPromiseStrong, pub listen_callback: jsc::AnyTask::AnyTask, @@ -288,12 +304,13 @@ pub struct NewServer { /// times due to SNI, so we have to store them. pub user_routes: Vec>, - pub on_clienterror: jsc::StrongOptional, + /// Raw shadow of the wrapper's `m_onClientError` WriteBarrier slot. + /// `JSValue::ZERO` when unset; written by `server_set_on_client_error_`. + pub on_clienterror: JSValue, - /// node:http compat: JS callback invoked with the JSNodeHTTPServerSocket - /// when a connection is accepted (for TLS, when its handshake completes), - /// before any request bytes. Backs `server.emit("connection", ...)`. - pub on_connection: jsc::StrongOptional, + /// Raw shadow of the wrapper's `m_onConnection` WriteBarrier slot. + /// `JSValue::ZERO` when unset; written by `server_set_on_connection_`. + pub on_connection: JSValue, pub inspector_server_id: jsc::DebuggerId, } @@ -307,8 +324,7 @@ pub struct UserRoute { impl Drop for NewServer { fn drop(&mut self) { // The remaining owned fields (config, base_url, h3_alt_svc, dev_server, - // user_routes, all_closed_promise, on_clienterror, on_connection) drop - // automatically. + // user_routes, all_closed_promise) drop automatically. if let Some(p) = self.plugins.take() { // SAFETY: `plugins` carries the `heap::alloc` provenance from // `ServePlugins::init`; this releases the server's counted ref. @@ -568,6 +584,68 @@ impl NewServer { self.js_value.try_get().expect("js_value alive") } + /// Returns the wrapper while it is alive (`Strong` or `Weak`), or `None` + /// once `finalize()` has set `Finalized`. `Weak` means the wrapper cell is + /// still live (its WriteBarrier slots still root the handlers); only + /// `Finalized` means the slots are gone and the `config` shadows may point + /// at freed cells. Dispatch trampolines answer 503+close on `None`. + pub fn js_value_for_dispatch(&self) -> Option { + self.js_value.try_get() + } +} + +/// gcProtect every handler callback `ServerConfig::from_js` / `Handler::from_js` +/// stored as a raw-`JSValue` shadow, for the window between those being moved +/// into an unscanned heap box (`init()`'s `mem::take` on serve, +/// `self.config.websocket = Some(ws)` on reload) and the `wrap_handler_slot` +/// writes. Returns an RAII array whose `Drop` unprotects on every exit path. +/// `JSValue::ZERO` slots are cheap no-ops on both sides (the C++ +/// `gcProtect`/`gcUnprotect` early-return on non-cells), so there is no need +/// to branch on `is_empty()`. +pub(crate) fn protect_handler_shadows(config: &ServerConfig) -> [bun_jsc::js_value::Protected; 10] { + let ws = config.websocket.as_ref().map(|w| &w.handler); + let z = JSValue::ZERO; + [ + config.on_request, + config.on_error, + config.on_node_http_request, + ws.map_or(z, |h| h.on_open), + ws.map_or(z, |h| h.on_message), + ws.map_or(z, |h| h.on_close), + ws.map_or(z, |h| h.on_drain), + ws.map_or(z, |h| h.on_error), + ws.map_or(z, |h| h.on_ping), + ws.map_or(z, |h| h.on_pong), + ] + .map(JSValue::protected) +} + +/// Single point of truth for "wrap a handler callback and mirror it into the +/// wrapper's WriteBarrier slot". If `*shadow` is unset (empty/undefined/null), +/// normalize it to `ZERO` and clear the slot; otherwise apply the +/// async-context wrap and write the wrapped fn into both the slot and +/// `*shadow`. Every call site already holds a live wrapper (`ptr_to_js` on +/// serve, `callframe.this()` on reload / setOnClientError), so `server_js` +/// is always valid. Keeping the is-empty check, the wrap step, and the +/// shadow↔slot pairing in one helper is what stops the serve / reload / ws / +/// clientError sites from drifting. +#[inline] +pub(crate) fn wrap_handler_slot( + shadow: &mut JSValue, + server_js: JSValue, + global: &JSGlobalObject, + set: fn(JSValue, &JSGlobalObject, JSValue), +) { + let v = if shadow.is_empty_or_undefined_or_null() { + JSValue::ZERO + } else { + shadow.with_async_context_if_needed(global) + }; + set(server_js, global, v); + *shadow = v; +} + +impl NewServer { /// Per-monomorphization static. /// Rust statics cannot be const-generic; routed through a /// `&'static AtomicBool` so the four (SSL,DEBUG) instantiations share one @@ -838,6 +916,16 @@ impl NewServer { callback: JSValue, extra_args: [JSValue; ARG_COUNT], ) { + // Same Finalized-only gate as the network trampolines. Unreachable + // today — the saved request's `pending_requests` increment keeps the + // wrapper `Strong`, so `finalize()` cannot run — but explicit so a + // future accounting bug 503s instead of dispatching with a stale + // handler shadow. + // SAFETY: `this` is the live server backref for this request. + let Some(server_js) = unsafe { &*this }.js_value_for_dispatch() else { + server_body::respond_stopped_503(bun_opaque::opaque_deref_mut(resp)); + return; + }; let prepared: PreparedRequest = match &req { SavedRequestUnion::Stack(r) => { // reshaped for borrowck — decouple the inner @@ -884,7 +972,7 @@ impl NewServer { // SAFETY: `this` is the live server backref for this request. let server = unsafe { &*this }; let global = server.global_this(); - let response_value = match callback.call(global, server.js_value_assert_alive(), &args) { + let response_value = match callback.call(global, server_js, &args) { Ok(v) => v, Err(err) => global.take_exception(err), }; @@ -1018,6 +1106,14 @@ impl NewServer { req: &mut uws_sys::Request, resp: *mut uws_sys::NewAppResponse, ) { + // Idle keep-alive sockets aren't counted in pending_requests, so the + // wrapper can have been finalized before this fires. Refuse and close + // rather than dispatching with a stale handler shadow. + // SAFETY: `this` is the live server backref for this request. + let Some(js_value) = unsafe { &*this }.js_value_for_dispatch() else { + server_body::respond_stopped_503(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, @@ -1032,16 +1128,10 @@ 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); + let on_request = server.config.on_request; debug_assert!(!on_request.is_empty()); let global = server.global_this(); - let js_value = server.js_value_assert_alive(); let response_value = match on_request.call(global, js_value, &[prepared.js_request, js_value]) { Ok(v) => v, @@ -1069,6 +1159,12 @@ impl NewServer { let server = user_route.server.cast_mut(); let index = user_route.id; + // SAFETY: `server` is the live backref stored in `user_route`. + let Some(server_js) = unsafe { &*server }.js_value_for_dispatch() else { + server_body::respond_stopped_503(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, @@ -1087,7 +1183,6 @@ impl NewServer { // SAFETY: `server` is the live backref stored in `user_route`. let server_ref = unsafe { &*server }; let global = server_ref.global_this(); - let server_js = server_ref.js_value_assert_alive(); let server_request_list = Self::js_route_list_get_cached(server_js).expect("routeList cached value missing"); let response_value = bun_jsc::host_fn::from_js_host_call(global, || { @@ -1154,6 +1249,11 @@ impl NewServer { // SAFETY: `this` is the live server backref registered as the uws // userdata; only one borrow derived from it is alive at a time. + if unsafe { &*this }.js_value_for_dispatch().is_none() { + server_body::respond_stopped_503(resp); + return; + } + // SAFETY: same `this` as above. unsafe { (*this).on_pending_request() }; // Read-only access goes through `BackRef` (safe `Deref`); each use // materialises a fresh short-lived `&Self`, so the JS-reentrant calls @@ -1191,12 +1291,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; // 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 @@ -1400,34 +1495,12 @@ impl NewServer { } } - /// `js.routeListGetCached` — read back the codegen'd `WriteBarrier` slot. - fn js_route_list_get_cached(server_js: JSValue) -> Option { - match (SSL, DEBUG) { - (false, false) => route_list_cached::http::route_list_get_cached(server_js), - (true, false) => route_list_cached::https::route_list_get_cached(server_js), - (false, true) => route_list_cached::debug_http::route_list_get_cached(server_js), - (true, true) => route_list_cached::debug_https::route_list_get_cached(server_js), - } + pub(crate) fn js_route_list_get_cached(server_js: JSValue) -> Option { + server_js_cached!(SSL, DEBUG, route_list_get_cached(server_js)) } - /// `js.gc.routeList.set` — write the codegen'd `WriteBarrier` - /// slot on the per-type C++ wrapper so route JS objects stay GC-rooted. - pub fn js_gc_route_list_set(server_js: JSValue, global: &JSGlobalObject, route_list: JSValue) { - match (SSL, DEBUG) { - (false, false) => { - route_list_cached::http::route_list_set_cached(server_js, global, route_list) - } - (true, false) => { - route_list_cached::https::route_list_set_cached(server_js, global, route_list) - } - (false, true) => { - route_list_cached::debug_http::route_list_set_cached(server_js, global, route_list) - } - (true, true) => { - route_list_cached::debug_https::route_list_set_cached(server_js, global, route_list) - } - } - } + // `js_gc_route_list_set` and the per-callback slot setters live with the + // `slot_setter!` invocations below (alongside `cached_values`). /// Wrap an already-heap-allocated server pointer in its JS object. /// Ownership transfers to the C++ wrapper (freed via `finalize`). @@ -1454,10 +1527,25 @@ 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 prev = self.active_websocket_count.get(); + // Guard underflow: a `close` reaching us without a matching `open` + // (double-close on the JS side) must not wrap and return `true`. + if prev == 0 { + return false; + } + let remaining = prev - 1; + self.active_websocket_count.set(remaining); + remaining == 0 } pub fn has_active_web_sockets(&self) -> bool { @@ -1584,15 +1672,27 @@ impl NewServer { ws.handler.app = None; } self.flags.insert(ServerFlags::TERMINATED); + // `app.close()` synchronously drains every open websocket; their + // `on_close` defers call `on_websocket_closed`, which would + // dispatch `deinit_if_we_can` through a fresh `&mut NewServer` + // while this frame still holds `&mut self`. Hold the re-entrance + // guard across the drain so that nested call early-returns — + // `stop()` runs `deinit_if_we_can` itself right after this returns. + self.deinit_running.set(true); // S012: `NewApp` is a ZST opaque — safe `*mut → &mut` deref. bun_opaque::opaque_deref_mut(self.app.unwrap()).close(); + self.deinit_running.set(false); + // Only clear after the drain — `on_close` defers reach + // `on_websocket_closed` through `handler.server`, so wiping it + // earlier would strand the live-socket count and the idle pass + // would never see it drained. + if let Some(ws) = self.config.websocket.as_mut() { + ws.handler.server = None; + } } } pub fn stop(&mut self, abrupt: bool) { - if self.js_value.is_not_empty() { - self.js_value.downgrade(); - } if self.config.allow_hot && !self.config.id.is_empty() { // `hot_map()` is reached via the thread-local VM singleton (raw ptr // deref) and does not borrow `self`, so it cannot overlap with the @@ -1611,6 +1711,27 @@ impl NewServer { #[inline] pub fn deinit_if_we_can(&mut self) { + // Re-entrance guard. The websocket-close trigger reaches this through + // an `AnyServer` raw-ptr dispatch and can land here while an outer + // `&mut self` frame is already on the stack (abrupt `app.close()` + // drains sockets synchronously; their close defers call back in). The + // body is idempotent, so the outer call will finish the work — skip + // the nested run rather than mutate under the aliased borrow. + // This replaces the old `!TERMINATED` proxy in `on_websocket_closed`, + // which was permanent and so also blocked the *post*-stop close defer + // that should fire the downgrade. + if self.deinit_running.get() { + return; + } + self.deinit_running.set(true); + // Cleared inline at the tail (no early returns below). Not a + // scopeguard: the body forms `&mut self` reborrows for + // `unref()`/`schedule_deinit()`, and a guard holding either `&Cell` or + // a raw `*mut` into `*self` across those would either fail + // borrow-check or have its provenance tag popped under Stacked + // Borrows. A panic mid-body leaves the flag set — acceptable, the idle + // pass panicking means this server is unrecoverable anyway. + httplog!( "deinitIfWeCan. requests={}, listener={}, websockets={}, has_handled_all_closed_promise={}, all_closed_promise={}, has_js_deinited={}", self.pending_requests, @@ -1671,8 +1792,14 @@ impl NewServer { ); } if self.pending_requests == 0 && !self.has_listener() && !self.has_active_web_sockets() { + // Make the wrapper collectible. Dispatch still works while it is + // `Weak` (its WriteBarrier slots still root the handlers); the + // `js_value_for_dispatch` gate only trips once the wrapper is + // actually finalized. + self.js_value.downgrade(); if let Some(ws) = self.config.websocket.as_mut() { ws.handler.app = None; + ws.handler.server = None; } self.unref(); @@ -1692,6 +1819,7 @@ impl NewServer { self.schedule_deinit(); } } + self.deinit_running.set(false); } pub fn schedule_deinit(&mut self) { @@ -1873,9 +2001,9 @@ impl NewServer { } } - // owned-field cleanup (all_closed_promise / user_routes / - // config / on_clienterror / h3_alt_svc / dev_server / plugins) is - // handled by the heap::take drop below — see `impl Drop for NewServer`. + // owned-field cleanup (all_closed_promise / user_routes / config / + // h3_alt_svc / dev_server / plugins) is handled by the heap::take drop + // below — see `impl Drop for NewServer`. if Self::HAS_H3 { if let Some(h3a) = this_ref.h3_app.take() { // SAFETY: live H3::App handle owned by this server. @@ -1926,6 +2054,8 @@ impl NewServer { h3_alt_svc: Box::<[u8]>::default(), js_value: jsc::JsRef::empty(), pending_requests: 0, + active_websocket_count: core::cell::Cell::new(0), + deinit_running: core::cell::Cell::new(false), 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 @@ -1940,8 +2070,8 @@ impl NewServer { flags: ServerFlags::default(), plugins: None, user_routes: Vec::new(), - on_clienterror: jsc::StrongOptional::empty(), - on_connection: jsc::StrongOptional::empty(), + on_clienterror: JSValue::ZERO, + on_connection: JSValue::ZERO, inspector_server_id: jsc::DebuggerId::init(0), })); @@ -2386,8 +2516,8 @@ impl NewServer { // --- 9. Consolidated "/*" HTTP fallback registration --- let ud = self_ptr.cast::(); - let has_node_http = self.config.on_node_http_request.is_some(); - let has_on_request = self.config.on_request.is_some(); + let has_node_http = !self.config.on_node_http_request.is_empty(); + let has_on_request = !self.config.on_request.is_empty(); if star_methods_covered_by_user == http_method::Set::all() { // User/Static/Dev has already provided a "/*" handler for ALL methods. // No further global "/*" HTTP fallback needed. @@ -2709,7 +2839,7 @@ impl NewServer { route_list_value = unsafe { &mut *this }.set_routes(); } - if this_ref.config.on_node_http_request.is_some() { + if !this_ref.config.on_node_http_request.is_empty() { // SAFETY: `this` is the live boxed server from `init()`; no other borrow is live. unsafe { &mut *this }.set_using_custom_expect_handler(true); } @@ -2884,17 +3014,28 @@ impl NewServer { // SAFETY: `this` is the live boxed server from `init()`; no other borrow is live. unsafe { &mut *this }.ref_(); - // Starting up an HTTP server is a good time to GC. - let vm = this_ref.vm(); + // NOTE: the "starting an HTTP server is a good time to GC" nudge runs + // from the caller (`serve_with!` in BunObject.rs) after the handler + // callbacks are rooted in the wrapper's WriteBarrier slots; see + // `gc_hint_after_listen` below. + + route_list_value + } + + /// The server-just-started GC nudge, split out of `listen()` so + /// `serve_with!` can run it after the wrapper's handler slots are + /// populated. Between `init()` (which `mem::take`s `config` into the + /// heap-boxed `NewServer`) and the slot writes, a Proxy- or + /// accessor-backed options object's fresh handler fn is held only by the + /// unscanned heap box, so collecting there would free a fn we then write + /// into the slot and dispatch into. + pub(crate) fn gc_hint_after_listen(&self) { + let vm = self.vm(); if vm.aggressive_garbage_collection == jsc::virtual_machine::GCLevel::Aggressive { vm.auto_garbage_collect(); } else { - // SAFETY: event_loop() returns the VM's owned `*mut EventLoop`; - // non-null while the VM is alive. vm.event_loop_ref().perform_gc(); } - - route_list_value } } @@ -2903,21 +3044,113 @@ impl NewServer { // them here instead of redeclaring with a divergent `req` pointer type. use server_body::{Bun__ServerRouteList__callRoute, Bun__ServerRouteList__create}; -/// Per-type cached-accessor shims for the `routeList` `WriteBarrier` slot. -/// `codegen_cached_accessors!` emits `route_list_{get,set}_cached` wrapping -/// `${T}Prototype__routeList{Get,Set}CachedValue` (generate-classes.ts). -mod route_list_cached { +/// Per-type cached-accessor shims for the server `WriteBarrier` value slots. +/// `codegen_cached_accessors!` emits `${snake}_{get,set}_cached` wrapping +/// `${T}Prototype__${prop}{Get,Set}CachedValue` (generate-classes.ts). +mod cached_values { + macro_rules! per_type { + ($ty:literal) => { + bun_jsc::codegen_cached_accessors!( + $ty; routeList, onRequest, onError, onNodeHTTPRequest, onClientError, onConnection, + wsOnOpen, wsOnMessage, wsOnClose, wsOnDrain, wsOnError, wsOnPing, wsOnPong + ); + }; + } pub(super) mod http { - bun_jsc::codegen_cached_accessors!("HTTPServer"; routeList); + per_type!("HTTPServer"); } pub(super) mod https { - bun_jsc::codegen_cached_accessors!("HTTPSServer"; routeList); + per_type!("HTTPSServer"); } pub(super) mod debug_http { - bun_jsc::codegen_cached_accessors!("DebugHTTPServer"; routeList); + per_type!("DebugHTTPServer"); } pub(super) mod debug_https { - bun_jsc::codegen_cached_accessors!("DebugHTTPSServer"; routeList); + per_type!("DebugHTTPSServer"); + } +} + +/// `(SSL, DEBUG)` → per-type `cached_values` submodule dispatch for the +/// codegen'd `${snake}_{get,set}_cached` accessors. +macro_rules! server_js_cached { + ($ssl:expr, $debug:expr, $fn:ident($($arg:expr),* $(,)?)) => { + match ($ssl, $debug) { + (false, false) => $crate::server::cached_values::http::$fn($($arg),*), + (true, false) => $crate::server::cached_values::https::$fn($($arg),*), + (false, true) => $crate::server::cached_values::debug_http::$fn($($arg),*), + (true, true) => $crate::server::cached_values::debug_https::$fn($($arg),*), + } + }; +} +pub(crate) use server_js_cached; + +// Dispatch reads from the shadow JSValue fields, not the wrapper slots, so +// only the slot setter is generated here. The slot is the GC-traced root; the +// shadow is the hot-path read. +macro_rules! slot_setter { + ($set_fn:ident, $set_cached:ident) => { + pub fn $set_fn(server_js: JSValue, global: &JSGlobalObject, v: JSValue) { + server_js_cached!(SSL, DEBUG, $set_cached(server_js, global, v)) + } + }; +} + +impl NewServer { + slot_setter!(js_gc_route_list_set, route_list_set_cached); + slot_setter!(js_gc_on_request_set, on_request_set_cached); + slot_setter!(js_gc_on_error_set, on_error_set_cached); + slot_setter!( + js_gc_on_node_http_request_set, + on_node_h_t_t_p_request_set_cached + ); + slot_setter!(js_gc_on_client_error_set, on_client_error_set_cached); + slot_setter!(js_gc_on_connection_set, on_connection_set_cached); + slot_setter!(js_gc_ws_on_open_set, ws_on_open_set_cached); + slot_setter!(js_gc_ws_on_message_set, ws_on_message_set_cached); + slot_setter!(js_gc_ws_on_close_set, ws_on_close_set_cached); + slot_setter!(js_gc_ws_on_drain_set, ws_on_drain_set_cached); + slot_setter!(js_gc_ws_on_error_set, ws_on_error_set_cached); + slot_setter!(js_gc_ws_on_ping_set, ws_on_ping_set_cached); + slot_setter!(js_gc_ws_on_pong_set, ws_on_pong_set_cached); + + /// Mirror all 7 `Handler.on_*` shadows into the wrapper's `m_wsOn*` + /// WriteBarrier slots, applying the async-context wrap (deferred from + /// `Handler::from_js` so the wrapped fn is rooted the moment it exists). + /// Writes all slots unconditionally — `JSValue::ZERO` clears, so a reload + /// that omits a callback drops the previous root. Same contract as + /// [`wrap_handler_slot`]; called after `ptr_to_js` in `serve()` and after + /// the websocket-context swap in `on_reload_from_zig`; dispatch keeps + /// reading the shadow. + pub fn write_ws_handler_slots(&mut self, server_js: JSValue, global: &JSGlobalObject) { + // No websocket config: route a throwaway ZERO through each slot so a + // transition to "no websocket" drops the previous roots. Redundant on + // initial serve (slots default ZERO) — `serve()` gates this call on + // `websocket.is_some()` — but the doc'd contract is "writes all slots + // unconditionally" so a future call site can't leave stale roots behind. + let mut zeros = [JSValue::ZERO; 7]; + let [open, message, close, drain, error, ping, pong] = match self.config.websocket.as_mut() + { + Some(ws) => { + let h = &mut ws.handler; + [ + &mut h.on_open, + &mut h.on_message, + &mut h.on_close, + &mut h.on_drain, + &mut h.on_error, + &mut h.on_ping, + &mut h.on_pong, + ] + } + None => zeros.each_mut(), + }; + wrap_handler_slot(open, server_js, global, Self::js_gc_ws_on_open_set); + wrap_handler_slot(message, server_js, global, Self::js_gc_ws_on_message_set); + wrap_handler_slot(close, server_js, global, Self::js_gc_ws_on_close_set); + wrap_handler_slot(drain, server_js, global, Self::js_gc_ws_on_drain_set); + wrap_handler_slot(error, server_js, global, Self::js_gc_ws_on_error_set); + wrap_handler_slot(ping, server_js, global, Self::js_gc_ws_on_ping_set); + wrap_handler_slot(pong, server_js, global, Self::js_gc_ws_on_pong_set); } } @@ -3484,6 +3717,12 @@ impl AnyServer { any_server_dispatch!(self, |s| &s.config) } + /// Same gate as [`NewServer::js_value_for_dispatch`]. + #[inline] + pub fn js_value_for_dispatch(&self) -> Option { + any_server_dispatch!(self, |s| s.js_value_for_dispatch()) + } + pub fn h3_alt_svc(&self) -> Option<&[u8]> { match self.tag { AnyServerTag::HTTPSServer => self.as_https().h3_alt_svc(), @@ -3496,6 +3735,29 @@ 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 and, when the last socket drained on + /// an already-stopped server, run the idle pass so the `JsRef` downgrade + /// (and deferred deinit) that was held back by the open sockets fires. + /// + /// Skipped while still listening (the idle pass would no-op). Re-entrance + /// during the abrupt-stop synchronous drain is handled by the + /// `deinit_running` guard inside `deinit_if_we_can` itself — gating on + /// `TERMINATED` here also blocked the post-`stop(true)` close defer that + /// must fire the downgrade when `stop` was called from inside a close + /// handler (the socket whose handler ran decrements only after `stop` + /// returns, so `stop`'s own idle pass still sees it live). + pub(crate) fn on_websocket_closed(&self) { + let drained = + any_server_dispatch!(self, |s| s.note_websocket_closed() && !s.has_listener()); + 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.classes.ts b/src/runtime/server/server.classes.ts index 3e2af3471080..87f319a1bd0f 100644 --- a/src/runtime/server/server.classes.ts +++ b/src/runtime/server/server.classes.ts @@ -90,7 +90,21 @@ function generate(name) { finalize: true, construct: true, noConstructor: true, - values: ["routeList"], + values: [ + "routeList", + "onRequest", + "onError", + "onNodeHTTPRequest", + "onClientError", + "onConnection", + "wsOnOpen", + "wsOnMessage", + "wsOnClose", + "wsOnDrain", + "wsOnError", + "wsOnPing", + "wsOnPong", + ], }); } export default [ @@ -351,7 +365,7 @@ export default [ finalize: true, construct: true, klass: {}, - values: ["socket"], + values: ["server"], }), define({ diff --git a/src/runtime/server/server_body.rs b/src/runtime/server/server_body.rs index c058a048d2f1..d5a4f2def6f3 100644 --- a/src/runtime/server/server_body.rs +++ b/src/runtime/server/server_body.rs @@ -26,7 +26,7 @@ use bun_jsc::ZigStringJsc as _; use bun_jsc::uuid::UUID; use bun_jsc::{ self as jsc, ArrayBuffer, CallFrame, GlobalRef, JSGlobalObject, JSPromise, JSValue, JsError, - JsResult, Node, StringJsc as _, Strong, StrongOptional, VirtualMachine, host_fn, + JsResult, Node, StringJsc as _, StrongOptional, VirtualMachine, host_fn, }; use bun_paths as paths; use bun_ptr::RefPtr; @@ -330,6 +330,7 @@ impl ReqLike for uws_sys::h3::Request { } pub trait RespLike { + const IS_H3: bool; fn write_status(&mut self, status: &[u8]); fn end_without_body(&mut self, close_connection: bool); fn timeout(&mut self, seconds: u8); @@ -337,6 +338,7 @@ pub trait RespLike { fn to_any_response(&mut self) -> uws::AnyResponse; } impl RespLike for uws_sys::NewAppResponse { + const IS_H3: bool = false; #[inline] fn write_status(&mut self, s: &[u8]) { uws_sys::NewAppResponse::::write_status(self, s) @@ -376,6 +378,7 @@ impl RespLike for uws_sys::NewAppResponse { } } impl RespLike for uws_sys::h3::Response { + const IS_H3: bool = true; #[inline] fn write_status(&mut self, s: &[u8]) { uws_sys::h3::Response::write_status(self, s) @@ -402,6 +405,21 @@ impl RespLike for uws_sys::h3::Response { } } +/// Answer a request that arrived after `finalize()` set the wrapper's +/// `JsRef` to `Finalized` (idle keep-alive sockets aren't counted in +/// `pending_requests`, so `self` can outlive the wrapper between the +/// finalizer and the next-tick `schedule_deinit`). 503 instead of +/// dispatching into a dead handler shadow. One helper so every dispatch +/// trampoline gets the same guard. H1 closes the connection; H3 ends only +/// this stream (`!R::IS_H3`) so sibling streams on the same QUIC connection +/// survive — same per-protocol close treatment as the other reject fast +/// paths. +#[inline] +pub(super) fn respond_stopped_503(resp: &mut R) { + resp.write_status(b"503 Service Unavailable"); + resp.end_without_body(!R::IS_H3); +} + pub(super) type ServerRequestContext = NewRequestContext, SSL, DEBUG, false>; pub(super) type ServerH3RequestContext = @@ -1745,6 +1763,25 @@ where return Ok(JSValue::FALSE); } + // After a graceful stop() has drained to idle, `deinit_if_we_can` + // downgrades the wrapper and clears `handler.server` / `handler.app`. + // `js_value_for_dispatch` still lets a late keep-alive request reach + // `fetch()` while the wrapper is `Weak`, but accepting a new websocket + // there would create a `ServerWebSocket` whose `init`/`on_open` skip + // the `m_server` trace edge and `on_websocket_opened()` (because + // `handler.server` is `None`), so `has_active_web_sockets()` would + // stay false and the next idle pass could free the `NewServer` box + // under a live socket. Refuse the upgrade once idle; the caller sees + // `false` and can fall through to a regular response. + if self + .config + .websocket + .as_ref() + .is_some_and(|ws| ws.handler.server.is_none()) + { + return Ok(JSValue::FALSE); + } + if let Some(node_http_response) = ::from_js(object) { // SAFETY: from_js returns a live *mut NodeHTTPResponse let node_http_response = unsafe { &mut *node_http_response }; @@ -2148,8 +2185,9 @@ where /// configuration (handlers, websocket, routes) with `new_config` and /// re-registers routes on the uws app(s). Ownership of moved-in fields /// transfers to `self.config`; the caller's `new_config` is left in a - /// valid-but-emptied state (`ServerConfig`'s `Drop` then frees whatever - /// was *not* taken — e.g. a websocket block we declined to adopt). + /// valid-but-emptied state and its `Drop` frees whatever was *not* taken. + /// Any `Some(ws)` is adopted unconditionally — `Handler::from_js` already + /// rejected configs with no non-error callback. pub fn on_reload_from_zig(&mut self, new_config: &mut ServerConfig, global: &JSGlobalObject) { httplog!("onReload"); @@ -2162,56 +2200,66 @@ where } } - // Only reload `on_request` / `on_error` when the new config actually - // specifies one. `Option` drops the old handle (= JSValue.unprotect()). - if new_config - .on_request - .as_ref() - .is_some_and(|s| !s.get().is_undefined()) - { - self.config.on_request = new_config.on_request.take(); + // `on_request` / `on_error` keep their previous value when the reload + // config omits them. The async-context re-wrap is unconditional: + // `with_async_context_if_needed` is a no-op when no ALS frame is + // active, so re-wrapping on every reload keeps the captured frame in + // sync with the call-time context. `on_reload` is a host_fn — the + // wrapper is `callframe.this()` on the JS stack, alive even if + // `js_value` was downgraded after stop(). The slot writes must reach + // it so the new handlers are GC-rooted. + let server_js = self.js_value_assert_alive(); + if !new_config.on_request.is_empty_or_undefined_or_null() { + super::wrap_handler_slot( + &mut new_config.on_request, + server_js, + global, + Self::js_gc_on_request_set, + ); + self.config.on_request = new_config.on_request; } - // Swap on any change, *including* clearing to `.zero` when the reload + // Swap on any change, *including* clearing to ZERO when the reload // config omits the handler, so subsequent `on_web_socket_upgrade` / - // `set_routes` stop routing through the node:http path. `take()` yields - // `None` when the new config omitted it; assignment drops the old Strong. + // `set_routes` stop routing through the node:http path. // - // Never the other direction: a server that was not created as a node:http - // server cannot become one through reload(). listen() already sized every - // future connection's socket ext block for this server's kind - // (HttpResponseData vs the bigger NodeHttpResponseData) and set_routes - // would swap the context onto the node:http handler instantiation under - // those already-sized allocations, so the node request path would - // construct and index past them. - if self.config.on_node_http_request.is_some() - && self.config.on_node_http_request.as_ref().map(Strong::get) - != new_config.on_node_http_request.as_ref().map(Strong::get) + // Never the other direction: a server that was not created as a + // node:http server cannot become one through reload(). listen() + // already sized every future connection's socket ext block for this + // server's kind (HttpResponseData vs the bigger NodeHttpResponseData) + // and set_routes would swap the context onto the node:http handler + // instantiation under those already-sized allocations, so the node + // request path would construct and index past them. + if !self.config.on_node_http_request.is_empty() + && self.config.on_node_http_request != new_config.on_node_http_request { - self.config.on_node_http_request = new_config.on_node_http_request.take(); + super::wrap_handler_slot( + &mut new_config.on_node_http_request, + server_js, + global, + Self::js_gc_on_node_http_request_set, + ); + self.config.on_node_http_request = new_config.on_node_http_request; } - if new_config - .on_error - .as_ref() - .is_some_and(|s| !s.get().is_undefined()) - { - self.config.on_error = new_config.on_error.take(); + if !new_config.on_error.is_empty_or_undefined_or_null() { + super::wrap_handler_slot( + &mut new_config.on_error, + server_js, + global, + Self::js_gc_on_error_set, + ); + self.config.on_error = new_config.on_error; } if let Some(mut ws) = new_config.websocket.take() { + // `Handler::from_js` already rejected configs with no non-error + // callback, so any `Some(ws)` is adoptable — match initial-serve + // and adopt unconditionally. ws.handler .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() { - old_ws.unprotect(); - } - ws.global_object = bun_ptr::BackRef::new(global); - self.config.websocket = Some(ws); - } else { - // Not adopting it: release the protections taken in - // `WebSocketServerContext::on_create` so the handlers don't leak. - ws.unprotect(); - } + ws.global_object = bun_ptr::BackRef::new(global); + self.config.websocket = Some(ws); + self.write_ws_handler_slots(server_js, global); } // These get re-applied when we set the static routes again. @@ -2235,11 +2283,7 @@ where let route_list_value = self.set_routes(); if new_config.had_routes_object { - if let Some(server_js_value) = self.js_value.try_get() { - if !server_js_value.is_empty() { - Self::js_gc_route_list_set(server_js_value, global, route_list_value); - } - } + Self::js_gc_route_list_set(server_js, global, route_list_value); } if self.inspector_server_id.get() != 0 { @@ -2266,10 +2310,8 @@ where } let route_list_value = self.set_routes(); if !route_list_value.is_empty() { - if let Some(server_js_value) = self.js_value.try_get() { - if !server_js_value.is_empty() { - Self::js_gc_route_list_set(server_js_value, &self.global(), route_list_value); - } + if let Some(server_js_value) = self.js_value_for_dispatch() { + Self::js_gc_route_list_set(server_js_value, &self.global(), route_list_value); } } Ok(true) @@ -2303,6 +2345,11 @@ where return Err(JsError::Thrown); } + // `on_reload_from_zig` moves `new_config.websocket` into the unscanned + // `self.config` heap box before `write_ws_handler_slots` roots the 7 + // ws shadows, and each `wrap_handler_slot` call allocates via + // `with_async_context_if_needed`. Same window as `serve()`; same fix. + let _handler_pins = super::protect_handler_shadows(&new_config); self.on_reload_from_zig(&mut new_config, global); Ok(self.js_value.try_get().unwrap_or(JSValue::UNDEFINED)) @@ -2312,7 +2359,7 @@ where pub fn on_fetch(&mut self, ctx: &JSGlobalObject, callframe: &CallFrame) -> JsResult { jsc::mark_binding!(); - if self.config.on_request.is_none() { + if self.config.on_request.is_empty() { return Ok( JSPromise::dangerously_create_rejected_promise_value_without_notifying_vm( ctx, @@ -2444,9 +2491,9 @@ where // local going out of scope does not also drop it (double-free / UAF). let request: *mut Request = bun_core::heap::into_raw(existing_request); - debug_assert!(self.config.on_request.is_some()); // confirmed above + debug_assert!(!self.config.on_request.is_empty()); // confirmed above let global_this = self.global(); - let on_request = self.config.on_request.as_ref().unwrap().get(); + let on_request = self.config.on_request; // SAFETY: `request` was just allocated via `heap::alloc`; ownership // transfers to the JS wrapper inside `to_js`. let request_value = unsafe { (*request).to_js(&global_this) }; @@ -2700,12 +2747,11 @@ where if !self.has_listener() && self.pending_requests == 0 { return JSPromise::resolved_promise(global, JSValue::UNDEFINED).to_js(); } - let prom = &mut self.all_closed_promise; - if prom.has_value() { - return prom.value(); + if self.all_closed_promise.has_value() { + return self.all_closed_promise.value(); } - *prom = jsc::JSPromiseStrong::init(global); - prom.value() + self.all_closed_promise = jsc::JSPromiseStrong::init(global); + self.all_closed_promise.value() } // `notify_inspector_server_stopped` lives in the unbounded impl block @@ -2715,7 +2761,7 @@ where if !Self::HAS_H3 { unreachable!(); } - if self.config.on_request.is_none() { + if self.config.on_request.is_empty() { return Self::on_h3_404(self, req, resp); } self.on_request_for::>(req, resp); @@ -2812,6 +2858,11 @@ where let server = unsafe { &mut *server_ptr }; let index = user_route.id; + let Some(server_js) = server.js_value_for_dispatch() else { + respond_stopped_503(resp); + return; + }; + let should_deinit_context = core::cell::Cell::new(false); let Some(mut prepared) = server.prepare_js_request_context_for::( req, @@ -2829,8 +2880,7 @@ where // SAFETY: `server_ptr` outlives `prepared`; reborrow to break the // exclusive lifetime tie between `prepared` and `server`. let server = unsafe { &mut *server_ptr }; - let server_request_list = - Self::js_route_list_get_cached(server.js_value_assert_alive()).unwrap(); + let server_request_list = Self::js_route_list_get_cached(server_js).unwrap(); let call_route = if Ctx::IS_H3 { Bun__ServerRouteList__callRouteH3 } else { @@ -2842,7 +2892,7 @@ where global, index, prepared.request_object, - server.js_value_assert_alive(), + server_js, server_request_list, &mut prepared.js_request, std::ptr::from_mut(req).cast::(), @@ -2896,6 +2946,10 @@ where req: &mut Ctx::Req, resp: &mut Ctx::Resp, ) { + let Some(js_value) = self.js_value_for_dispatch() else { + respond_stopped_503(resp); + return; + }; let self_ptr: *mut Self = self; let should_deinit_context = core::cell::Cell::new(false); let Some(prepared) = self.prepare_js_request_context_for::( @@ -2912,16 +2966,10 @@ where // (`config.on_request`, `global_this`, `js_value`) are disjoint from // the request/ctx allocations it references. Reborrow to satisfy NLL. let this = unsafe { &mut *self_ptr }; - debug_assert!(this.config.on_request.is_some()); + debug_assert!(!this.config.on_request.is_empty()); let global = this.global_this(); - let js_value = this.js_value_assert_alive(); - let on_request_fn = this - .config - .on_request - .as_ref() - .map(|s| s.get()) - .unwrap_or(JSValue::UNDEFINED); + let on_request_fn = this.config.on_request; let response_value = match on_request_fn.call(global, js_value, &[prepared.js_request, js_value]) { Ok(v) => v, @@ -3193,6 +3241,11 @@ where let server_ptr = server_ref.as_ptr(); let index = this.id; + let Some(server_js) = server_ref.js_value_for_dispatch() else { + respond_stopped_503(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. @@ -3208,8 +3261,6 @@ where }; // SAFETY: `prepared.ctx` is the freshly-allocated RequestContext slot. unsafe { (*prepared.ctx).upgrade_context = Some(upgrade_ctx) }; - // BACKREF: `server_ref` outlives this request (see decl above). - let server_js = server_ref.js_value_assert_alive(); let server_request_list = Self::js_route_list_get_cached(server_js).unwrap(); // S008: `JSGlobalObject` is an `opaque_ffi!` ZST — safe deref. let global = bun_opaque::opaque_deref(server_ref.global_this); @@ -3269,13 +3320,21 @@ where // (mod.rs `app.ws("/*", self_ptr, 0, ..)`); live for the request's // duration. let this = unsafe { &mut *self_ptr }; - if this.config.on_node_http_request.is_some() { + // Guards both branches below: the `on_request` fallthrough has no + // other gate, and the node:http branch's own re-check (mod.rs: + // `on_node_http_request_with_upgrade_ctx`) is redundant on this path + // but load-bearing for its other caller (`on_node_http_request`). + let Some(server_js) = this.js_value_for_dispatch() else { + respond_stopped_503(resp); + return; + }; + if !this.config.on_node_http_request.is_empty() { // NOTE: receiver is `*mut Self` (mod.rs) — the callee re-enters // JS, so a long-lived `&mut self` here would alias on callback. Self::on_node_http_request_with_upgrade_ctx(self_ptr, req, resp, upgrade_ctx); return; } - if this.config.on_request.is_none() { + if this.config.on_request.is_empty() { // require fetch method to be set otherwise we dont know what route to call // this should be the fallback in case no route is provided to upgrade resp.write_status(b"403 Forbidden"); @@ -3335,18 +3394,10 @@ where // We keep the Request object alive for the duration of the request so that we can remove the pointer to the UWS request object. let global = this.global(); // SAFETY: `request_object_ptr` is live; no other borrow is outstanding. - let args = [ - unsafe { (*request_object_ptr).to_js(&global) }, - this.js_value_assert_alive(), - ]; - let request_value = args[0]; - request_value.ensure_still_alive(); - - let response_value = match this.config.on_request.as_ref().unwrap().get().call( - &global, - this.js_value_assert_alive(), - &args, - ) { + let args = [unsafe { (*request_object_ptr).to_js(&global) }, server_js]; + args[0].ensure_still_alive(); + + let response_value = match this.config.on_request.call(&global, server_js, &args) { Ok(v) => v, Err(err) => global.take_exception(err), }; @@ -3360,7 +3411,7 @@ where // SAFETY: self_ptr is live for the request's duration; the &mut held // by ctx.create's BACKREF aliases disjoint fields. - ctx.on_response(unsafe { &*self_ptr }, request_value, response_value); + ctx.on_response(unsafe { &*self_ptr }, args[0], response_value); ctx.defer_deinit_until_callback_completes = None; @@ -3498,9 +3549,13 @@ where error_code: u8, raw_packet: &[u8], ) { - let Some(callback) = self.on_clienterror.get() else { + if self.js_value_for_dispatch().is_none() { return; - }; + } + let callback = self.on_clienterror; + if callback.is_empty() { + return; + } { let is_ssl = SSL; let global = self.global(); @@ -3546,9 +3601,13 @@ where /// `onConnection` callback so `node:http` can emit 'connection' before any /// request bytes arrive. pub fn on_connection_callback(&mut self, socket: *mut c_void) { - let Some(callback) = self.on_connection.get() else { + if self.js_value_for_dispatch().is_none() { return; - }; + } + let callback = self.on_connection; + if callback.is_empty() { + return; + } let global = self.global(); let node_socket = match jsc::from_js_host_call(&global, || { Bun__getOrCreateNodeHTTPServerSocket(SSL, socket, &global) @@ -3671,8 +3730,13 @@ pub(super) fn server_set_on_client_error_( // SAFETY: as_ returned a non-null *mut to a live server. let this = unsafe { &mut *this }; if let Some(app) = this.app { - this.on_clienterror.deinit(); - this.on_clienterror = StrongOptional::create(callback, global); + this.on_clienterror = callback; + super::wrap_handler_slot( + &mut this.on_clienterror, + server, + global, + <$T>::js_gc_on_client_error_set, + ); // uws_sys::App::on_client_error takes the raw C-ABI handler shape; // wrap our typed callback in an extern "C" thunk that slices raw_packet. extern "C" fn thunk( @@ -3734,8 +3798,13 @@ pub(super) fn server_set_on_connection_( // SAFETY: as_ returned a non-null *mut to a live server. let this = unsafe { &mut *this }; if let Some(app) = this.app { - this.on_connection.deinit(); - this.on_connection = StrongOptional::create(callback, global); + this.on_connection = callback; + super::wrap_handler_slot( + &mut this.on_connection, + server, + global, + <$T>::js_gc_on_connection_set, + ); // uws filters fire with `1` when an HTTP connection is opened // (for TLS, when its handshake completes) and `-1` on close; // only the open notification is forwarded to JS. 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..5addb57ce4c4 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,49 @@ 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 < 30 && liveServerWrappers() > target; i++) { + Bun.gc(true); + fullGC(); + await new Promise(resolve => setImmediate(resolve)); + } +} + +// `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", () => { + // libuv platforms may materialize both Debug and non-Debug prototypes. + expect(serverWrapperBaseline).toBeLessThanOrEqual(2); +}); + +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 4be490e0b093..32a8b92f381c 100644 --- a/test/js/bun/http/bun-server.test.ts +++ b/test/js/bun/http/bun-server.test.ts @@ -1,6 +1,14 @@ import type { Server, ServerWebSocket, Socket } from "bun"; import { describe, expect, test } from "bun:test"; -import { bunEnv, bunExe, isWindows, rejectUnauthorizedScope, tempDirWithFiles, tls } from "harness"; +import { + bunEnv, + bunExe, + isWindows, + normalizeBunSnapshot, + rejectUnauthorizedScope, + tempDirWithFiles, + tls, +} from "harness"; import path from "path"; describe.concurrent("Server", () => { @@ -623,6 +631,354 @@ test("should be able to await server.stop(true) with keep alive", async () => { expect(async () => await fetch(server.url)).toThrow(); }); +// Shared rig for the two "late keep-alive" tests below: open a raw TCP +// socket, hold the first request in-flight across stop()/close(), pipeline a +// second request behind it, release, GC, and print the second response's +// status line. The subprocess runs the rig so a (former) panic in the dispatch +// trampoline surfaces as a non-zero exit instead of taking down the runner. +// +// The wrapper's `js_value` downgrades to Weak once the first request +// completes (pending_requests → 0 in `deinit_if_we_can`). While Weak the +// wrapper cell is still alive and its WriteBarrier slots still root the +// handlers, so the pipelined request must dispatch cleanly — no panic, and a +// 200 from the same handler. The `respond_stopped_503` guard in the +// trampolines is a safety net for the `Finalized` case (wrapper GC'd while +// `self` still lives between `finalize()` and the next-tick +// `schedule_deinit`); that window is not deterministically reachable from a +// test, so these pin the Weak→dispatch path plus clean collection afterwards. +// +// `serverSnippet` must define `port` (the listen port) and `stop()` in scope, +// and may read `release`/`inflight`/`hits` for the hold protocol. +async function runLateKeepAlive(reqPath: string, serverSnippet: string) { + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + ` + const { fullGC } = require("bun:jsc"); + + let received = ""; + let sockClosed = false; + let waiter = Promise.withResolvers(); + // Wait until a complete HTTP/1.1 response (headers + content-length + // body, or empty for 503) has arrived, then consume + return its + // status line. + const nextResponse = async () => { + while (true) { + const headerEnd = received.indexOf("\\r\\n\\r\\n"); + if (headerEnd !== -1) { + const head = received.slice(0, headerEnd); + const m = /content-length: (\\d+)/i.exec(head); + const bodyLen = m ? Number(m[1]) : 0; + const total = headerEnd + 4 + bodyLen; + if (received.length >= total) { + const status = head.split("\\r\\n")[0]; + received = received.slice(total); + return status; + } + } + if (sockClosed) return ""; + await waiter.promise; + waiter = Promise.withResolvers(); + } + }; + + const release = Promise.withResolvers(); + const inflight = Promise.withResolvers(); + let hits = 0; + + await (async () => { + ${serverSnippet} + + globalThis.sock = await Bun.connect({ + hostname: "127.0.0.1", + port, + socket: { + data(_s, d) { received += d.toString("latin1"); waiter.resolve(); }, + close() { sockClosed = true; waiter.resolve(); }, + error() { sockClosed = true; waiter.resolve(); }, + }, + }); + + // First request: handler parks on \`release\`, keeping + // pending_requests > 0 so stop() defers the js_value downgrade. + sock.write("GET ${reqPath} HTTP/1.1\\r\\nHost: x\\r\\nConnection: keep-alive\\r\\n\\r\\n"); + await inflight.promise; + // Pipeline the late request behind the held one. uws won't read it + // until the first response is sent, by which time js_value is Weak. + sock.write("GET ${reqPath} HTTP/1.1\\r\\nHost: x\\r\\nConnection: close\\r\\n\\r\\n"); + + // Graceful stop: listener closes; downgrade deferred (request in flight). + stop(); + })(); + // The only server binding is now out of scope. + + // First request completes → pending_requests → 0 → js_value downgrades + // to Weak. The pipelined request then hits the trampoline with the + // wrapper still alive (Weak) → handler runs → 200. Previously: panic + // (or 503 when the gate checked Strong-only). + release.resolve(); + const first = await nextResponse(); + if (!first.includes("200")) throw new Error("first request failed: " + first); + const second = await nextResponse(); + + // Wrapper is now Weak and unreferenced; GC must collect it cleanly. + for (let i = 0; i < 3; i++) { + Bun.gc(true); + fullGC(); + await Bun.sleep(0); + } + console.log(second); + + sock.end(); + process.exit(0); + `, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + // Must actually dispatch — empty would mean the socket was closed before the + // pipelined request reached the trampoline. + expect({ stdout: stdout.trim(), stderr, exitCode }).toEqual({ + stdout: expect.stringMatching(/^HTTP\/1\.1 200\b/), + stderr: "", + exitCode: 0, + }); +} + +test("late keep-alive request to a route after stop() dispatches while the wrapper is Weak", async () => { + // Per-route handlers live in ServerRouteList, which is reachable from JS only + // through the Server wrapper — exercises on_user_route_request's gate. + await runLateKeepAlive( + "/r", + ` + const server = Bun.serve({ + port: 0, + hostname: "127.0.0.1", + routes: { + "/r": async () => { + if (++hits === 1) { + inflight.resolve(); + await release.promise; // keep pending_requests > 0 across stop() + } + return new Response("ok"); + }, + }, + }); + const port = server.port; + const stop = () => server.stop(); + `, + ); +}); + +test("late keep-alive WebSocket upgrade after stop()+idle is refused by server.upgrade()", async () => { + // Sibling of the HTTP late-keep-alive test for the WebSocket upgrade path. + // After `deinit_if_we_can` downgrades the wrapper AND clears + // `handler.server`/`handler.app`, `js_value_for_dispatch()` still lets the + // pipelined request reach `fetch()` while the wrapper is Weak, but + // `server.upgrade()` must return false: accepting would create a + // `ServerWebSocket` whose open/close accounting is skipped (`handler.server` + // is None), so `has_active_web_sockets()` would stay false and the next idle + // pass could free the `NewServer` box under a live socket. + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + /* js */ ` + const release = Promise.withResolvers(); + const inflight = Promise.withResolvers(); + let upgraded; + const server = Bun.serve({ + port: 0, hostname: "127.0.0.1", + async fetch(req, server) { + if (req.headers.get("x-hold")) { + inflight.resolve(); + await release.promise; + return new Response("held", { headers: { "content-length": "4" } }); + } + upgraded = server.upgrade(req); + if (upgraded) return; + return new Response("426 no-upgrade", { status: 426, headers: { "content-length": "14" } }); + }, + websocket: { open() {}, message() {}, close() {} }, + }); + const port = server.port; + let received = ""; + let sockClosed = false; + let waiter = Promise.withResolvers(); + globalThis.sock = await Bun.connect({ + hostname: "127.0.0.1", port, + socket: { + data(_s, d) { received += d.toString("latin1"); waiter.resolve(); }, + close() { sockClosed = true; waiter.resolve(); }, + error() { sockClosed = true; waiter.resolve(); }, + }, + }); + // Hold one request so stop() can't downgrade yet. + sock.write("GET / HTTP/1.1\\r\\nHost: x\\r\\nx-hold: 1\\r\\nConnection: keep-alive\\r\\n\\r\\n"); + await inflight.promise; + // Pipeline the upgrade behind it. + const key = Buffer.from(crypto.getRandomValues(new Uint8Array(16))).toString("base64"); + sock.write("GET / HTTP/1.1\\r\\nHost: x\\r\\nUpgrade: websocket\\r\\nConnection: Upgrade\\r\\nSec-WebSocket-Key: " + key + "\\r\\nSec-WebSocket-Version: 13\\r\\n\\r\\n"); + server.stop(); + release.resolve(); + // Wait for both responses. + while (!sockClosed && (received.match(/\\r\\n\\r\\n/g) || []).length < 2) { + await waiter.promise; waiter = Promise.withResolvers(); + } + // Response bodies are not CRLF-terminated, so the next status line is + // glued to the previous body; match status lines by pattern. + const statuses = [...received.matchAll(/HTTP\\/1\\.1 \\d{3} [^\\r\\n]*/g)].map(m => m[0]); + sock.end(); + console.log(JSON.stringify({ statuses, upgraded })); + `, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + const out = JSON.parse(stdout.trim() || "{}"); + expect({ stderr, exitCode }).toEqual({ stderr: "", exitCode: 0 }); + // First request 200 (held handler), pipelined upgrade refused → 426 body. + expect(out.upgraded).toBe(false); + expect(out.statuses?.[0]).toMatch(/^HTTP\/1\.1 200\b/); + expect(out.statuses?.[1]).toMatch(/^HTTP\/1\.1 426\b/); +}); + +test("late keep-alive request to a node:http server after close() dispatches while the wrapper is Weak", async () => { + // Same shape but through node:http so the request dispatches via + // on_node_http_request_with_upgrade_ctx — the trampoline that would panic + // on a stale shadow without the `js_value_for_dispatch` gate. + await runLateKeepAlive( + "/", + ` + const http = require("node:http"); + const srv = http.createServer(async (req, res) => { + if (++hits === 1) { + inflight.resolve(); + await release.promise; // hold socket non-idle through close() + } + res.writeHead(200, { "content-length": 2 }); + res.end("ok"); + }); + await new Promise(r => srv.listen(0, "127.0.0.1", r)); + const port = srv.address().port; + // close() → closeIdleConnections() (skips this socket) → stop(). + // Also drops node:http's own reference to the Bun server. + const stop = () => srv.close(); + `, + ); +}); + +test("server wrapper survives GC while a websocket is connected after stop()", async () => { + // The previous test exercises the one-tick HTTP keep-alive race; this one + // covers the steadier websocket case. After a graceful stop() with a live + // websocket, the user may drop their `server` binding. The native struct + // stays alive (active_websockets > 0), but stop() previously downgraded + // js_value immediately, so GC could finalize the JS wrapper — and with it + // m_routeList — while the connection was still in use. With the downgrade + // deferred into deinit_if_we_can's idle predicate, the wrapper must outlive + // the websocket and become collectable only after the last close. + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + ` + const { fullGC, heapStats } = require("bun:jsc"); + + const serverCount = () => { + const c = heapStats().objectTypeCounts; + return (c.DebugHTTPServer ?? 0) + (c.HTTPServer ?? 0); + }; + + async function drain(target) { + for (let i = 0; i < 30 && serverCount() > target; i++) { + Bun.gc(true); + fullGC(); + await new Promise(r => setImmediate(r)); + await Bun.sleep(10); + } + } + + // objectTypeCounts includes the (lazily created) prototype object(s) + // once the first server is constructed — and on libuv platforms both + // Debug and non-Debug prototypes may end up materialized. Create+stop + // a trivial server first so the baseline captures whatever prototype + // floor this build settles at; assertions are then relative to it. + await (async () => { + const s = Bun.serve({ port: 0, fetch: () => new Response("ok") }); + s.stop(true); + })(); + await drain(0); + const baseline = serverCount(); + + const ws = await (async () => { + const server = Bun.serve({ + port: 0, + hostname: "127.0.0.1", + routes: { "/r": () => new Response("ok") }, + fetch(req, server) { + if (server.upgrade(req)) return; + return new Response("nope", { status: 404 }); + }, + websocket: { open() {}, message() {}, close() {} }, + }); + + const opened = Promise.withResolvers(); + const ws = new WebSocket("ws://127.0.0.1:" + server.port); + ws.onopen = () => opened.resolve(); + ws.onerror = e => opened.reject(e); + await opened.promise; + + // Graceful stop: listener closes, the live websocket stays open. + server.stop(); + return ws; + })(); + // The only \`server\` binding is now out of scope; only the live + // websocket keeps the native side around. + + for (let i = 0; i < 30; i++) { + Bun.gc(true); + fullGC(); + await new Promise(r => setImmediate(r)); + await Bun.sleep(10); + } + const afterStopGC = serverCount(); + + const closed = Promise.withResolvers(); + ws.onclose = () => closed.resolve(); + ws.close(); + await closed.promise; + + await drain(baseline); + const afterCloseGC = serverCount(); + + console.log(JSON.stringify({ baseline, afterStopGC, afterCloseGC })); + process.exit(0); + `, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + const { baseline, afterStopGC, afterCloseGC } = JSON.parse(stdout.trim() || "{}"); + expect({ stderr, exitCode }).toEqual({ stderr: "", exitCode: 0 }); + // js_value stays Strong while a websocket is connected → GC must not + // collect the wrapper. baseline already includes the prototype(s), so the + // live instance shows as baseline+1. + expect(afterStopGC).toBeGreaterThan(baseline); + // Last websocket closing triggers deinit_if_we_can → downgrade → wrapper + // becomes collectable again (no leak). + expect(afterCloseGC).toBe(baseline); +}, 15_000); + 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({ @@ -1480,3 +1836,806 @@ test("HEAD request for a Response with an S3 file body reports the object size a expect(stdout.trim()).toBe("s3-head-ok"); expect(exitCode).toBe(0); }); + +// Handler callbacks (fetch/error/websocket.*) are stored on the JS wrapper and +// traced by the GC rather than independently rooted. These tests lock in that +// reload/stop transitions never leave a window where a handler is collected +// while a dispatch path can still reach it. +describe("handler liveness across reload/stop", () => { + test("server.reload({ fetch }) swaps the handler for the next request", async () => { + using server = Bun.serve({ + port: 0, + hostname: "127.0.0.1", + fetch() { + return new Response("first"); + }, + }); + + expect(await (await fetch(server.url)).text()).toBe("first"); + + server.reload({ + fetch() { + return new Response("second"); + }, + }); + // Drop any last reference the test frame holds to the old handler, then + // collect. The new handler must be the one the wrapper traces now. + Bun.gc(true); + + expect(await (await fetch(server.url)).text()).toBe("second"); + + // A second reload back-to-back must also take effect (catches a stale + // cached read of the previous slot value). + server.reload({ + fetch() { + return new Response("third"); + }, + }); + Bun.gc(true); + expect(await (await fetch(server.url)).text()).toBe("third"); + }); + + test("in-flight request completes with its handler after stop() + GC", async () => { + const received = Promise.withResolvers(); + const release = Promise.withResolvers(); + let handlerRan = 0; + + const server = Bun.serve({ + port: 0, + hostname: "127.0.0.1", + async fetch() { + handlerRan++; + received.resolve(); + await release.promise; + return new Response("in-flight-ok", { headers: { Connection: "close" } }); + }, + }); + + const responsePromise = fetch(server.url); + await received.promise; + + // stop() drops the listener while the request is mid-handler. The wrapper + // must remain live (pending_requests > 0) so the handler the request was + // dispatched into is still reachable. + const stopped = server.stop(); + Bun.gc(true); + + release.resolve(); + const body = await (await responsePromise).text(); + await stopped; + + expect(body).toBe("in-flight-ok"); + expect(handlerRan).toBe(1); + }); + + test("websocket close handler fires when stop() closes an open connection", async () => { + const opened = Promise.withResolvers(); + const serverClose = Promise.withResolvers<{ code: number; reason: string }>(); + const clientClose = Promise.withResolvers(); + + const server = Bun.serve({ + port: 0, + hostname: "127.0.0.1", + fetch(req, server) { + if (server.upgrade(req)) return; + return new Response(null, { status: 404 }); + }, + websocket: { + open() { + opened.resolve(); + }, + message() {}, + close(_ws, code, reason) { + serverClose.resolve({ code, reason }); + }, + }, + }); + + const ws = new WebSocket(`ws://127.0.0.1:${server.port}/`); + ws.onclose = () => clientClose.resolve(); + await opened.promise; + + // Connection is open; force-stop the server. The wrapper must stay live + // long enough for the close callback (read off the wrapper) to fire. + Bun.gc(true); + const stopped = server.stop(true); + Bun.gc(true); + + const { code } = await serverClose.promise; + await clientClose.promise; + await stopped; + + // The invariant is that the close handler ran at all (it's read off the + // wrapper after stop()); the exact close code is uws's choice. + expect(typeof code).toBe("number"); + expect(code).toBeGreaterThanOrEqual(1000); + }); + + test("ws.close() with a reason whose toString() re-enters close() decrements the count once", async () => { + // ServerWebSocket.close coerces the reason arg via toString(), which can + // re-enter ws.close() before the outer call sets the closed flag. The + // re-check after coercion ensures only one on_websocket_closed() runs. + let openCount = 0; + const bothOpen = Promise.withResolvers(); + const targetClosed = Promise.withResolvers(); + let reentered = 0; + let closedTarget: unknown; + + using server = Bun.serve({ + port: 0, + hostname: "127.0.0.1", + fetch(req, server) { + if (server.upgrade(req)) return; + return new Response(null, { status: 404 }); + }, + websocket: { + open() { + if (++openCount === 2) bothOpen.resolve(); + }, + message(ws, m) { + if (m === "do-close") { + // Only c1 sends this; capture c1's server-side peer here rather + // than by open() order, which is not guaranteed across platforms. + closedTarget = ws; + ws.close(1000, { + toString() { + reentered++; + ws.close(); // re-entrant close before outer sets closed=true + return "bye"; + }, + } as unknown as string); + } + }, + close(ws) { + if (ws === closedTarget) targetClosed.resolve(); + }, + }, + }); + + const c1 = new WebSocket(`ws://127.0.0.1:${server.port}/`); + const c2 = new WebSocket(`ws://127.0.0.1:${server.port}/`); + const c1Open = Promise.withResolvers(); + const c2Open = Promise.withResolvers(); + const c1Closed = Promise.withResolvers(); + c1.onopen = () => c1Open.resolve(); + c2.onopen = () => c2Open.resolve(); + c1.onerror = e => c1Open.reject(e); + c2.onerror = e => c2Open.reject(e); + c1.onclose = () => c1Closed.resolve(); + await Promise.all([bothOpen.promise, c1Open.promise, c2Open.promise]); + expect(server.pendingWebSockets).toBe(2); + + c1.send("do-close"); + await targetClosed.promise; + await c1Closed.promise; + + // Without the re-check, the outer close() would decrement again: 2→0. + expect({ reentered, pending: server.pendingWebSockets }).toEqual({ reentered: 1, pending: 1 }); + + const c2Closed = Promise.withResolvers(); + c2.onclose = () => c2Closed.resolve(); + c2.close(); + await c2Closed.promise; + }); + + test("server.fetch() still dispatches to the handler after stop()", async () => { + const server = Bun.serve({ + port: 0, + hostname: "127.0.0.1", + fetch(req) { + return new Response("via server.fetch: " + new URL(req.url).pathname); + }, + }); + + const url = `http://${server.hostname}:${server.port}/after-stop`; + + server.stop(); + Bun.gc(true); + + // No listener, but the JS wrapper is still on our stack — server.fetch() + // reads the handler off the wrapper, so it must still resolve. + const response = await server.fetch(url); + expect(await response.text()).toBe("via server.fetch: /after-stop"); + expect(response.status).toBe(200); + }); +}); + +// The native↔JS cycle: a handler that closes over `server` used to be +// uncollectable because ServerConfig held it as a Strong root. With handlers +// stored as WriteBarrier slots on the wrapper, the cycle is all-JS-heap and +// GC collects it once nothing else references the wrapper. +describe("handler GC tracing (heapStats wrapper-count)", () => { + test("server with handler closing over itself is collected after stop()", async () => { + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + /* js */ ` + const { heapStats, fullGC } = require("bun:jsc"); + const live = () => { + const c = heapStats().objectTypeCounts; + return (c.DebugHTTPServer ?? 0) + (c.HTTPServer ?? 0); + }; + async function drain(target) { + for (let i = 0; i < 30 && live() > target; i++) { + Bun.gc(true); + fullGC(); + await new Promise(r => setImmediate(r)); + await Bun.sleep(10); + } + } + + // Materialize prototype(s) first so baseline = whatever floor this + // build settles at (libuv platforms may surface 2, not 1). + await (async () => { + const s = Bun.serve({ port: 0, development: true, fetch: () => new Response("ok") }); + s.stop(true); + })(); + await drain(0); + const baseline = live(); + + await (async () => { + const server = Bun.serve({ + port: 0, + development: true, + // Closes over server — the cycle. + fetch: () => new Response("port " + server.port), + error: e => { server.stop(); return new Response(String(e)); }, + }); + const r = await fetch(server.url, { keepalive: false }); + if (!(await r.text()).startsWith("port ")) throw new Error("dispatch broke"); + server.stop(true); + })(); + // No live reference to server or its handlers from here. + await drain(baseline); + console.log(JSON.stringify({ baseline, after: live() })); + `, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + const { baseline, after } = JSON.parse(stdout.trim() || "{}"); + expect({ stderr, exitCode }).toEqual({ stderr: "", exitCode: 0 }); + // baseline already includes the prototype(s); a collected instance returns + // to it exactly. On main this fails: the cycle keeps the instance alive + // (after = baseline+1). + expect(after).toBe(baseline); + }, 15_000); + + // Control: a handler that does NOT close over server is collected on main + // today. This pins that the redesign doesn't regress the non-cycle case. + test("server with handler NOT closing over itself is collected (control)", async () => { + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + /* js */ ` + const { heapStats, fullGC } = require("bun:jsc"); + const live = () => { + const c = heapStats().objectTypeCounts; + return (c.DebugHTTPServer ?? 0) + (c.HTTPServer ?? 0); + }; + async function drain(target) { + for (let i = 0; i < 30 && live() > target; i++) { + Bun.gc(true); fullGC(); + await new Promise(r => setImmediate(r)); + await Bun.sleep(10); + } + } + await (async () => { + const s = Bun.serve({ port: 0, development: true, fetch: () => new Response("ok") }); + s.stop(true); + })(); + await drain(0); + const baseline = live(); + + await (async () => { + const server = Bun.serve({ + port: 0, development: true, + fetch: () => new Response("ok"), + }); + await fetch(server.url, { keepalive: false }); + server.stop(true); + })(); + await drain(baseline); + console.log(JSON.stringify({ baseline, after: live() })); + `, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + const { baseline, after } = JSON.parse(stdout.trim() || "{}"); + expect({ stderr, exitCode }).toEqual({ stderr: "", exitCode: 0 }); + expect(after).toBe(baseline); + }, 15_000); + + // JSServerWebSocket holds a traced reference to the JSServer wrapper, so the + // server (and its ws handlers) stay alive while any websocket is connected, + // and become collectable once the last one closes. + test("server stays alive while a websocket is connected, then collects after close", async () => { + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + /* js */ ` + const { heapStats, fullGC } = require("bun:jsc"); + const liveServer = () => { + const c = heapStats().objectTypeCounts; + return (c.DebugHTTPServer ?? 0) + (c.HTTPServer ?? 0); + }; + + async function gcUntilCountAtMost(max) { + for (let i = 0; i < 30; i++) { + Bun.gc(true); + fullGC(); + if (liveServer() <= max) return liveServer(); + await new Promise(r => setImmediate(r)); + await Bun.sleep(10); + } + return liveServer(); + } + + // Materialize prototype(s) first; baseline = the floor count. + await (async () => { + const s = Bun.serve({ port: 0, development: true, fetch: () => new Response("ok") }); + s.stop(true); + })(); + await gcUntilCountAtMost(0); + const baseline = liveServer(); + + const opened = Promise.withResolvers(); + const clientOpen = Promise.withResolvers(); + const echoed = Promise.withResolvers(); + const closed = Promise.withResolvers(); + + // Scope server so the only post-stop root is the connected websocket. + // Assign client directly to the outer var rather than returning it — + // returning keeps the async frame's scope (which contains server) + // alive via the resolved-value chain in JSC. + let client; + await (async () => { + const server = Bun.serve({ + port: 0, + development: true, + fetch(req, s) { if (s.upgrade(req)) return; return new Response("ok"); }, + websocket: { + open() { opened.resolve(); }, + // Closes over server — the cycle through wsHandlers. + message(ws, m) { ws.send(server.port + ":" + m); }, + }, + }); + client = new WebSocket(server.url.href.replace("http", "ws")); + client.onopen = () => clientOpen.resolve(); + client.onmessage = e => echoed.resolve(e.data); + client.onclose = () => closed.resolve(); + await opened.promise; // server-side ws created (roots wrapper) + await clientOpen.promise; // client ready to send (avoid InvalidStateError) + server.stop(); // graceful — listener gone, ws stays + })(); + + // server out of scope. Wrapper is rooted only via: + // ServerWebSocket(this_value strong) → JSServerWebSocket → m_server → JSServer + // GC must NOT collect while the ws is open. + Bun.gc(true); fullGC(); + const whileConnected = liveServer(); + + // Dispatch through the cycle-captured handler (proves it's alive). + client.send("hi"); + const echo = await echoed.promise; + + client.close(); + await closed.promise; + client = null; + // The last ws closing triggers on_websocket_closed → deinit_if_we_can, + // which downgrades the wrapper without an explicit stop(true) — that's + // the path under test, so no force-finish here. + const afterClose = await gcUntilCountAtMost(baseline); + + console.log(JSON.stringify({ baseline, whileConnected, echo, afterClose })); + `, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + const { baseline, whileConnected, echo, afterClose } = JSON.parse(stdout.trim() || "{}"); + expect({ stderr, exitCode }).toEqual({ stderr: "", exitCode: 0 }); + // baseline already includes the prototype(s); the instance on top of it + // proves the ws traced root kept it alive across GC. + expect(whileConnected).toBeGreaterThan(baseline); + expect(echo).toMatch(/^\d+:hi$/); // handler dispatched (server.port captured) + expect(afterClose).toBe(baseline); // instance collected, back to prototype floor + }, 15_000); + + // Reload swaps handlers via WriteBarrier .set() — old handlers become + // unreachable once nothing else holds them. + test("reload() releases the old handlers for collection", async () => { + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + /* js */ ` + const { heapStats, fullGC } = require("bun:jsc"); + // objectTypeCounts only tracks JSC cell types, not user-defined JS + // classes, so use AsyncFunction as the observable: the OLD handler is + // async (counted), the NEW handler is a plain function (not counted). + const liveAsync = () => heapStats().objectTypeCounts.AsyncFunction ?? 0; + + const baseline = liveAsync(); + const server = Bun.serve({ + port: 0, + fetch: async () => new Response("old"), + }); + const beforeReload = liveAsync(); + server.reload({ fetch: () => new Response("new") }); + for (let i = 0; i < 30 && liveAsync() > baseline; i++) { + Bun.gc(true); + fullGC(); + await new Promise(r => setImmediate(r)); + await Bun.sleep(10); + } + console.log(JSON.stringify({ baseline, beforeReload, afterReload: liveAsync() })); + server.stop(true); + `, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + const { baseline, beforeReload, afterReload } = JSON.parse(stdout.trim() || "{}"); + expect({ stderr, exitCode }).toEqual({ stderr: "", exitCode: 0 }); + expect(beforeReload).toBeGreaterThan(baseline); // sanity: the async handler was counted + expect(afterReload).toBeLessThan(beforeReload); // old handler released after reload + }); + + // reload({websocket}) that omits a previously-set per-event handler must + // CLEAR that wrapper slot, not leave the old handler pinned. + test("reload() that drops a websocket handler clears its slot", async () => { + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + /* js */ ` + const { heapStats, fullGC } = require("bun:jsc"); + const liveAsync = () => heapStats().objectTypeCounts.AsyncFunction ?? 0; + + let oldPingFired = 0; + + const baseline = liveAsync(); + const server = Bun.serve({ + port: 0, + fetch: (req, s) => s.upgrade(req) ? undefined : new Response("ok"), + websocket: { + message(ws, m) { ws.send(m); }, + // async so it shows up in objectTypeCounts.AsyncFunction. + ping: async () => { oldPingFired++; }, + }, + }); + const withPing = liveAsync(); + + // Reload with a websocket config that omits ping. The wsOnPing slot + // must be cleared (not left holding the old async closure). + server.reload({ + fetch: (req, s) => s.upgrade(req) ? undefined : new Response("ok"), + websocket: { message(ws, m) { ws.send(m); } }, + }); + for (let i = 0; i < 30 && liveAsync() > baseline; i++) { + Bun.gc(true); + fullGC(); + await new Promise(r => setImmediate(r)); + await Bun.sleep(10); + } + const afterReload = liveAsync(); + + // Behavioral check: a client ping must not reach the dropped handler. + const opened = Promise.withResolvers(); + const echoed = Promise.withResolvers(); + const ws = new WebSocket(server.url.href.replace("http", "ws")); + ws.onopen = () => opened.resolve(); + ws.onerror = e => { opened.reject(e); echoed.reject(e); }; + ws.onmessage = e => echoed.resolve(e.data); + await opened.promise; + ws.ping("p"); + ws.send("hi"); // round-trip after the ping so any ping dispatch has happened + await echoed.promise; + ws.close(); + server.stop(true); + + console.log(JSON.stringify({ baseline, withPing, afterReload, oldPingFired })); + `, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + const { baseline, withPing, afterReload, oldPingFired } = JSON.parse(stdout.trim() || "{}"); + expect({ stderr, exitCode }).toEqual({ stderr: "", exitCode: 0 }); + expect(withPing).toBeGreaterThan(baseline); // sanity: async ping was counted + expect(afterReload).toBeLessThan(withPing); // dropped slot cleared → old ping collected + expect(oldPingFired).toBe(0); // and never dispatched after reload + }); + + // Stress test under aggressive GC — catches missing write barriers. + test("serve+ws+reload survives BUN_JSC_collectContinuously=1", async () => { + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + /* js */ ` + const server = Bun.serve({ + port: 0, + fetch: (req, s) => s.upgrade(req) ? undefined : new Response("ok"), + websocket: { open() {}, message(ws, m) { ws.send(m); } }, + }); + for (let i = 0; i < 10; i++) { + const ws = new WebSocket(server.url.href.replace("http", "ws")); + // Reject (don't hang) if the connection drops mid-await — under + // collectContinuously a missing write barrier surfaces as an abrupt + // close/error, and a bare onopen-only resolver would just time out. + const fail = Promise.withResolvers(); + ws.onerror = e => fail.reject(e.error ?? new Error("ws error on iter " + i)); + ws.onclose = e => fail.reject(new Error("ws closed (" + e.code + ") on iter " + i)); + await Promise.race([new Promise(r => { ws.onopen = r; }), fail.promise]); + ws.send("hi"); + await Promise.race([new Promise(r => { ws.onmessage = r; }), fail.promise]); + const closed = new Promise(r => { ws.onclose = r; }); // before close(): event may fire synchronously + ws.close(); + await closed; + server.reload({ + fetch: (req, s) => s.upgrade(req) ? undefined : new Response("ok " + i), + websocket: { open() {}, message(ws, m) { ws.send(m + i); } }, + }); + } + server.stop(true); + console.log("survived"); + `, + ], + env: { ...bunEnv, BUN_JSC_collectContinuously: "1", BUN_JSC_useConcurrentGC: "0" }, + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + expect(normalizeBunSnapshot(stdout)).toBe("survived"); + expect(exitCode).toBe(0); + }, 30_000); + + // with_async_context_if_needed wraps each handler in a fresh AsyncContextFrame + // that is NOT a property of the user's options arg. Stored as a raw JSValue in + // heap-boxed ServerConfig, it must stay rooted across init→listen→ptr_to_js→ + // slot-set (which includes vm.perform_gc()). + test("handlers wrapped via AsyncLocalStorage survive Bun.serve init under collectContinuously", async () => { + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + /* js */ ` + const { AsyncLocalStorage } = require("node:async_hooks"); + const als = new AsyncLocalStorage(); + // Serve inside als.run so with_async_context_if_needed wraps every handler. + const server = await als.run({ ctx: 1 }, async () => { + return Bun.serve({ + port: 0, development: true, + fetch: () => new Response(String(als.getStore()?.ctx)), + error: () => new Response("err"), + websocket: { + open() {}, message(ws, m) { ws.send(m); }, close() {}, + }, + }); + }); + const r = await fetch(server.url, { keepalive: false }); + const body = await r.text(); + server.stop(true); + // The handler's ALS context wrapper survived init→ptr_to_js (would crash + // under collectContinuously if the AsyncContextFrame were collected). + console.log(JSON.stringify({ body, ok: body === "1" })); + `, + ], + env: { ...bunEnv, BUN_JSC_collectContinuously: "1", BUN_JSC_useConcurrentGC: "0" }, + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + const { body, ok } = JSON.parse(stdout.trim() || "{}"); + expect({ stderr, exitCode }).toEqual({ stderr: "", exitCode: 0 }); + expect({ body, ok }).toEqual({ body: "1", ok: true }); + }, 30_000); + + // An accessor- or Proxy-backed options object returns a fresh handler fn + // that is NOT a data property of the object, so nothing on the JS heap + // retains it between from_js reading it and serve_with! writing it into the + // wrapper's WriteBarrier slot. Without a scoped gcProtect across + // init()/listen()'s allocations, that fn is collectible; under + // collectContinuously it IS collected, and the first request dispatches + // into a freed cell. Pre-PR this was safe because from_js rooted each + // callback in a Strong the moment get_truthy returned. + test("handlers returned by an accessor-backed options object survive Bun.serve init under collectContinuously", async () => { + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + /* js */ ` + // Each get_truthy("fetch"/"message"/...) hits a getter that allocates + // a fresh closure with no other JS-heap referrer. Use getters (not a + // Proxy) so accidental extra lookups of the same key don't allocate a + // second fn that rotates the first one out of the arena; "extra + // lookup collected the cell early" and "no protect collected it" are + // indistinguishable failures otherwise. + const opts = { + port: 0, + development: true, + get fetch() { return (req, server) => { + if (server.upgrade(req)) return; + return new Response("ok-fetch"); + }; }, + get error() { return () => new Response("err", { status: 500 }); }, + websocket: { + get open() { return ws => ws.send("ws-open"); }, + get message() { return (ws, m) => ws.send("m:" + m); }, + close() {}, + }, + }; + const server = Bun.serve(opts); + // HTTP path (on_request slot). + const body = await (await fetch(server.url, { keepalive: false })).text(); + // WebSocket path (wsOnOpen + wsOnMessage slots). + const ws = new WebSocket(server.url); + const msgs = []; + const got2 = new Promise(r => { + ws.onmessage = e => { msgs.push(e.data); if (msgs.length === 2) r(); }; + ws.onopen = () => ws.send("hi"); + ws.onerror = () => r(); + }); + await got2; + ws.close(); + server.stop(true); + console.log(JSON.stringify({ body, msgs })); + `, + ], + env: { ...bunEnv, BUN_JSC_collectContinuously: "1", BUN_JSC_useConcurrentGC: "0" }, + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + const { body, msgs } = JSON.parse(stdout.trim() || "{}"); + expect({ stderr, exitCode }).toEqual({ stderr: "", exitCode: 0 }); + expect({ body, msgs }).toEqual({ body: "ok-fetch", msgs: ["ws-open", "m:hi"] }); + }, 30_000); + + // Sibling of the above for server.reload(): on_reload_from_zig moves the + // websocket handler shadows into the heap-boxed self.config before + // write_ws_handler_slots roots them, and each wrap_handler_slot allocates + // via with_async_context_if_needed. Pre-PR on_create's server.protect() + // gcProtected all 7 at read time. + test("reload() with accessor-backed websocket handlers survives under collectContinuously", async () => { + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + /* js */ ` + const { AsyncLocalStorage } = require("node:async_hooks"); + const als = new AsyncLocalStorage(); + const server = Bun.serve({ + port: 0, development: true, + fetch(req, s) { if (s.upgrade(req)) return; return new Response("v1"); }, + websocket: { open() {}, message() {}, close() {} }, + }); + // Reload inside als.run so with_async_context_if_needed allocates an + // AsyncContextFrame for every ws handler it wraps (the GC point + // between moving the shadows into self.config and rooting them). + als.run({}, () => server.reload({ + fetch(req, s) { if (s.upgrade(req)) return; return new Response("v2"); }, + websocket: { + get open() { return ws => ws.send("r-open"); }, + get message() { return (ws, m) => ws.send("r:" + m); }, + get close() { return () => {}; }, + get drain() { return () => {}; }, + get ping() { return () => {}; }, + get pong() { return () => {}; }, + }, + })); + const body = await (await fetch(server.url, { keepalive: false })).text(); + const ws = new WebSocket(server.url); + const msgs = []; + await new Promise(r => { + ws.onmessage = e => { msgs.push(e.data); if (msgs.length === 2) r(); }; + ws.onopen = () => ws.send("hi"); + ws.onerror = () => r(); + }); + ws.close(); + server.stop(true); + console.log(JSON.stringify({ body, msgs })); + `, + ], + env: { ...bunEnv, BUN_JSC_collectContinuously: "1", BUN_JSC_useConcurrentGC: "0" }, + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + const { body, msgs } = JSON.parse(stdout.trim() || "{}"); + expect({ stderr, exitCode }).toEqual({ stderr: "", exitCode: 0 }); + expect({ body, msgs }).toEqual({ body: "v2", msgs: ["r-open", "r:hi"] }); + }, 30_000); + + // A ws.close() inside the message handler on the last socket of a stopped + // server downgrades the wrapper (the sole GC root for wsOnError) before the + // message handler returns. The error path must have copied on_error to the + // stack before entering user JS, or a GC between the close and the throw + // collects it and run_error_callback calls a freed cell. + test("error handler survives ws.close()+throw inside the last socket's message handler under collectContinuously", async () => { + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + /* js */ ` + let errorFired = 0; + const opened = Promise.withResolvers(); + const closed = Promise.withResolvers(); + let ws; + // Scope server so the module-level frame holds no reference to the + // wrapper when message(ws) runs; after ws.close() downgrades js_value + // and clears m_server, the wrapper must have zero roots for Bun.gc to + // reach wsOnError. + await (async () => { + const server = Bun.serve({ + port: 0, hostname: "127.0.0.1", + fetch(req, s) { if (s.upgrade(req)) return; return new Response("no"); }, + websocket: { + open() {}, + message(ws) { + ws.close(); // last socket of a stopped server → wrapper downgrades + Bun.gc(true); + throw new Error("boom"); + }, + error(e) { errorFired++; }, + }, + }); + ws = new WebSocket("ws://127.0.0.1:" + server.port); + ws.onopen = () => opened.resolve(); + ws.onerror = e => opened.reject(e); + ws.onclose = () => closed.resolve(); + await opened.promise; + server.stop(); // graceful: listener gone, this ws keeps wrapper Strong + })(); + ws.send("go"); + await closed.promise; + console.log(JSON.stringify({ errorFired })); + process.exit(0); + `, + ], + env: { ...bunEnv, BUN_JSC_collectContinuously: "1", BUN_JSC_useConcurrentGC: "0" }, + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + expect({ out: JSON.parse(stdout.trim() || "null"), stderr, exitCode }).toEqual({ + out: { errorFired: 1 }, + stderr: "", + exitCode: 0, + }); + }, 30_000); +}); diff --git a/test/js/node/http/node-http-uaf.test.ts b/test/js/node/http/node-http-uaf.test.ts index fed1f257ea3d..1693eda1c32a 100644 --- a/test/js/node/http/node-http-uaf.test.ts +++ b/test/js/node/http/node-http-uaf.test.ts @@ -1,5 +1,8 @@ import { expect, test } from "bun:test"; import { bunEnv, bunExe, isASAN } from "harness"; +import { once } from "node:events"; +import http from "node:http"; +import net, { type AddressInfo } from "node:net"; import { join } from "path"; uafTest("node-http-uaf-fixture.ts"); @@ -79,3 +82,32 @@ test.concurrent.each([ }); expect(JSON.parse(stdout).received).toBeGreaterThan(8 * 1024 * 1024); }); + +test("'connection' and 'clientError' callbacks survive GC", async () => { + // The server's native struct stores these two node:http callbacks on the JS + // wrapper (GC-visited WriteBarrier slots), not in Strong handles. Force GC + // between registration and dispatch to prove the wrapper roots them. + let gotConnection = 0; + let gotClientError = 0; + const server = http.createServer((req, res) => res.end()); + server.on("connection", () => void gotConnection++); + server.on("clientError", (err, sock) => { + gotClientError++; + sock.destroy(); + }); + await once(server.listen(0, "127.0.0.1"), "listening"); + try { + Bun.gc(true); + + const sock = net.connect((server.address() as AddressInfo).port, "127.0.0.1"); + sock.on("error", () => {}); + await once(sock, "connect"); + Bun.gc(true); + sock.write("!!!garbage!!!\r\n\r\n"); + await once(sock, "close"); + + expect({ gotConnection, gotClientError }).toEqual({ gotConnection: 1, gotClientError: 1 }); + } finally { + server.close(); + } +});