From 80404bec26b5dd6d9bbc2544fb84e098ed331a3e Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 26 Jul 2026 01:09:11 +0000 Subject: [PATCH 1/6] node:http: gate server.close() on connection drain, not request count server.close(cb) was wired to Bun.serve's all-closed promise, whose condition is pending_requests == 0 && !listener && !websockets. A keep-alive connection that was serving a request when close() ran is still open once that request finishes, so the callback fired (and 'close' emitted) while the connection kept accepting requests. close() also nulled this[serverSymbol] immediately, so closeIdleConnections() and closeAllConnections() became no-ops after close(), and closeAllConnections() was a full stop(true) that tore down the listener. Gate emitCloseServer on kTrackedConnections.size (re-checked from the last connection's #onClose), keep the native handle reachable under a kClosing flag until the server has actually drained, and rewrite closeAllConnections()/closeIdleConnections() to iterate kTrackedConnections so they work before and after close() without touching the listener. --- src/js/node/_http_server.ts | 69 ++++- .../test-http-server.listening-should-work.ts | 4 + ...ible-using-kConnectionsCheckingInterval.ts | 5 + test/js/first_party/ws/ws.test.ts | 1 + test/js/node/http/node-http-with-ws.test.ts | 1 + test/js/node/http/node-http.test.ts | 245 ++++++++++++++++++ test/js/web/fetch/client-fetch.test.ts | 2 + test/js/web/fetch/fetch.stream.test.ts | 1 + 8 files changed, 314 insertions(+), 14 deletions(-) diff --git a/src/js/node/_http_server.ts b/src/js/node/_http_server.ts index 282248b5675b..ca90c7be39cc 100644 --- a/src/js/node/_http_server.ts +++ b/src/js/node/_http_server.ts @@ -86,6 +86,8 @@ const OutgoingMessagePrototype = OutgoingMessage.prototype; const { kIncomingMessage } = require("node:_http_common"); const kConnectionsCheckingInterval = Symbol("http.server.connectionsCheckingInterval"); const kTrackedConnections = Symbol("http.server.trackedConnections"); +const kClosing = Symbol("http.server.closing"); +const kPendingDrainClose = Symbol("http.server.pendingDrainClose"); const kHttpAllowHalfOpen = Symbol("http.server.httpAllowHalfOpen"); // node.http trace events ('http.server.request' b/e). The agent module is @@ -120,6 +122,20 @@ const DateNow = Date.now; let cluster; function emitCloseServer(self: Server) { + // Like Node.js's net.Server#_emitCloseIfDrained: 'close' (and the close + // callback) only fire once every accepted connection has ended. The native + // all-closed promise that schedules this resolves on pending_requests == 0, + // so a keep-alive connection that was serving a request when close() ran is + // still open when that promise resolves; the last connection's #onClose + // re-checks and reschedules once kTrackedConnections empties. + const connections = self[kTrackedConnections]; + if (connections && connections.size > 0) { + self[kPendingDrainClose] = true; + return; + } + self[kPendingDrainClose] = false; + self[kClosing] = false; + self[serverSymbol] = undefined; callCloseCallback(self); self.emit("close"); } @@ -309,6 +325,8 @@ function Server(options, callback): void { defineHttpAllowHalfOpen(this); this[kInternalSocketData] = undefined; this[kTrackedConnections] = new Set(); + this[kClosing] = false; + this[kPendingDrainClose] = false; this[tlsSymbol] = null; this.noDelay = true; if (typeof options === "function") { @@ -472,15 +490,14 @@ Server.prototype.unref = function () { }; Server.prototype.closeAllConnections = function () { - const server = this[serverSymbol]; - if (!server) { - return; + // Node destroys every tracked connection and leaves the listen socket alone. + // Driven off kTrackedConnections so it also works as the forced half of a + // close() + closeAllConnections() drain, once the native handle is gone. + const connections = this[kTrackedConnections]; + if (!connections) return; + for (const socket of connections) { + socket.destroy(); } - this[serverSymbol] = undefined; - clearInterval(this[kConnectionsCheckingInterval]); - this.listening = false; - - server.stop(true); }; Server.prototype.getConnections = function (callback) { @@ -494,8 +511,22 @@ Server.prototype.getConnections = function (callback) { }; Server.prototype.closeIdleConnections = function () { - const server = this[serverSymbol]; - server?.closeIdleConnections(); + // Like Node.js: destroy connections that are neither writing a response nor + // receiving a request, and leave the listen socket alone. The native sweep + // skips connections with a partially-parsed request (its isIdle flag); the + // kTrackedConnections pass is what keeps this working after close(), once + // the native app has deinit'd. + this[serverSymbol]?.closeIdleConnections(); + const connections = this[kTrackedConnections]; + if (!connections) return; + for (const socket of connections) { + if (socket.destroyed) continue; + const message = socket._httpMessage; + if (message && !message.finished) continue; + if (socket[kPipelinedResponses]?.length) continue; + if (socket[kHandle]?.response) continue; + socket.destroy(); + } }; Server.prototype.close = function (optionalCallback?) { @@ -503,12 +534,15 @@ 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[kClosing]) { if (typeof optionalCallback === "function") process.nextTick(optionalCallback, $ERR_SERVER_NOT_RUNNING()); // Like Node.js's net.Server#close, close() returns the server. return this; } - this[serverSymbol] = undefined; + // The native handle stays reachable until every connection has drained + // (emitCloseServer clears it) so closeIdleConnections/closeAllConnections + // keep working after close(); kClosing is Node's "_handle == null" stand-in. + this[kClosing] = true; if (typeof optionalCallback === "function") setCloseCallback(this, optionalCallback); this.listening = false; server.closeIdleConnections(); @@ -553,7 +587,7 @@ Server.prototype[Symbol.asyncDispose] = function () { }; Server.prototype.address = function () { - if (!this[serverSymbol]) return null; + if (!this[serverSymbol] || this[kClosing]) return null; return this[serverSymbol].address; }; @@ -1667,7 +1701,14 @@ const NodeHTTPServerSocket = class Socket extends NetSocket { // released parser (free() invoked, kOnTimeout nulled). releaseServerParserShim(this); this[kHandle] = null; - this.server?.[kTrackedConnections]?.delete(this); + const server = this.server; + const tracked = server?.[kTrackedConnections]; + if (tracked) { + tracked.delete(this); + if (tracked.size === 0 && server[kPendingDrainClose]) { + process.nextTick(emitCloseServer, server); + } + } const timer = this[kSocketTimeoutTimer]; if (timer) { clearTimeout(timer); diff --git a/test/js/bun/test/parallel/test-http-server.listening-should-work.ts b/test/js/bun/test/parallel/test-http-server.listening-should-work.ts index 8f9e565558b8..872e9d1cd87e 100644 --- a/test/js/bun/test/parallel/test-http-server.listening-should-work.ts +++ b/test/js/bun/test/parallel/test-http-server.listening-should-work.ts @@ -6,5 +6,9 @@ const { expect } = createTest(import.meta.path); const server = http.createServer(); await once(server.listen(0), "listening"); expect(server.listening).toBe(true); +// closeAllConnections() destroys the connections, it does not stop listening. server.closeAllConnections(); +expect(server.listening).toBe(true); +server.close(); expect(server.listening).toBe(false); +await once(server, "close"); diff --git a/test/js/bun/test/parallel/test-http-timeout-destruction-should-be-visible-using-kConnectionsCheckingInterval.ts b/test/js/bun/test/parallel/test-http-timeout-destruction-should-be-visible-using-kConnectionsCheckingInterval.ts index 70ac11ae6895..4c73ba57a6ea 100644 --- a/test/js/bun/test/parallel/test-http-timeout-destruction-should-be-visible-using-kConnectionsCheckingInterval.ts +++ b/test/js/bun/test/parallel/test-http-timeout-destruction-should-be-visible-using-kConnectionsCheckingInterval.ts @@ -6,5 +6,10 @@ const { expect } = createTest(import.meta.path); const { kConnectionsCheckingInterval } = require("_http_server"); const server = http.createServer(); await once(server.listen(0), "listening"); +expect(server[kConnectionsCheckingInterval]._destroyed).toBe(false); +// Only close() tears the interval down; closeAllConnections() keeps listening. server.closeAllConnections(); +expect(server[kConnectionsCheckingInterval]._destroyed).toBe(false); +server.close(); expect(server[kConnectionsCheckingInterval]._destroyed).toBe(true); +await once(server, "close"); diff --git a/test/js/first_party/ws/ws.test.ts b/test/js/first_party/ws/ws.test.ts index 060bc7a1c917..5aa40fcd3363 100644 --- a/test/js/first_party/ws/ws.test.ts +++ b/test/js/first_party/ws/ws.test.ts @@ -771,6 +771,7 @@ it("Server should be able to send empty pings", async () => { return await promise; } finally { httpServer.closeAllConnections(); + httpServer.close(); } } { diff --git a/test/js/node/http/node-http-with-ws.test.ts b/test/js/node/http/node-http-with-ws.test.ts index a3ef8cac6a29..b64787a6b9a4 100644 --- a/test/js/node/http/node-http-with-ws.test.ts +++ b/test/js/node/http/node-http-with-ws.test.ts @@ -94,6 +94,7 @@ test.concurrent("should not crash when closing sockets after upgrade", async () http_socket?.destroy(); }); server.closeAllConnections(); + server.close(); resolve(); }, 10); } diff --git a/test/js/node/http/node-http.test.ts b/test/js/node/http/node-http.test.ts index a204e6d37259..f1e67dd0a573 100644 --- a/test/js/node/http/node-http.test.ts +++ b/test/js/node/http/node-http.test.ts @@ -1543,6 +1543,7 @@ describe("HTTP Server Security Tests - Advanced", () => { // Close the server if it's still running if (server.listening) { server.closeAllConnections(); + server.close(); } }); @@ -3467,6 +3468,250 @@ it("server.close(cb) completes after a raw upgrade once both sockets are destroy await closed; }); +// Node's server.close(cb) waits for every accepted connection to end +// (net.Server#_emitCloseIfDrained), not for the in-flight request count to +// reach zero. These four scenarios cover graceful-shutdown shapes that +// otherwise look identical to the "pending requests == 0" condition. +describe("server.close() drains connections, not requests", () => { + async function startServer(handler: http.RequestListener) { + const server = createServer(handler); + server.keepAliveTimeout = 60_000; + server.listen(0, "127.0.0.1"); + await once(server, "listening"); + const { port } = server.address() as AddressInfo; + const sock = connect(port, "127.0.0.1"); + sock.on("error", () => {}); + await once(sock, "connect"); + return { server, sock, port }; + } + + async function drainTicks() { + for (let i = 0; i < 6; i++) await new Promise(r => setImmediate(r)); + } + + it("A: close() FINs an idle keep-alive connection and fires", async () => { + const { server, sock } = await startServer((req, res) => res.end("ok:" + req.url)); + try { + let body = ""; + sock.on("data", c => (body += c)); + sock.write("GET /a HTTP/1.1\r\nHost: x\r\n\r\n"); + while (!body.includes("ok:/a")) await once(sock, "data"); + await drainTicks(); + + const ended = once(sock, "end"); + const closed = Promise.withResolvers(); + server.close(() => closed.resolve()); + await closed.promise; + await ended; + } finally { + sock.destroy(); + server.closeAllConnections(); + } + }); + + it("C: close(cb) waits while a keep-alive connection is still open", async () => { + const inHandler = Promise.withResolvers(); + let finishFirst!: () => void; + const paths: string[] = []; + const { server, sock } = await startServer((req, res) => { + paths.push(req.url as string); + if (paths.length === 1) { + inHandler.resolve(); + finishFirst = () => res.end("ok:" + req.url); + } else { + res.end("ok:" + req.url); + } + }); + try { + let body = ""; + sock.on("data", c => (body += c)); + sock.write("GET /first HTTP/1.1\r\nHost: x\r\n\r\n"); + await inHandler.promise; + + let closeCbFired = false; + let closeEventFired = false; + server.once("close", () => (closeEventFired = true)); + const closed = Promise.withResolvers(); + server.close(() => { + closeCbFired = true; + closed.resolve(); + }); + + finishFirst(); + while (!body.includes("ok:/first")) await once(sock, "data"); + await drainTicks(); + expect({ closeCbFired, closeEventFired }).toEqual({ closeCbFired: false, closeEventFired: false }); + + sock.write("GET /second HTTP/1.1\r\nHost: x\r\n\r\n"); + while (!body.includes("ok:/second")) await once(sock, "data"); + await drainTicks(); + expect({ closeCbFired, closeEventFired }).toEqual({ closeCbFired: false, closeEventFired: false }); + expect(paths).toEqual(["/first", "/second"]); + + sock.destroy(); + await closed.promise; + expect({ closeCbFired, closeEventFired }).toEqual({ closeCbFired: true, closeEventFired: true }); + } finally { + sock.destroy(); + server.closeAllConnections(); + } + }); + + it("B: closeIdleConnections() after close() drains a now-idle connection", async () => { + const inHandler = Promise.withResolvers(); + let finishFirst!: () => void; + const { server, sock } = await startServer((req, res) => { + inHandler.resolve(); + finishFirst = () => res.end("ok:" + req.url); + }); + try { + let body = ""; + sock.on("data", c => (body += c)); + const ended = once(sock, "end"); + sock.write("GET /b HTTP/1.1\r\nHost: x\r\n\r\n"); + await inHandler.promise; + + let closeCbFired = false; + const closed = Promise.withResolvers(); + server.close(() => { + closeCbFired = true; + closed.resolve(); + }); + + finishFirst(); + while (!body.includes("ok:/b")) await once(sock, "data"); + await drainTicks(); + expect(closeCbFired).toBe(false); + + // The response has been delivered and the connection is idle; a + // post-close closeIdleConnections() must reach it and let the + // callback fire. + server.closeIdleConnections(); + await closed.promise; + await ended; + } finally { + sock.destroy(); + server.closeAllConnections(); + } + }); + + it("B': closeAllConnections() after close() drains the in-flight connection", async () => { + const inHandler = Promise.withResolvers(); + const { server, sock } = await startServer((req, res) => { + inHandler.resolve(); + void res; + }); + try { + sock.write("GET /b2 HTTP/1.1\r\nHost: x\r\n\r\n"); + await inHandler.promise; + + const closed = Promise.withResolvers(); + server.close(() => closed.resolve()); + await drainTicks(); + + // The request handler never responded; closeAllConnections() must + // destroy the connection regardless and let the callback fire. + server.closeAllConnections(); + await closed.promise; + expect(sock.destroyed || sock.readableEnded).toBe(true); + } finally { + sock.destroy(); + server.closeAllConnections(); + } + }); + + it("D: close(cb) never fires while a keep-alive client keeps the connection busy", async () => { + // SIGTERM shape: close() arrives while a request is in flight, the client + // then keeps issuing keep-alive requests on that same connection. The + // close callback must not fire (and no request is served "after cb") + // until the client releases the connection. + const inHandler = Promise.withResolvers(); + let finishFirst!: () => void; + const paths: string[] = []; + const { server, sock } = await startServer((req, res) => { + paths.push(req.url as string); + if (paths.length === 1) { + inHandler.resolve(); + finishFirst = () => res.end("ok:" + req.url); + } else { + res.end("ok:" + req.url); + } + }); + try { + let body = ""; + sock.on("data", c => (body += c)); + let servedAfterCb = 0; + let closeCbFired = false; + + sock.write("GET /d0 HTTP/1.1\r\nHost: x\r\n\r\n"); + await inHandler.promise; + + const closed = Promise.withResolvers(); + server.close(() => { + closeCbFired = true; + closed.resolve(); + }); + finishFirst(); + while (!body.includes("ok:/d0")) await once(sock, "data"); + await drainTicks(); + if (closeCbFired) servedAfterCb = -1; + + for (let i = 1; i <= 5; i++) { + const marker = "ok:/d" + i; + sock.write(`GET /d${i} HTTP/1.1\r\nHost: x\r\n\r\n`); + while (!body.includes(marker)) await once(sock, "data"); + if (closeCbFired) servedAfterCb++; + await drainTicks(); + } + expect({ closeCbFired, servedAfterCb }).toEqual({ closeCbFired: false, servedAfterCb: 0 }); + + sock.destroy(); + await closed.promise; + expect(paths).toEqual(["/d0", "/d1", "/d2", "/d3", "/d4", "/d5"]); + } finally { + sock.destroy(); + server.closeAllConnections(); + } + }); + + it("closeAllConnections() leaves the listener running", async () => { + const { server, sock, port } = await startServer((req, res) => res.end("ok:" + req.url)); + try { + let body = ""; + sock.on("data", c => (body += c)); + sock.write("GET /l HTTP/1.1\r\nHost: x\r\n\r\n"); + while (!body.includes("ok:/l")) await once(sock, "data"); + + let closeEventFired = false; + server.once("close", () => (closeEventFired = true)); + const clientClosed = once(sock, "close"); + server.closeAllConnections(); + await clientClosed; + await drainTicks(); + expect(server.listening).toBe(true); + expect(closeEventFired).toBe(false); + + // A fresh connection is still accepted. + const sock2 = connect(port, "127.0.0.1"); + sock2.on("error", () => {}); + await once(sock2, "connect"); + let body2 = ""; + sock2.on("data", c => (body2 += c)); + sock2.write("GET /l2 HTTP/1.1\r\nHost: x\r\n\r\n"); + while (!body2.includes("ok:/l2")) await once(sock2, "data"); + sock2.destroy(); + + const closed = Promise.withResolvers(); + server.close(() => closed.resolve()); + await closed.promise; + } finally { + sock.destroy(); + server.closeAllConnections(); + server.close(); + } + }); +}); + it("req.upgrade is true inside the 'connect' listener", async () => { let upgradeValue: unknown = "unset"; const { promise: sawConnect, resolve: onConnect } = Promise.withResolvers(); diff --git a/test/js/web/fetch/client-fetch.test.ts b/test/js/web/fetch/client-fetch.test.ts index 37cf159bbc09..2b90d8a2c14c 100644 --- a/test/js/web/fetch/client-fetch.test.ts +++ b/test/js/web/fetch/client-fetch.test.ts @@ -85,6 +85,7 @@ test("pre aborted with readable request body", async () => { ).rejects.toThrow(); } finally { server.closeAllConnections(); + server.close(); } }); @@ -559,6 +560,7 @@ test("fetching with Request object - issue #1527", async () => { expect(await fetch(request)).resolves.pass(); } finally { server.closeAllConnections(); + server.close(); } }); diff --git a/test/js/web/fetch/fetch.stream.test.ts b/test/js/web/fetch/fetch.stream.test.ts index ca7ee6af46dd..181d610077fb 100644 --- a/test/js/web/fetch/fetch.stream.test.ts +++ b/test/js/web/fetch/fetch.stream.test.ts @@ -243,6 +243,7 @@ describe.concurrent("fetch() with streaming", () => { expect(true).toBe(true); } finally { server?.closeAllConnections(); + server?.close(); } }); } From e0f88d568f7d46e7f4b6b3c43058d3556b672a5c Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 26 Jul 2026 01:52:44 +0000 Subject: [PATCH 2/6] gate closeIdleConnections Set sweep on kClosing; cancel drain on re-listen The kTrackedConnections pass in closeIdleConnections() treated a connection whose request head is still arriving as idle (no _httpMessage yet). On a live server the native isIdle sweep already spares it, so restrict the Set pass to the kClosing window it exists for. Reset kClosing/kPendingDrainClose in kRealListen and bail in emitCloseServer when !kClosing so a listen() during a prior close()'s drain does not have its new handle cleared by the stale drain, and gate emitListeningNextTick on !kClosing for the same _handle-stand-in consistency as address(). --- src/js/node/_http_server.ts | 29 ++++++++++++++-------- test/js/node/http/node-http.test.ts | 38 +++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 10 deletions(-) diff --git a/src/js/node/_http_server.ts b/src/js/node/_http_server.ts index ca90c7be39cc..4a9da8b66fdc 100644 --- a/src/js/node/_http_server.ts +++ b/src/js/node/_http_server.ts @@ -123,11 +123,14 @@ let cluster; function emitCloseServer(self: Server) { // Like Node.js's net.Server#_emitCloseIfDrained: 'close' (and the close - // callback) only fire once every accepted connection has ended. The native - // all-closed promise that schedules this resolves on pending_requests == 0, - // so a keep-alive connection that was serving a request when close() ran is - // still open when that promise resolves; the last connection's #onClose - // re-checks and reschedules once kTrackedConnections empties. + // callback) only fire once every accepted connection has ended, and a + // re-listen before the drain completes cancels it (Node bails when + // `_handle` is set again). The native all-closed promise that schedules + // this resolves on pending_requests == 0, so a keep-alive connection that + // was serving a request when close() ran is still open when that promise + // resolves; the last connection's #onClose re-checks and reschedules once + // kTrackedConnections empties. + if (!self[kClosing]) return; const connections = self[kTrackedConnections]; if (connections && connections.size > 0) { self[kPendingDrainClose] = true; @@ -292,7 +295,7 @@ function emitRequestCloseNT(self) { } function emitListeningNextTick(self, hostname, port) { - if ((self.listening = !!self[serverSymbol])) { + if ((self.listening = !!self[serverSymbol] && !self[kClosing])) { // TODO: remove the arguments // Note does not pass any arguments. self.emit("listening", null, hostname, port); @@ -512,11 +515,13 @@ Server.prototype.getConnections = function (callback) { Server.prototype.closeIdleConnections = function () { // Like Node.js: destroy connections that are neither writing a response nor - // receiving a request, and leave the listen socket alone. The native sweep - // skips connections with a partially-parsed request (its isIdle flag); the - // kTrackedConnections pass is what keeps this working after close(), once - // the native app has deinit'd. + // receiving a request, and leave the listen socket alone. On a live server + // the native sweep is authoritative (its isIdle flag spares connections + // whose request head is still arriving); the kTrackedConnections pass runs + // only during a close() drain, so it keeps working once the native app has + // deinit'd without undoing that decision on a live server. this[serverSymbol]?.closeIdleConnections(); + if (!this[kClosing]) return; const connections = this[kTrackedConnections]; if (!connections) return; for (const socket of connections) { @@ -705,6 +710,10 @@ Server.prototype[kRealListen] = function (tls, port, host, socketPath, reusePort if (tls) { this.serverName = tls.serverName || host || "localhost"; } + // A listen() during a prior close()'s drain installs a fresh handle; the + // pending drain must not clear it (emitCloseServer bails on !kClosing). + this[kClosing] = false; + this[kPendingDrainClose] = false; this[serverSymbol] = Bun.serve({ idleTimeout: 0, // nodejs dont have a idleTimeout by default tls, diff --git a/test/js/node/http/node-http.test.ts b/test/js/node/http/node-http.test.ts index f1e67dd0a573..e41eb3b0d236 100644 --- a/test/js/node/http/node-http.test.ts +++ b/test/js/node/http/node-http.test.ts @@ -3710,6 +3710,44 @@ describe("server.close() drains connections, not requests", () => { server.close(); } }); + + it("closeIdleConnections() on a live server spares a connection mid-header-parse", async () => { + // Node's ConnectionsList.idle() excludes parsers with last_message_start != 0 + // (test-http-server-close-idle.js client1). The JS kTrackedConnections sweep + // would treat such a connection as idle, so it must not run on a live server. + const server = createServer((req, res) => res.end("ok:" + req.url)); + server.keepAliveTimeout = 60_000; + server.listen(0, "127.0.0.1"); + await once(server, "listening"); + const { port } = server.address() as AddressInfo; + const connected = once(server, "connection"); + const sock = connect(port, "127.0.0.1"); + sock.on("error", () => {}); + await once(sock, "connect"); + try { + let ended = false; + sock.on("end", () => (ended = true)); + sock.write("GET /p HTTP/1.1"); + await connected; + await drainTicks(); + + server.closeIdleConnections(); + await drainTicks(); + expect({ socketDestroyed: sock.destroyed, socketEnded: ended }).toEqual({ + socketDestroyed: false, + socketEnded: false, + }); + + let body = ""; + sock.on("data", c => (body += c)); + sock.write("\r\nHost: x\r\n\r\n"); + while (!body.includes("ok:/p")) await once(sock, "data"); + } finally { + sock.destroy(); + server.closeAllConnections(); + server.close(); + } + }); }); it("req.upgrade is true inside the 'connect' listener", async () => { From 375afa296a40740a228549fc63e572ac85cc14fa Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 26 Jul 2026 01:56:00 +0000 Subject: [PATCH 3/6] tighten drain-path comments --- src/js/node/_http_server.ts | 32 ++++++++++---------------------- 1 file changed, 10 insertions(+), 22 deletions(-) diff --git a/src/js/node/_http_server.ts b/src/js/node/_http_server.ts index 4a9da8b66fdc..27f4cfe14a19 100644 --- a/src/js/node/_http_server.ts +++ b/src/js/node/_http_server.ts @@ -121,15 +121,10 @@ const DateNow = Date.now; let cluster; +// Like Node.js's net.Server#_emitCloseIfDrained: the native all-closed promise +// resolves on pending_requests == 0 (not connections == 0), so gate the actual +// 'close' on kTrackedConnections draining; #onClose reschedules once it does. function emitCloseServer(self: Server) { - // Like Node.js's net.Server#_emitCloseIfDrained: 'close' (and the close - // callback) only fire once every accepted connection has ended, and a - // re-listen before the drain completes cancels it (Node bails when - // `_handle` is set again). The native all-closed promise that schedules - // this resolves on pending_requests == 0, so a keep-alive connection that - // was serving a request when close() ran is still open when that promise - // resolves; the last connection's #onClose re-checks and reschedules once - // kTrackedConnections empties. if (!self[kClosing]) return; const connections = self[kTrackedConnections]; if (connections && connections.size > 0) { @@ -492,10 +487,8 @@ Server.prototype.unref = function () { return this; }; +// Node destroys every tracked connection and leaves the listen socket alone. Server.prototype.closeAllConnections = function () { - // Node destroys every tracked connection and leaves the listen socket alone. - // Driven off kTrackedConnections so it also works as the forced half of a - // close() + closeAllConnections() drain, once the native handle is gone. const connections = this[kTrackedConnections]; if (!connections) return; for (const socket of connections) { @@ -514,12 +507,9 @@ Server.prototype.getConnections = function (callback) { }; Server.prototype.closeIdleConnections = function () { - // Like Node.js: destroy connections that are neither writing a response nor - // receiving a request, and leave the listen socket alone. On a live server - // the native sweep is authoritative (its isIdle flag spares connections - // whose request head is still arriving); the kTrackedConnections pass runs - // only during a close() drain, so it keeps working once the native app has - // deinit'd without undoing that decision on a live server. + // Native sweep is authoritative on a live server (its isIdle flag spares + // mid-parse connections); the kTrackedConnections pass covers the + // post-close() window once the native app has deinit'd. this[serverSymbol]?.closeIdleConnections(); if (!this[kClosing]) return; const connections = this[kTrackedConnections]; @@ -544,9 +534,8 @@ Server.prototype.close = function (optionalCallback?) { // Like Node.js's net.Server#close, close() returns the server. return this; } - // The native handle stays reachable until every connection has drained - // (emitCloseServer clears it) so closeIdleConnections/closeAllConnections - // keep working after close(); kClosing is Node's "_handle == null" stand-in. + // kClosing is Node's "_handle == null" stand-in; serverSymbol stays set + // until emitCloseServer so the drain helpers keep working after close(). this[kClosing] = true; if (typeof optionalCallback === "function") setCloseCallback(this, optionalCallback); this.listening = false; @@ -710,8 +699,7 @@ Server.prototype[kRealListen] = function (tls, port, host, socketPath, reusePort if (tls) { this.serverName = tls.serverName || host || "localhost"; } - // A listen() during a prior close()'s drain installs a fresh handle; the - // pending drain must not clear it (emitCloseServer bails on !kClosing). + // Cancel any prior close()'s pending drain before installing a new handle. this[kClosing] = false; this[kPendingDrainClose] = false; this[serverSymbol] = Bun.serve({ From 0a908e4c0003a421e3517a11ff4622d5a33ef8e6 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 26 Jul 2026 02:21:53 +0000 Subject: [PATCH 4/6] clear kCloseCallback on re-listen; add server.close() to drain-test finally blocks The re-listen reset cleared kClosing/kPendingDrainClose but not kCloseCallback, so close(cb1); listen(); close(cb2) hit setCloseCallback's 'Close callback already set' throw. The new drain tests' finally blocks only called closeAllConnections(), which no longer stops the listener; add server.close() so a failure before the happy-path close() does not leak the listener. --- src/js/node/_http_server.ts | 1 + test/js/node/http/node-http.test.ts | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/src/js/node/_http_server.ts b/src/js/node/_http_server.ts index 27f4cfe14a19..4b7b3c94db64 100644 --- a/src/js/node/_http_server.ts +++ b/src/js/node/_http_server.ts @@ -702,6 +702,7 @@ Server.prototype[kRealListen] = function (tls, port, host, socketPath, reusePort // Cancel any prior close()'s pending drain before installing a new handle. this[kClosing] = false; this[kPendingDrainClose] = false; + this[kCloseCallback] = undefined; this[serverSymbol] = Bun.serve({ idleTimeout: 0, // nodejs dont have a idleTimeout by default tls, diff --git a/test/js/node/http/node-http.test.ts b/test/js/node/http/node-http.test.ts index e41eb3b0d236..9945cd62e327 100644 --- a/test/js/node/http/node-http.test.ts +++ b/test/js/node/http/node-http.test.ts @@ -3506,6 +3506,7 @@ describe("server.close() drains connections, not requests", () => { } finally { sock.destroy(); server.closeAllConnections(); + server.close(); } }); @@ -3554,6 +3555,7 @@ describe("server.close() drains connections, not requests", () => { } finally { sock.destroy(); server.closeAllConnections(); + server.close(); } }); @@ -3592,6 +3594,7 @@ describe("server.close() drains connections, not requests", () => { } finally { sock.destroy(); server.closeAllConnections(); + server.close(); } }); @@ -3617,6 +3620,7 @@ describe("server.close() drains connections, not requests", () => { } finally { sock.destroy(); server.closeAllConnections(); + server.close(); } }); @@ -3671,6 +3675,7 @@ describe("server.close() drains connections, not requests", () => { } finally { sock.destroy(); server.closeAllConnections(); + server.close(); } }); From c08104856f78efd705ec9906d21b7b7a2e750b79 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 26 Jul 2026 03:13:56 +0000 Subject: [PATCH 5/6] clear serverSymbol in kRealListen reset so a throwing Bun.serve() leaves no stale handle --- src/js/node/_http_server.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/js/node/_http_server.ts b/src/js/node/_http_server.ts index 4b7b3c94db64..eb67665e849f 100644 --- a/src/js/node/_http_server.ts +++ b/src/js/node/_http_server.ts @@ -703,6 +703,7 @@ Server.prototype[kRealListen] = function (tls, port, host, socketPath, reusePort this[kClosing] = false; this[kPendingDrainClose] = false; this[kCloseCallback] = undefined; + this[serverSymbol] = undefined; this[serverSymbol] = Bun.serve({ idleTimeout: 0, // nodejs dont have a idleTimeout by default tls, From 768edc06dfaac6325c7469b68635e3222a8aec3d Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 26 Jul 2026 03:41:13 +0000 Subject: [PATCH 6/6] commit re-listen drain reset only after Bun.serve() succeeds Clearing serverSymbol before the fallible Bun.serve() call orphaned a live listener in the listen() -> listen(fails) path. Move the drain-state reset to after the assignment so a throw leaves the server in exactly its pre-listen() state. --- src/js/node/_http_server.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/js/node/_http_server.ts b/src/js/node/_http_server.ts index eb67665e849f..37f6cbb0adc9 100644 --- a/src/js/node/_http_server.ts +++ b/src/js/node/_http_server.ts @@ -699,11 +699,6 @@ Server.prototype[kRealListen] = function (tls, port, host, socketPath, reusePort if (tls) { this.serverName = tls.serverName || host || "localhost"; } - // Cancel any prior close()'s pending drain before installing a new handle. - this[kClosing] = false; - this[kPendingDrainClose] = false; - this[kCloseCallback] = undefined; - this[serverSymbol] = undefined; this[serverSymbol] = Bun.serve({ idleTimeout: 0, // nodejs dont have a idleTimeout by default tls, @@ -1181,6 +1176,11 @@ Server.prototype[kRealListen] = function (tls, port, host, socketPath, reusePort // }, }); + // Cancel any prior close()'s pending drain now that the new handle is + // installed (after the fallible Bun.serve() so a throw leaves state intact). + this[kClosing] = false; + this[kPendingDrainClose] = false; + this[kCloseCallback] = undefined; getBunServerAllClosedPromise(this[serverSymbol]).$then(emitCloseNTServer.bind(this)); isHTTPS = this[serverSymbol].protocol === "https"; applyServerCustomOptions(this);