diff --git a/src/js/node/_http_server.ts b/src/js/node/_http_server.ts index 8e515ee41c0e..3675c4e20f5d 100644 --- a/src/js/node/_http_server.ts +++ b/src/js/node/_http_server.ts @@ -75,6 +75,11 @@ const OutgoingMessagePrototype = OutgoingMessage.prototype; const { kIncomingMessage } = require("node:_http_common"); const kConnectionsCheckingInterval = Symbol("http.server.connectionsCheckingInterval"); const kTrackedConnections = Symbol("http.server.trackedConnections"); +// Set synchronously in close(); tracks whether the server has entered +// the shutdown sequence, so address()/close() behave like Node even +// while the underlying Bun.serve reference is still populated for +// closeAllConnections()/unref() to reach. +const kServerClosed = Symbol("http.server.closed"); const getBunServerAllClosedPromise = $newZigFunction("node_http_binding.zig", "getBunServerAllClosedPromise", 1); const sendHelper = $newZigFunction("node_cluster_binding.zig", "sendHelperChild", 3); @@ -95,7 +100,22 @@ function emitCloseServer(self: Server) { callCloseCallback(self); self.emit("close"); } -function emitCloseNTServer(this: Server) { +function emitCloseNTServer(this: Server, closingServer) { + // The underlying Bun server has finished shutting down — drop our + // reference so the allClosed promise chain can settle and the event + // loop can exit. msal and other consumers call + // `close() → closeAllConnections() → unref()` in sequence; the + // underlying reference has to survive past `close()` so the follow-up + // calls can still reach the native layer. + // + // Guard against re-listen: if the caller already started a fresh + // `listen()` before this callback fired, `this[serverSymbol]` now + // points at a NEW Bun.serve handle — nulling it would trash the new + // listener. Only clear when the handle still matches the one whose + // allClosed promise we're resolving. + if (this[serverSymbol] === closingServer) { + this[serverSymbol] = undefined; + } process.nextTick(emitCloseServer, this); } @@ -212,7 +232,10 @@ function emitRequestCloseNT(self) { } function emitListeningNextTick(self, hostname, port) { - if ((self.listening = !!self[serverSymbol])) { + // `serverSymbol` now survives past close() (see close()/emitCloseNTServer), + // so a close() that raced this deferred tick must not re-announce + // 'listening'; gate on `kServerClosed` like address()/close() do. + if ((self.listening = !!self[serverSymbol] && !self[kServerClosed])) { // TODO: remove the arguments // Note does not pass any arguments. self.emit("listening", null, hostname, port); @@ -336,7 +359,11 @@ function setupConnectionsTracking(this: any) { Server.prototype.ref = function () { this._unref = false; - this[serverSymbol]?.ref?.(); + // Don't re-pin the loop on a server that's already closing — after + // close() `serverSymbol` is still populated but `kServerClosed` is set. + // (unref() below is intentionally NOT gated: it's the third step of + // msal's `close() → closeAllConnections() → unref()` teardown.) + if (!this[kServerClosed]) this[serverSymbol]?.ref?.(); return this; }; @@ -351,10 +378,15 @@ Server.prototype.closeAllConnections = function () { if (!server) { return; } - this[serverSymbol] = undefined; + this[kServerClosed] = true; clearInterval(this[kConnectionsCheckingInterval]); this.listening = false; + // Abrupt stop — force-closes any open connections, including the ones + // a graceful `close()` would have left open. We keep `this[serverSymbol]` + // populated so subsequent calls (e.g. `unref()`) still reach the native + // server; it's cleared in `emitCloseNTServer` once everything actually + // drains. server.stop(true); }; @@ -378,13 +410,18 @@ Server.prototype.close = function (optionalCallback?) { // Node.js's httpServerPreClose clears the connections-checking interval // even when the server was never listening. clearInterval(this[kConnectionsCheckingInterval]); - if (!server) { + if (!server || this[kServerClosed]) { if (typeof optionalCallback === "function") process.nextTick(optionalCallback, $ERR_SERVER_NOT_RUNNING()); return; } - this[serverSymbol] = undefined; + this[kServerClosed] = true; if (typeof optionalCallback === "function") setCloseCallback(this, optionalCallback); this.listening = false; + // Graceful stop: stop accepting, close idle connections. Callers like + // `@azure/msal-node` follow this with `closeAllConnections()` to force- + // close remaining in-flight sockets, so we must NOT null out + // `this[serverSymbol]` here — that reference is still needed by the + // subsequent `closeAllConnections()` / `unref()` calls. server.closeIdleConnections(); server.stop(); }; @@ -426,7 +463,12 @@ Server.prototype[Symbol.asyncDispose] = function () { }; Server.prototype.address = function () { - if (!this[serverSymbol]) return null; + // Node returns null from address() once close() has been called, even + // if draining isn't finished. We keep `this[serverSymbol]` populated + // past `close()` so the msal pattern `close() → closeAllConnections()` + // still reaches the native layer — `kServerClosed` is the "has close + // been called" signal. + if (!this[serverSymbol] || this[kServerClosed]) return null; return this[serverSymbol].address; }; @@ -877,8 +919,21 @@ Server.prototype[kRealListen] = function (tls, port, host, socketPath, reusePort // }, }); - getBunServerAllClosedPromise(this[serverSymbol]).$then(emitCloseNTServer.bind(this)); - isHTTPS = this[serverSymbol].protocol === "https"; + // Reset the "has close been called" flag — Node allows listening + // again after close, so address()/close() behave normally on the + // fresh listen. Done only after `Bun.serve()` succeeds: if it threw + // (e.g. EADDRINUSE) `serverSymbol` still points at the old draining + // handle, and leaving `kServerClosed` set keeps address()/close() + // reporting "not running" until that handle actually drains. + this[kServerClosed] = false; + + // Capture the Bun.serve handle for this listen generation so the + // close callback can tell whether it's still the current one (see + // `emitCloseNTServer`). Without this, a fresh `listen()` that races + // the previous shutdown would have its new handle nulled out. + const bunServer = this[serverSymbol]; + getBunServerAllClosedPromise(bunServer).$then(emitCloseNTServer.bind(this, bunServer)); + isHTTPS = bunServer.protocol === "https"; // always set strict method validation to true for node.js compatibility setServerCustomOptions( this[serverSymbol], @@ -909,7 +964,11 @@ Server.prototype[kRealListen] = function (tls, port, host, socketPath, reusePort Server.prototype.setTimeout = function (msecs, callback) { const server = this[serverSymbol]; - if (server) { + // After close() `serverSymbol` is still populated but `kServerClosed` + // is set; defer the timeout (as when not yet listening) so the next + // listen() replays it onto the fresh server rather than configuring + // the stopped one. + if (server && !this[kServerClosed]) { setServerIdleTimeout(server, Math.ceil(msecs / 1000)); if (typeof callback === "function") this.once("timeout", callback); } else { diff --git a/src/runtime/server/mod.rs b/src/runtime/server/mod.rs index 9a5e5f90b6f4..489886b5564f 100644 --- a/src/runtime/server/mod.rs +++ b/src/runtime/server/mod.rs @@ -1539,6 +1539,28 @@ impl NewServer { } let Some(listener) = self.listener.take() else { + // Upgrading from graceful (`stop(false)`) to abrupt: + // `stop(false)` already took the listener, but the uWS App + // still owns open/idle keep-alive connections that would + // otherwise pin the event loop. `app.close()` force-closes + // every remaining socket on the app. This is the path that + // makes `server.close(); server.closeAllConnections();` + // actually drain msal-style loopback servers. + // + // This runs BEFORE the h3 branch below, because the h3 branch + // flips `TERMINATED` on abrupt stops, which would make our + // `!TERMINATED` guard false and skip the app close for + // h3-enabled servers (HTTPS + h3: true). + if abrupt && !self.flags.contains(ServerFlags::TERMINATED) { + if let Some(app) = self.app { + if let Some(ws) = self.config.websocket.as_mut() { + ws.handler.app = None; + } + self.flags.insert(ServerFlags::TERMINATED); + // S012: `NewApp` is a ZST opaque — safe deref. + bun_opaque::opaque_deref_mut(app).close(); + } + } if Self::HAS_H3 && self.h3_app.is_some() { self.unref(); self.notify_inspector_server_stopped(); diff --git a/src/runtime/server/server_body.rs b/src/runtime/server/server_body.rs index 6182510d5cd2..9d8ba08f0a47 100644 --- a/src/runtime/server/server_body.rs +++ b/src/runtime/server/server_body.rs @@ -2499,23 +2499,39 @@ where pub fn stop_from_js(&mut self, abruptly: Option) -> JSValue { let rc = self.get_all_closed_promise(&self.global()); - if self.has_listener() { - let abrupt = 'brk: { - if let Some(val) = abruptly { - if val.is_boolean() && val.to_boolean() { - break 'brk true; - } + let abrupt = 'brk: { + if let Some(val) = abruptly { + if val.is_boolean() && val.to_boolean() { + break 'brk true; } - false - }; - self.stop(abrupt); + } + false + }; + + // For an abrupt stop, the app may still have open connections + // even after a graceful `stop(false)` cleared `self.listener`. + // `stop_listening` force-closes those via `app.close()` only when + // it actually enters the abrupt branch, so we can't gate on + // `has_listener()` here. `stop`/`stop_listening` are idempotent: + // second calls short-circuit via the `terminated` flag and the + // `deinit_scheduled` flag, so this is safe. + if abrupt { + if self.has_listener() + || (self.app.is_some() && !self.flags.contains(ServerFlags::TERMINATED)) + { + self.stop(true); + } + } else if self.has_listener() { + self.stop(false); } rc } pub fn dispose_from_js(&mut self) -> JSValue { - if self.has_listener() { + if self.has_listener() + || (self.app.is_some() && !self.flags.contains(ServerFlags::TERMINATED)) + { self.stop(true); } JSValue::UNDEFINED diff --git a/test/js/node/http/node-http-close-all-connections.test.ts b/test/js/node/http/node-http-close-all-connections.test.ts new file mode 100644 index 000000000000..e5e4611bbf72 --- /dev/null +++ b/test/js/node/http/node-http-close-all-connections.test.ts @@ -0,0 +1,153 @@ +import { expect, test } from "bun:test"; +import { bunEnv, bunExe } from "harness"; +import { once } from "node:events"; +import http from "node:http"; +import type { AddressInfo } from "node:net"; +import { connect } from "node:net"; + +// Regression: `@azure/msal-node`'s `LoopbackClient.closeServer` calls +// server.close(); +// server.closeAllConnections(); +// server.unref(); +// in sequence. Bun used to null out the internal server reference in +// `close()`, so the subsequent `closeAllConnections()` was a no-op — +// the keep-alive socket kept the event loop alive and the process hung. +// Issue: https://github.com/oven-sh/bun/issues/30501 +test("closeAllConnections() after close() force-closes in-flight sockets", async () => { + const { promise: requestReceived, resolve: resolveReceived } = Promise.withResolvers(); + const server = http.createServer((req, _res) => { + // Signal receipt but DO NOT reply — socket is "in flight" (not idle) + // when the teardown sequence runs. This is the case where close() + // alone (which only closes idle connections) cannot reclaim the + // socket, and closeAllConnections() must do it. + resolveReceived(); + }); + await once(server.listen(0, "127.0.0.1"), "listening"); + const { port } = server.address() as AddressInfo; + + const sock = connect(port, "127.0.0.1"); + const { promise: sockClosed, resolve: resolveClosed } = Promise.withResolvers(); + sock.on("close", () => resolveClosed()); + sock.on("error", () => {}); + await once(sock, "connect"); + sock.write("GET / HTTP/1.1\r\nHost: localhost\r\nConnection: keep-alive\r\n\r\n"); + await requestReceived; + + // msal-style teardown — no waiting between calls + server.close(); + server.closeAllConnections(); + server.unref(); + + // The client socket must be force-closed by closeAllConnections(). + // Without the fix, the socket stays open indefinitely (msal hang). + await sockClosed; +}); + +// Regression: when a caller does close() and then listen() again before +// the previous shutdown's allClosed promise has fulfilled, the stale +// callback must not null out the newly-created server handle. +test("listen() during an in-flight close() doesn't corrupt the new server", async () => { + const server = http.createServer((_req, res) => res.end("ok")); + + await once(server.listen(0, "127.0.0.1"), "listening"); + + // Fire-and-forget close; don't wait for the allClosed callback. + server.close(); + + // Re-listen immediately while the previous shutdown is still settling. + await once(server.listen(0, "127.0.0.1"), "listening"); + const secondAddress = server.address() as AddressInfo | null; + expect(secondAddress).not.toBeNull(); + const secondPort = secondAddress!.port; + expect(secondPort).toBeInteger(); + + // Drain the new server — address() must still return a port after + // microtasks run (this is where the stale close callback would have + // hit, pre-fix). + await new Promise(r => setImmediate(r)); + expect((server.address() as AddressInfo | null)?.port).toBe(secondPort); + + // Confirm the new server actually serves requests, not just has a port. + const res = await fetch(`http://127.0.0.1:${secondPort}`); + expect(await res.text()).toBe("ok"); + + await new Promise(r => server.close(() => r())); +}); + +// Regression: close() now leaves the native handle populated (so +// closeAllConnections()/unref() can still reach it), which means ref() +// must not re-pin the event loop on an already-closed server. A stray +// ref() after close() would otherwise keep the loop alive until GC. +test("ref() after close() does not keep the event loop alive", async () => { + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + ` + const http = require("node:http"); + const server = http.createServer(); + server.listen(0, "127.0.0.1", () => { + server.close(); + // Pre-fix this re-activated the poll ref on a closed server and + // pinned the loop (no listener, no connections) until GC. + server.ref(); + process.stdout.write("REF_DONE\\n"); + }); + `, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + // If ref() re-pinned the loop, the subprocess never exits and + // `proc.exited` never resolves — the runner's timeout catches that. + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stderr).toBe(""); + expect(stdout).toContain("REF_DONE"); + expect(exitCode).toBe(0); +}); + +// End-to-end: spawn a child that opens an HTTP server, accepts a +// keep-alive connection, and calls the msal teardown. Must exit +// immediately — not wait for the keep-alive idle timeout to reclaim +// the in-flight socket. +test("process exits after close() + closeAllConnections() + unref() teardown", async () => { + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + ` + const http = require("node:http"); + const net = require("node:net"); + const server = http.createServer((req, _res) => { + // Never reply — keep the socket in-flight (not idle) so that + // only closeAllConnections() (abrupt) can reclaim it. + server.close(); + server.closeAllConnections(); + server.unref(); + process.stdout.write("TEARDOWN_DONE\\n"); + }); + server.listen(0, "127.0.0.1", () => { + const port = server.address().port; + const sock = net.connect(port, "127.0.0.1", () => { + sock.write("GET / HTTP/1.1\\r\\nHost: localhost\\r\\nConnection: keep-alive\\r\\n\\r\\n"); + }); + sock.on("data", () => {}); + sock.on("error", () => {}); + }); + `, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + // If the teardown path is broken, the subprocess never exits and + // `proc.exited` never resolves — the bun:test runner's default 5s + // timeout catches that. + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + // Surface any uncaught exception / ASAN trace before the exit-code + // assertion so failures point at the real cause. + expect(stderr).toBe(""); + expect(stdout).toContain("TEARDOWN_DONE"); + expect(exitCode).toBe(0); +});