Skip to content
Closed
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
18 changes: 18 additions & 0 deletions src/js/node/_http_server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1217,6 +1217,24 @@ function onServerConnection(this: Server, socketHandle) {
// Already wrapped (shouldn't happen for a brand-new connection).
return;
}
// Like Node.js's net.Server onconnection: once maxConnections is reached,
// each further accepted connection is reported via 'drop' and destroyed
// before http's connectionListener sees it.
const maxConnections = this.maxConnections;
if (maxConnections != null && this[kTrackedConnections].size >= maxConnections) {
const remote = socketHandle.remoteAddress;
const local = socketHandle.localAddress;
this.emit("drop", {
localAddress: local?.address,
localPort: local?.port,
localFamily: local?.family,
remoteAddress: remote?.address,
remotePort: remote?.port,
remoteFamily: remote?.family,
});
socketHandle.close();
return;
Comment thread
robobun marked this conversation as resolved.
Resolved
}
const isTLS = !!this[tlsSymbol];
const socket = new NodeHTTPServerSocket(this, socketHandle, isTLS);
// Node's connectionListener attaches the HTTPParser (socket.parser) before
Expand Down
75 changes: 75 additions & 0 deletions test/js/node/http/node-http.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4054,3 +4054,78 @@
c.outputData.push({ data: "y", encoding: "utf8", callback: null });
expect(d.outputData.length).toBe(0);
});

it("http.Server maxConnections destroys the excess and emits 'drop' like net.Server", async () => {
let served = 0;
const drops: any[] = [];
const server = createServer((_req, res) => {
served++;
res.end("ok");
});
server.maxConnections = 2;
Comment thread
robobun marked this conversation as resolved.
Resolved
server.on("drop", data => drops.push(data));
await once(server.listen(0, "127.0.0.1"), "listening");
const port = (server.address() as AddressInfo).port;

const openAndRequest = async () => {
const sock = connect(port, "127.0.0.1");
sock.on("error", () => {});
await once(sock, "connect");
let body = "";
sock.setEncoding("utf8");
sock.on("data", c => (body += c));
sock.write("GET / HTTP/1.1\r\nHost: x\r\n\r\n");
while (!body.endsWith("ok")) await once(sock, "data");

Check warning on line 4078 in test/js/node/http/node-http.test.ts

View check run for this annotation

Claude / Claude Code Review

openAndRequest hangs forever if the connection is closed without a response

`once(sock, 'data')` rejects on `'error'` but never settles on `'end'`/`'close'`, so if the server drops this connection with a clean FIN (exactly what the new `maxConnections` path does via `socketHandle.close()`) this loop blocks forever and the test times out with no diagnostic instead of a clear assertion failure. Race `'data'` against `'close'` and reject with the accumulated body — REVIEW.md requires wiring every failure event (including `close`) to reject the awaited promise.
Comment thread
robobun marked this conversation as resolved.
Resolved
return sock;
};

const s1 = await openAndRequest();
const s2 = await openAndRequest();
expect(served).toBe(2);
expect(drops).toEqual([]);

// Connections 3 and 4 are over the limit: the server emits 'drop' and
// destroys them on accept; the client side observes the close, and no
// 'request' is dispatched.
const outcomes: string[] = [];
for (let i = 0; i < 2; i++) {
const sock = connect(port, "127.0.0.1");
sock.on("error", () => {});
const outcome = new Promise<string>(r => {
sock.once("data", () => r("served"));
sock.once("close", () => r("dropped"));
});
await once(sock, "connect");
sock.write("GET / HTTP/1.1\r\nHost: x\r\n\r\n");
outcomes.push(await outcome);
sock.destroy();
}

expect({ served, drops: drops.length, outcomes }).toEqual({
served: 2,
drops: 2,
outcomes: ["dropped", "dropped"],
});
expect(drops[0]).toEqual({
localAddress: "127.0.0.1",
localPort: port,
localFamily: "IPv4",
remoteAddress: "127.0.0.1",
remotePort: expect.any(Number),
remoteFamily: "IPv4",
});
const connectionCount = await new Promise<number>(r => server.getConnections((_e, n) => r(n)));
expect(connectionCount).toBe(2);

// Freeing a slot lets the next connection through again.
s1.destroy();
while ((await new Promise<number>(r => server.getConnections((_e, n) => r(n)))) !== 1) {
await new Promise(r => setImmediate(r));
}
const s5 = await openAndRequest();
expect(served).toBe(3);
expect(drops.length).toBe(2);

for (const s of [s2, s5]) s.destroy();
await new Promise<void>(r => server.close(() => r()));

Check warning on line 4130 in test/js/node/http/node-http.test.ts

View check run for this annotation

Claude / Claude Code Review

Test leaks server and sockets on assertion failure

Cleanup (`server.close()` and the `s2`/`s5` destroys) only runs if every assertion above passes — a throw at `expect(served).toBe(2)`, the drop-payload `toEqual`, or `expect(connectionCount).toBe(2)` leaks the listening server and open keep-alive sockets. REVIEW.md requires resources be released via `try`/`finally` registered *before* the assertions, and the neighboring tests in this file (lines 3930-4020) all follow that pattern. Wrap the body after `listen` in `try { ... } finally { for (const
Comment thread
robobun marked this conversation as resolved.
Resolved
});
Loading