Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion src/js/node/_http_server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@
const { kIncomingMessage } = require("node:_http_common");
const kConnectionsCheckingInterval = Symbol("http.server.connectionsCheckingInterval");
const kTrackedConnections = Symbol("http.server.trackedConnections");
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
Expand Down Expand Up @@ -120,6 +121,16 @@
let cluster;

function emitCloseServer(self: Server) {
// Like Node.js's net.Server#_emitCloseIfDrained: 'close' waits for every
// accepted connection to end. The native all-closed promise 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.
Comment thread
robobun marked this conversation as resolved.
Outdated
const connections = self[kTrackedConnections];
if (connections && connections.size > 0) {
self[kPendingDrainClose] = true;
return;
}

Check failure on line 132 in src/js/node/_http_server.ts

View check run for this annotation

Claude / Claude Code Review

close(cb) can now stall indefinitely because closeAllConnections()/closeIdleConnections() are no-ops after close()

Deferring `'close'` until `kTrackedConnections` drains is correct, but `close()` sets `this[serverSymbol] = undefined` (line 523), which makes `closeAllConnections()` (line 488) and `closeIdleConnections()` (line 510) no-ops. So the documented Node deadline pattern — `server.close(cb); setTimeout(() => server.closeAllConnections(), N)` — now hangs where pre-PR it (incorrectly, but terminatingly) fired `cb` early. When `serverSymbol` is already cleared, both methods should fall back to iterating
Comment thread
robobun marked this conversation as resolved.
self[kPendingDrainClose] = false;

Check failure on line 133 in src/js/node/_http_server.ts

View check run for this annotation

Claude / Claude Code Review

kPendingDrainClose is not reset on re-listen; stale flag can fire 'close' on a listening server

`kPendingDrainClose` is set to `true` here but never reset in `listen()`/`kRealListen`, and `emitCloseServer` doesn't check `self[serverSymbol]`. If the user re-listens while the flag is armed, the old keep-alive connection's `#onClose` will schedule `emitCloseServer` and fire `'close'` (plus the stored close callback) on an actively-listening server. Node's `_emitCloseIfDrained` — which the comment says this mirrors — guards this with `if (this._handle || this._connections) return`; add the equ
Comment thread
robobun marked this conversation as resolved.
callCloseCallback(self);
self.emit("close");
}
Expand Down Expand Up @@ -309,6 +320,7 @@
defineHttpAllowHalfOpen(this);
this[kInternalSocketData] = undefined;
this[kTrackedConnections] = new Set();
this[kPendingDrainClose] = false;
this[tlsSymbol] = null;
this.noDelay = true;
if (typeof options === "function") {
Expand Down Expand Up @@ -1667,7 +1679,14 @@
// 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);
Expand Down
71 changes: 71 additions & 0 deletions test/js/node/http/node-http.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3467,6 +3467,77 @@ it("server.close(cb) completes after a raw upgrade once both sockets are destroy
await closed;
});

it("server.close(cb) does not fire while a keep-alive connection is still open", async () => {
// Node's net.Server#close callback (and the 'close' event) only fires once
// every accepted connection has ended. A connection that was mid-request
// when close() ran stays open after the response is delivered, so the
// callback must be withheld until the client closes the socket.
const inHandler = Promise.withResolvers<void>();
let releaseResponse!: () => void;
const paths: string[] = [];
const server = createServer((req, res) => {
paths.push(req.url as string);
if (paths.length === 1) {
inHandler.resolve();
releaseResponse = () => res.end("resp:" + req.url);
} else {
res.end("resp:" + req.url);
}
});
server.keepAliveTimeout = 60000;
server.listen(0, "127.0.0.1");
await once(server, "listening");
const { port } = server.address() as AddressInfo;

const socket = connect(port, "127.0.0.1");
try {
await once(socket, "connect");
let body = "";
socket.on("data", chunk => (body += chunk));
socket.on("error", () => {});

// First request: handler is entered but response held until after close().
socket.write("GET /first HTTP/1.1\r\nHost: x\r\n\r\n");
await inHandler.promise;

let closeEventFired = false;
server.once("close", () => (closeEventFired = true));
const closed = Promise.withResolvers<void>();
let closeCbFired = false;
server.close(() => {
closeCbFired = true;
closed.resolve();
});

// Handler finishes: response is delivered, connection stays open.
// Yield a few event-loop turns after the bytes arrive so the server's
// "all requests done" task chain has run before the callback is checked.
releaseResponse();
while (!body.includes("resp:/first")) await once(socket, "data");
for (let i = 0; i < 4; i++) await new Promise<void>(r => setImmediate(r));
expect(closeCbFired).toBe(false);
expect(closeEventFired).toBe(false);

// A second request on the same connection is still served (matching Node),
// and the close callback must still be withheld afterwards.
socket.write("GET /second HTTP/1.1\r\nHost: x\r\n\r\n");
while (!body.includes("resp:/second")) await once(socket, "data");
for (let i = 0; i < 4; i++) await new Promise<void>(r => setImmediate(r));
expect(closeCbFired).toBe(false);
expect(closeEventFired).toBe(false);
expect(paths).toEqual(["/first", "/second"]);

// Closing the connection drains the server and releases the callback.
socket.destroy();
await closed.promise;
expect(closeCbFired).toBe(true);
expect(closeEventFired).toBe(true);
} finally {
socket.destroy();
server.closeAllConnections();
}
});

it("req.upgrade is true inside the 'connect' listener", async () => {
let upgradeValue: unknown = "unset";
const { promise: sawConnect, resolve: onConnect } = Promise.withResolvers<void>();
Expand Down
Loading