Skip to content
19 changes: 15 additions & 4 deletions src/js/internal/http1_server_fallback.ts
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,7 @@
function connectionListenerHTTP1(server, socket, options) {
const http = require("node:http");
const { HTTPParser, prepareError, calculateLenientFlags, continueExpression } = require("node:_http_common");
const { queuePipelinedResponse, advanceResponsePipeline } = require("node:_http_server");
const { kHandle: kHttp1ResponseHandle } = require("internal/http");
const { allMethods } = process.binding("http_parser");

Expand Down Expand Up @@ -326,12 +327,22 @@
}
};
res[kHttp1ResponseHandle] = handle;
res.assignSocket(socket);
// node's resOnFinish: release the socket once the response completes so the next
// keep-alive request's response can attach (assignSocket throws
// ERR_HTTP_SOCKET_ASSIGNED while a previous response is still assigned).
// Node's parserOnIncoming outgoing queue: while the previous response is
// still assigned (its deferred 'finish' has not detached it yet -
// pipelined requests always land here), this response is queued and its
// write()/end() buffer until the finish path below assigns it the socket.
// assignSocket would throw ERR_HTTP_SOCKET_ASSIGNED.
Comment thread
robobun marked this conversation as resolved.
Outdated
if (socket._httpMessage) {
queuePipelinedResponse(socket, res, versionMajor < 1 || versionMinor < 1);

Check warning on line 336 in src/js/internal/http1_server_fallback.ts

View check run for this annotation

Claude / Claude Code Review

Fallback pipelining lacks the outgoingData read-pause flood guard

The fallback path queues pipelined responses but doesn't carry over the `kOutgoingData` read-pause flood guard the native dispatcher applies right after `queuePipelinedResponse` (`_http_server.ts:962-963`, mirroring Node's `parserOnIncoming` and exercised by `test-http-pipeline-flood`). `bufferPipelinedWrite`/`End` still increment `socket[kOutgoingData]` on the plain Duplex, but nothing consults it here, and `pausePipelineReads`/`releasePipelineOutgoingData`'s resume both go through `socket[kHan
Comment thread
robobun marked this conversation as resolved.
} else {
res.assignSocket(socket);
}

Check failure on line 339 in src/js/internal/http1_server_fallback.ts

View check run for this annotation

Claude / Claude Code Review

Queued pipelined responses on fallback connections are never destroyed when the socket closes

The fallback socket `'close'` handler never destroys `socket[kPipelinedResponses]`, so if the duplex closes while responses are queued they (and their `req`) are never destroyed and never emit `'close'` — Node's `socketOnClose` aborts them, and the native path does the same in `NodeHTTPServerSocket.#onClose`. Before this PR the fallback path never populated the queue, so this is a newly-introduced leaked error path; mirroring the `#onClose` loop in the fallback `'close'` listener fixes it.
Comment thread
robobun marked this conversation as resolved.
// node's resOnFinish: release the socket once the response completes so
// the next keep-alive request's response can attach, then hand it to the
// next queued pipelined response (replaying whatever it buffered).
Comment thread
robobun marked this conversation as resolved.
Outdated
res.on("finish", function onFallbackResponseFinish() {
this.detachSocket(socket);
advanceResponsePipeline(server, socket);
});
Comment thread
robobun marked this conversation as resolved.

// Node's parserOnIncoming Expect routing (the native dispatcher applies the
Expand Down
72 changes: 48 additions & 24 deletions src/js/node/_http_server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -948,16 +948,7 @@
// Node.js, this response is queued (res.socket === null) and its
// writes are buffered until the in-flight response finishes and the
// pipeline assigns it the socket (advanceResponsePipeline).
http_res[kPipelinedQueuedState] = {
ops: [],
bytes: 0,
headerBytes: 0,
needDrain: false,
ended: false,
isAncient: !!isAncientHTTP,
socket,
};
(socket[kPipelinedResponses] ??= []).push(http_res);
queuePipelinedResponse(socket, http_res, !!isAncientHTTP);
// A pipelined dispatch can arrive after the previous response finished and detached
// (bytes still flushing keep it pending), leaving nothing in flight to advance the
// queue. Kick the pipeline once this dispatch settles.
Expand Down Expand Up @@ -2566,6 +2557,23 @@
}
}

// Like the dispatcher's pipelined branch and Node.js's parserOnIncoming
// outgoing queue: park the response behind the connection's in-flight one.
// Its write()/end() buffer (kPipelinedQueuedState) until
// advanceResponsePipeline assigns it the socket and replays them.
Comment thread
robobun marked this conversation as resolved.
function queuePipelinedResponse(socket, res, isAncient) {
res[kPipelinedQueuedState] = {
ops: [],
bytes: 0,
headerBytes: 0,
needDrain: false,
ended: false,
isAncient,
socket,
};
(socket[kPipelinedResponses] ??= []).push(res);
}

function advanceResponsePipeline(server, socket) {
// The previous response on this connection closed it (Connection: close,
// HTTP/1.0, maxRequestsPerSocket): like Node.js's resOnFinish, advancing
Expand All @@ -2584,7 +2592,6 @@
res[kPipelinedQueuedState] = undefined;
releasePipelineOutgoingData(socket, queued.bytes);
const handle = res[kHandle];
const socketHandle = socket[kHandle];

if (res.destroyed || !handle) {
// The queued response was destroyed before it could be sent; the
Expand All @@ -2595,22 +2602,36 @@
return;
}

if (
!socketHandle ||
socket.destroyed ||
!socketHandle.startPipelinedResponse(handle, !!queued.isAncient, !requestShouldKeepAlive(res.req))
) {
// The connection is already gone; the socket close path destroys queued
// responses, but make sure this (already dequeued) one is not skipped.
if (!res.destroyed) {
res.destroy();
if (socket instanceof NodeHTTPServerSocket) {
const socketHandle = socket[kHandle];
if (
!socketHandle ||
socket.destroyed ||
!socketHandle.startPipelinedResponse(handle, !!queued.isAncient, !requestShouldKeepAlive(res.req))
) {
// The connection is already gone; the socket close path destroys queued
// responses, but make sure this (already dequeued) one is not skipped.
Comment thread
robobun marked this conversation as resolved.
if (!res.destroyed) {
res.destroy();
}
return;
}
return;
}

if (res.assignSocket === ServerResponse.prototype.assignSocket) {
assignSocketInternal(res, socket);
if (res.assignSocket === ServerResponse.prototype.assignSocket) {
assignSocketInternal(res, socket);
} else {
res.assignSocket(socket);
}
} else {
// internal/http1_server_fallback connection (a foreign duplex): each
// response's JS handle writes to the socket itself, so there is no native
// socket handle to switch. The prototype assignSocket also installs the
// 'close' listener a plain stream needs. Clear `finished` before the
// assignment: assignSocket's _flush would otherwise emit 'prefinish' for
// an already-ended queued response before its ops have been replayed.
Comment thread
robobun marked this conversation as resolved.
Outdated
if (queued.ended) {
res.finished = false;
}
res.assignSocket(socket);
}
socket[kRequest] = res.req;
Expand Down Expand Up @@ -4017,4 +4038,7 @@
Server,
ServerResponse,
kConnectionsCheckingInterval,
// Pipelining internals shared with internal/http1_server_fallback.
queuePipelinedResponse,
advanceResponsePipeline,

Check warning on line 4043 in src/js/node/_http_server.ts

View check run for this annotation

Claude / Claude Code Review

Internal pipelining helpers exported on public node:_http_server surface

nit: `queuePipelinedResponse` and `advanceResponsePipeline` are Bun-internal helpers, but exporting them from `node:_http_server` puts them on a user-visible Node compat surface — `Object.keys(require('node:_http_server'))` now includes two names Node doesn't have. The fallback file already does `require('internal/http')` on the line right below the new require; consider exporting these two from an `internal/*` module instead so the `node:` surface stays clean.
Comment thread
robobun marked this conversation as resolved.
Outdated
};
78 changes: 78 additions & 0 deletions test/js/node/http/node-http.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4139,3 +4139,81 @@ it("connectionListener hands off Upgrade and CONNECT like Node", async () => {
expect(serverSide.destroyed).toBe(true);
}
});

it("connectionListener queues pipelined responses like Node", async () => {
// Both requests parse in one socket 'data' event, so the second one's
// headers complete while the first response is still assigned (its 'finish'
// detach is deferred a tick). The second response must be queued, not
// assigned - assignSocket throws ERR_HTTP_SOCKET_ASSIGNED - and its output
// must be held back until the first response completes.
async function exchange(handler: (req: any, res: any) => void, requestBytes: string, done: (buf: string) => boolean) {
const server = createServer(handler);
const [clientSide, serverSide] = duplexPair();
server.emit("connection", serverSide);
try {
return await new Promise<string>((resolve, reject) => {
let buf = "";
clientSide.on("data", d => {
buf += d;
if (done(buf)) resolve(buf);
});
clientSide.on("error", reject);
clientSide.on("close", () => reject(new Error("closed before expected output: " + buf)));
clientSide.write(requestBytes);
});
} finally {
clientSide.destroy();
serverSide.destroy();
}
}

// Handler finishes after the current tick: the second request is dispatched
// while the first response is still in flight.
{
const out = await exchange(
(req, res) => setImmediate(() => res.end("ok:" + req.url)),
"GET /a HTTP/1.1\r\nHost: x\r\n\r\nGET /b HTTP/1.1\r\nHost: x\r\n\r\n",
buf => buf.includes("ok:/a") && buf.includes("ok:/b"),
);
expect(out.indexOf("ok:/a")).toBeLessThan(out.indexOf("ok:/b"));
expect(out.match(/HTTP\/1\.1 200/g)).toHaveLength(2);
}

// The queued response is written (write + end, chunked) before the first
// one finishes: its bytes must still go out second, after the first body.
{
let releaseFirst!: () => void;
const firstGate = new Promise<void>(resolve => (releaseFirst = resolve));
const out = await exchange(
(req, res) => {
if (req.url === "/a") {
firstGate.then(() => res.end("first"));
} else {
res.write("second-part1");
res.end("second-part2");
releaseFirst();
}
},
"GET /a HTTP/1.1\r\nHost: x\r\n\r\nGET /b HTTP/1.1\r\nHost: x\r\n\r\n",
buf => buf.includes("second-part2") && buf.endsWith("0\r\n\r\n"),
);
expect(out.indexOf("first")).toBeLessThan(out.indexOf("second-part1"));
expect(out.indexOf("second-part1")).toBeLessThan(out.indexOf("second-part2"));
const second = out.slice(out.indexOf("HTTP/1.1 200", 1));
expect(second).toContain("Transfer-Encoding: chunked");
}

// Three pipelined requests answered synchronously: each finish hands the
// socket to the next queued response.
{
const out = await exchange(
(req, res) => res.end("r:" + req.url + ";"),
"GET /1 HTTP/1.1\r\nHost: x\r\n\r\nGET /2 HTTP/1.1\r\nHost: x\r\n\r\nGET /3 HTTP/1.1\r\nHost: x\r\n\r\n",
buf => buf.includes("r:/3;"),
);
expect(out.indexOf("r:/1;")).toBeGreaterThanOrEqual(0);
expect(out.indexOf("r:/1;")).toBeLessThan(out.indexOf("r:/2;"));
expect(out.indexOf("r:/2;")).toBeLessThan(out.indexOf("r:/3;"));
expect(out.match(/HTTP\/1\.1 200/g)).toHaveLength(3);
}
});
Loading