Skip to content

node:http: keep receiving the request body after the response has ended - #38196

Closed
robobun wants to merge 8 commits into
mainfrom
farm/be122dd5/http-req-complete-after-early-response
Closed

robobun wants to merge 8 commits into
mainfrom
farm/be122dd5/http-req-complete-after-early-response

Conversation

@robobun

@robobun robobun commented Aug 13, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes #4733.
Fixes #18613.

Problem

  • A node:http handler that answers before the request body has arrived (res.end() right away on a POST, the usual early 413/401/redirect) immediately gets req.complete === true, req.readableEnded === true, req.destroyed === true, and req emits 'end' and 'close', while most of the body is still in flight. Node leaves all three false and emits 'end'/'close' only once the body has actually been received; if the peer drops the connection mid-body instead, Node emits nothing on req and leaves it incomplete.
  • The same mechanism loses the body for a consumer: req.on('data', ...) followed by a synchronous res.end() receives nothing and 'end' fires with an empty body (node:http IncomingMessage stream data cannot be read, events are not emitted #4733, http body won't be received if res.end is called too early #18613), whether the body was in the same packet as the headers (curl -d) or still in flight.
  • Native cause: NodeHTTPResponse::write_or_end::<true> (src/runtime/server/NodeHTTPResponse.rs) released the body read state at res.end() unless HAS_CUSTOM_ON_DATA was set, and that flag was never set: the dispatcher reset handle.hasCustomOnData = false right after arming the IncomingMessage's callback. uws's end() (markDone()) also nulls the connection's body data handler, so no byte after res.end() reached JS. maybe_stop_reading_body did the same from the dispatch tail for handlers that end synchronously.
  • JS cause: IncomingMessage._dump() cleared handle.ondata, and _read() treated _dumped as EOF, so a dumped request fabricated its own end right after res.end().
  • Non-keep-alive cause (Connection: close request or HTTP/1.0, i.e. curl --http1.0 -d, ab, many proxies): the response's 'finish' listener ends the server socket (kMustCloseConnection, Node's destroySoon()), and for a handler that responds synchronously that runs while uws is still parsing the read that carried the request. jsFunctionNodeHTTPServerSocketEnd shut the socket down on the spot, and HttpContext's request hook stops parsing a shut-down socket right after the request head, so a body that arrived together with the headers was dropped and the request never completed. With the two fixes above alone this path still lost the body (body: "", no 'end'); it was the same on main and with node:http: keep delivering the request body after a synchronous res.end() #35489, which discarded the body explicitly for this case.

Fix

  • Native: res.end() keeps the body read state while the IncomingMessage's ondata is still armed and the body is still arriving, and re-arms the connection's data handler after uws's end() dropped it (in buffering mode when the reader is paused). maybe_stop_reading_body only discards when no reader is armed or the transport is gone. pause(), resume() and arming ondata keep working after the response has ended, gated on body_still_arriving() (state Pending and no parked fin), which is exactly the window in which the per-connection handler slot belongs to this request.
  • Native, release of the request: at the fin, on_data_or_aborted now re-evaluates the pending state unconditionally (the fin callback's nextTick drain runs 'end' -> autoDestroy -> ondata = undefined, which releases body_read_ref before the tail looked at it, so the gated version stranded the request and server.close() never completed once the connection served another request); set_on_data's clear branch stops the native delivery and re-evaluates when a reader is torn down mid-body; mark_request_as_done also drops body_read_ref, which is still held when the connection closes after the response.
  • HAS_CUSTOM_ON_DATA / handle.hasCustomOnData are removed: the flag was never set and only existed to gate those discards (one test fixture located the handle through it and now uses ondata).
  • JS: _dump() leaves the native callback armed (onDataIncomingMessage already drops the chunks of a dumped request and reports the fin), _read() no longer emits EOF for a dumped request, and the dump decision moves from res.end() to the response's 'finish' listener, where Node's resOnFinish makes it, so a consumer attached in the same tick as res.end() still counts.
  • Non-keep-alive connections (src/js/node/_http_server.ts, src/jsc/bindings/node/JSNodeHTTPServerSocket*.cpp, packages/bun-uws/src/HttpResponse.h): onResponseFinishHandleSocket marks the connection (kEndAfterResponse) before calling socket.end(), and _final passes that to the native end(). For that end() only, when the response is complete, a body handler is still armed and the socket is the one uws is parsing right now (isDeliveringBodyAfterResponse()), shutdownAfterResponseDrains() sets HTTP_CONNECTION_CLOSE and returns instead of shutting down, exactly as it already does while response bytes are still buffered. uws finishes the buffer (the body reaches the IncomingMessage, the request completes) and its post-parse gate shuts down and closes the connection, which is Node's order too: the whole read is parsed, then destroySoon(). A socket.end() issued by user code, an end() outside a parse (res.end() from a timer), tunnels (isConnectRequest) and responses still in flight are all unaffected and shut down immediately as before.
  • While such a close is pending (this one or the pre-existing buffered-bytes one), the parser is told to dispatch nothing after the current message (nodeHttpStopDispatchingAfterCurrentMessage(), packages/bun-uws/src/HttpParser.h, the flag a Connection: close request already sets): a request pipelined behind it in the same read would otherwise start a new response, and starting one clears HTTP_CONNECTION_CLOSE, leaving the ended connection open and serving (for a close-delimited response, appending the next response to its body). Such a request is reported as HPE_CLOSED_CONNECTION, as it is behind a Connection: close request, and the parse-error exit of HttpContext::onData now runs the same close gate (packages/bun-uws/src/HttpContext.h), so the connection is closed even when a 'clientError' listener does not destroy it; the gate only acts on connections already marked to close whose response is complete, so every other parse error is still left to the listener.
  • Why this is the right behaviour: it matches Node. Every scenario in the new tests was run under Node v26.3.0 and under this build with identical output, including the peer-drop case (Node's socketOnClose only aborts requests whose response has not finished, which NodeHTTPServerSocket#onClose already mirrors) and server.close() completing in each. should_request_be_pending() already described "response ended, body pending" as pending; this change makes that state reachable and makes sure every way out of it releases the request.
  • Verified with test/js/node/http/node-http-server-abort-events.test.ts: 15 new tests. 7 of the first 8 fail on main (state flipping at res.end(), empty bodies, pause()/resume() never completing, the abort variant of the exit test; the eighth, exit after the body arrives and the connection is reused, guards the fin-time release). Under "on a connection the response closes": Connection: close and HTTP/1.0 with a consumer, an unread body and a body that never completes all failed on this branch before the JSNodeHTTPServerSocket change (empty body, no events); 3 of them fail on main as well, while the unread-body one passes there only because main's fabricated end at res.end() happens to produce the same final state. The 3 pipelined cases fail on main (body lost); with the deferral but without the no-dispatch flag and the error-path gate, the response-driven one and the 'clientError'-listener one fail (the connection is never closed), which is what they guard. Each asserts what Node v26.3.0 shows: the request state at the moment the server closes the socket, and for the pipelined cases exactly one response followed by the close (Node still runs the handler for a request pipelined behind a response-driven close but never answers it either; the handler count is deliberately not asserted).
  • test/js/node/http/node-http.test.ts, "request body still flows after res.end() was called in the handler": the 11 consumer-side cases from node:http: keep delivering the request body after a synchronous res.end() #35489 (pipe()/on('data') before and after res.end(), write()+end(), end() on nextTick, _dump() happening on 'finish', keep-alive reuse across three consumed bodies, a chunked body split across segments, a mid-upload close reaching beforeExit); 10 fail on main, all pass here. Its Connection: close case expected the fabricated 'end' and was replaced by the tests above.
  • Also run: node-http.test.ts, node-http-req-socket-pause, node-http-backpressure, node-http-transfer-encoding, node-http-server-socket-end-drain, node-http-ondata-reregister-leak, node-http-connect, node-http-with-ws, express and body-parser suites, and the 422 vendored test-http-* files (414 pass). The failures are the same with the release binary or without this diff: the env-proxy tests (this container sets HTTP_PROXY/NO_PROXY), test-http-agent-keepalive's 1 ms close window and a few 500 ms / 5 s budgets on the ASAN build, and node-http.test.ts's proxy test (localhost resolution here).
  • Related: node:http: keep delivering the request body after a synchronous res.end() #35489 fixed the consumer half of the same mechanism with a different native approach and is closed in favour of this PR; its tests were carried over as described above. node:http: deliver a pipelined POST's body when the previous response is still in flight #34761 (a pipelined successor's handler slot cleared from clear_on_data_callback) is independent and untouched; the code added here only touches the shared slot while body_still_arriving(). A body whose fin was parked while the stream was paused is still never released after res.end(); that is pre-existing (reproduces on 1.4.0) and is node:http: release a request whose body fin was buffered while paused once the response ends #38207.

Background

  • Body delivery: when a request has a body, the dispatcher arms handle.ondata = onDataIncomingMessage on the request's native handle; native calls it per chunk and once more with isLast at the body's fin. isLast is what sets req.complete and pushes EOF ('end', then autoDestroy's 'close'). req._dump() is Node's "nobody will read this body": it removes the 'data' listeners and resumes the stream so the bytes are discarded as they come in.
  • uws keeps one HttpResponseData per connection, reused by every request on a keep-alive connection, with a single body data handler (inStream) and context pointer. end() calls markDone(), which nulls that handler. One request's body fin always precedes the next request's head in the byte stream, so while a body is being parsed the handler slot can only belong to that request; once its fin has been seen (delivered, or parked while paused) the slot may already be the next request's.
  • Closing non-keep-alive connections: for an HTTP/1.0 or Connection: close request the JS layer marks the response kMustCloseConnection and its 'finish' listener calls socket.end() (Node: res._last and resOnFinish -> socket.destroySoon()). In Bun 'finish' for a synchronously ended response is emitted before the dispatcher returns to uws, i.e. while uws is still inside HttpContext::onData for the read that carried the request (HttpContextData::parsingSocket is that socket); the body bytes in the same read are only parsed after the dispatcher returns. uws already has two places that close a connection marked HTTP_CONNECTION_CLOSE once the response is complete and flushed: the gate at the end of onData and the one in onWritable; the existing buffered-response deferral in shutdownAfterResponseDrains() relies on the second, the new case on the first (plus, for a parse that ends in an error, the same gate run from the error exit). Dispatching a request resets the per-connection response state, including that mark, which is why a pending close also has to stop the parser from dispatching; the parser's nodeHttpSawConnectionClose already does that for requests that carried Connection: close (Node's parser raises HPE_CLOSED_CONNECTION for anything after such a message).
  • body_read_ref is an event-loop keep-alive held while a body is pending. IS_REQUEST_PENDING is a self-reference plus the server's in-flight request count, which is what server.close() waits on; mark_request_as_done releases both, and should_request_be_pending() decides when a response that has ended may be released.
Repro (Node v26 vs Bun)
import { once } from "node:events";
import { createServer } from "node:http";
import { connect } from "node:net";
let gotReq; const request = new Promise(r => (gotReq = r));
const server = createServer((req, res) => { res.end("ok"); gotReq(req); });
server.listen(0, "127.0.0.1"); await once(server, "listening");
const client = connect(server.address().port, "127.0.0.1");
let data = ""; client.on("data", d => (data += d));
await once(client, "connect");
client.write("POST / HTTP/1.1\r\nHost: x\r\nContent-Length: 6\r\n\r\nabc"); // half of the body
const req = await request;
while (!data.includes("ok")) await new Promise(r => setTimeout(r, 5));
console.log({ complete: req.complete, readableEnded: req.readableEnded, destroyed: req.destroyed });
req.on("end", () => console.log("req end")); req.on("close", () => console.log("req close"));
client.write("def");

Node v26.3.0 (and this branch): { complete: false, readableEnded: false, destroyed: false }, then req end, req close once def arrives.
Bun before this change: { complete: true, readableEnded: true, destroyed: true } right after res.end(), and neither event fires later.


[review] gate passed · iteration 7 · 14 files touched

fails on main (without fix)
ASAN without fix: 23 failed, 1 skipped
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/node/http/node-http-server-abort-events.test.ts test/js/node/http/node-http.test.ts
bun test v1.4.1 (65362b53b)

test/js/node/http/node-http.test.ts:
(pass) node:http > createServer > hello world [430.54ms]
(pass) node:http > createServer > is not marked encrypted (#5867) [63.65ms]
(pass) node:http > createServer > request & response body streaming (large) [103.41ms]
(pass) node:http > createServer > request & response body streaming (small) [64.99ms]
(pass) node:http > createServer > listen should return server [24.27ms]
(pass) node:http > createServer > listen callback should be bound to server [24.65ms]
(pass) node:http > createServer > emits 'listening' on the next tick, before the event loop polls [34.86ms]
(pass) node:http > createServer > emits a listen() error on the next tick, before the event loop polls [44.37ms]
(pass) node:http > createServer > calls the listen() callback after a retry from the EADDRINUSE 'error' handler [48.13ms]
(pass) node:http > createServer > http: closing a server listened from 'beforeExit' > 
... (truncated)

release without fix: 27 failed, 1 skipped
bun test v1.4.1-canary.1 (65362b53b)

test/js/node/http/node-http.test.ts:
(pass) node:http > createServer > hello world [12.05ms]
(pass) node:http > createServer > is not marked encrypted (#5867) [2.40ms]
(pass) node:http > createServer > request & response body streaming (large) [4.06ms]
(pass) node:http > createServer > request & response body streaming (small) [2.15ms]
(pass) node:http > createServer > listen should return server [0.76ms]
(pass) node:http > createServer > listen callback should be bound to server [0.77ms]
(pass) node:http > createServer > emits 'listening' on the next tick, before the event loop polls [1.09ms]
(pass) node:http > createServer > emits a listen() error on the next tick, before the event loop polls [1.58ms]
(pass) node:http > createServer > calls the listen() callback after a retry from the EADDRINUSE 'error' handler [1.89ms]
(pass) node:http > createServer > http: closing a server listened from 'beforeExit' > re-emits 'beforeExit' [23.20ms]
(pass) node:http > createServer > https: closing a server listened from 'beforeExit' > re-emits 'beforeExit' [34.32ms]
(pass) node:http > createServer > should use the provided port [1.33ms]
(pa
... (truncated)
passes on PR (with fix)
ASAN with fix: 1 skipped
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/node/http/node-http-server-abort-events.test.ts test/js/node/http/node-http.test.ts
bun test v1.4.1 (65362b53b)

test/js/node/http/node-http.test.ts:
(pass) node:http > createServer > hello world [421.79ms]
(pass) node:http > createServer > is not marked encrypted (#5867) [60.54ms]
(pass) node:http > createServer > request & response body streaming (large) [102.51ms]
(pass) node:http > createServer > request & response body streaming (small) [65.26ms]
(pass) node:http > createServer > listen should return server [24.64ms]
(pass) node:http > createServer > listen callback should be bound to server [22.86ms]
(pass) node:http > createServer > emits 'listening' on the next tick, before the event loop polls [35.72ms]
(pass) node:http > createServer > emits a listen() error on the next tick, before the event loop polls [41.15ms]
(pass) node:http > createServer > calls the listen() callback after a retry from the EADDRINUSE 'error' handler [47.87ms]
(pass) node:http > createServer > http: closing a server listened from 'beforeExit' > 
... (truncated)

release with fix: 1 skipped
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped)
  target       linux-x64-gnu
  build type   Release
  build dir    ./build/release
  revision     3e88b4f986
  features     baseline

23 deps, 131 codegen, 1172 objects in 631ms

ninja: Entering directory `/workspace/bun/build/release'
[1/1244] install /workspace/bun
bun install v1.4.1-canary.1 (65362b53b)

Checked 26 installs across 63 packages (no changes) [7.00ms]
[2/1244] install /workspace/bun/packages/bun-error
bun install v1.4.1-canary.1 (65362b53b)

Checked 1 install across 2 packages (no changes) [1.00ms]
[3/1244] gen ErrorCode+*.h
[4/1244] gen bindgenv2
[5/1244] install /workspace/bun/src/node-fallbacks
bun install v1.4.1-canary.1 (65362b53b)

Checked 111 installs across 104 packages (no changes) [13.00ms]
[6/1244] fetch tinycc
[tinycc] up to date
[7/1243] fetch zlib
[zlib] up to date
[8/1243] fetch libjpeg-turbo
[libjpeg-turbo] up to date
[9/1216] gen node-fallbacks/react-refresh.js
Bundled 1 module in 12ms

  react-refresh.js  4.81 KB  (entry point)

[10/1216] gen .bind.ts → GeneratedBindings.cpp
[11/1216] gen bake.{client,server,error}.js
-> bake.client.js, bake.serve
... (truncated)
diff hotspot
packages/bun-uws/src/HttpContext.h                 |  10 +
 packages/bun-uws/src/HttpParser.h                  |  14 +-
 packages/bun-uws/src/HttpResponse.h                |  16 +
 src/js/node/_http_incoming.ts                      |  15 +-
 src/js/node/_http_server.ts                        |  29 +-
 src/jsc/bindings/node/JSNodeHTTPServerSocket.cpp   |  21 +-
 src/jsc/bindings/node/JSNodeHTTPServerSocket.h     |   8 +-
 .../node/JSNodeHTTPServerSocketPrototype.cpp       |   8 +-
 src/runtime/server/NodeHTTPResponse.rs             | 182 +++++----
 src/runtime/server/mod.rs                          |   3 +-
 src/runtime/server/server.classes.ts               |   4 -
 .../node-http-ondata-reregister-leak.fixture.js    |   2 +-
 .../http/node-http-server-abort-events.test.ts     | 413 ++++++++++++++++++++-
 test/js/node/http/node-http.test.ts                | 316 ++++++++++++++++
 14 files changed, 932 insertions(+), 109 deletions(-)

gate history · 5 passed · 1 rejected · iteration 7

evidence per changed file
file                                                      reads  edits  tests
packages/bun-uws/src/HttpContext.h                            0      0      0
packages/bun-uws/src/HttpParser.h                             0      0      0
packages/bun-uws/src/HttpResponse.h                           0      0      0
src/js/node/_http_incoming.ts                                 2      3      0
src/js/node/_http_server.ts                                   3      2      0
src/jsc/bindings/node/JSNodeHTTPServerSocket.cpp              2      2      0
src/jsc/bindings/node/JSNodeHTTPServerSocket.h                2      2      0
…c/jsc/bindings/node/JSNodeHTTPServerSocketPrototype.cpp      1      1      0
src/runtime/server/NodeHTTPResponse.rs                        7      4      0
src/runtime/server/mod.rs                                     0      0      0
src/runtime/server/server.classes.ts                          0      0      0
…s/node/http/node-http-ondata-reregister-leak.fixture.js      0      0      0
test/js/node/http/node-http-server-abort-events.test.ts       4      3      0
test/js/node/http/node-http.test.ts                           5      3      0

root cause · written by the author bot

The bug was that calling res.end() inside the request handler before the request body had been consumed caused the server to drop or discard the remaining body, so data listeners and piped destinations attached to the request never received it. The fix aligns the behavior with Node.js by keeping the request body flowing to its consumers until it is fully received, deferring the decision to dump the body to the response's finish handler based on whether a consumer is attached at that point. As a result, a listener added after res.end() in the same tick still receives the complete body, and r…

Rebases onto main (a938cef, e88810b, 3e88b4f)

The rebase resolved four conflicts:

  • node-http-server-abort-events.test.ts: main added a "res.destroy() defers 'close'" suite at the same insertion point as this PR's suite. Kept both and merged the imports.
  • node-http.test.ts: main added the https 'clientError' after close() test at the same insertion point. Kept both.
  • _http_server.ts: main moved the server socket class into a lazy getNodeHTTPServerSocket(). Re-applied the kEndAfterResponse field and the _final change inside the new structure.
  • NodeHTTPResponse.rs: main removed the leftover "defer" comments (Remove leftover Zig defer comments #40051). Took this PR's calls without those comments.

Both test files pass after the rebase: 188 pass, 1 pre-existing skip.

The second rebase (e88810b) resolved one conflict: main replaced the manual ref_() / deref() keep-alive in on_data_or_aborted with an RAII ref_guard() (#40516). Took this PR's unconditional body_read_ref release and re-evaluation, and dropped the now redundant deref(). Same test result after the rebase.

The third rebase (3e88b4f) resolved two conflicts: main added the latin-1 header-bytes suite (#40685) at this PR's insertion point in node-http.test.ts (kept both), and main adopted the same 127.0.0.1 fix for node-http-proxy.js (took main's version). Both test files pass: 192 pass, 1 pre-existing skip.

@coderabbitai

coderabbitai Bot commented Aug 13, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

On-demand reviews are free for the next 23 days. After that, they cost $0.25 per reviewed file.

Or wait 7 minutes for your next included review.

View limit details

Limit details: You’ve used all 5 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: e9f67c18-4dae-438d-bb8f-a6eea8347ff8

📥 Commits

Reviewing files that changed from the base of the PR and between 69c6138 and 3e88b4f.

📒 Files selected for processing (14)
  • packages/bun-uws/src/HttpContext.h
  • packages/bun-uws/src/HttpParser.h
  • packages/bun-uws/src/HttpResponse.h
  • src/js/node/_http_incoming.ts
  • src/js/node/_http_server.ts
  • src/jsc/bindings/node/JSNodeHTTPServerSocket.cpp
  • src/jsc/bindings/node/JSNodeHTTPServerSocket.h
  • src/jsc/bindings/node/JSNodeHTTPServerSocketPrototype.cpp
  • src/runtime/server/NodeHTTPResponse.rs
  • src/runtime/server/mod.rs
  • src/runtime/server/server.classes.ts
  • test/js/node/http/node-http-ondata-reregister-leak.fixture.js
  • test/js/node/http/node-http-server-abort-events.test.ts
  • test/js/node/http/node-http.test.ts

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator Author

Status: reproduced and fixed; self-review in progress.

  • Reproduced with a raw net client sending half of a Content-Length: 6 POST body to a handler that calls res.end() immediately: on main req.complete / readableEnded / destroyed are true and 'end' + 'close' have fired by the time the response reaches the client; Node v26.3.0 reports all three false and emits the events once the rest of the body arrives. The consumer shapes from node:http IncomingMessage stream data cannot be read, events are not emitted #4733 / http body won't be received if res.end is called too early #18613 ('data' listener + synchronous res.end()) deliver an empty body on main.
  • Tests: test/js/node/http/node-http-server-abort-events.test.ts (new describe block). 7 of the 8 new tests fail on a build of main without the src/ changes and all pass with them; every scenario was also run under Node v26.3.0 with identical results.

@github-actions

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. node:http: keep delivering the request body after a synchronous res.end() #35489 - Fixes the same two issues (node:http IncomingMessage stream data cannot be read, events are not emitted #4733, http body won't be received if res.end is called too early #18613) with a near-identical fix: moves the req._dump() decision out of ServerResponse.end() into emitResponseFinish, drops the HAS_CUSTOM_ON_DATA gate in maybe_stop_reading_body/write_or_end, re-arms uWS on_data after end(), and defers mark_request_as_done to the body's fin.

🤖 Generated with Claude Code

@robobun

robobun commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator Author

Not a duplicate of #35489, although the two overlap on the consumer half (#4733 / #18613), as noted in the description.

The bug this PR was opened for is the request with no consumer: on main (and with #35489, which does not touch _http_incoming.ts) req._dump() still clears handle.ondata and _read() still treats _dumped as EOF, so req.complete / readableEnded / destroyed flip to true and 'end' + 'close' fire at res.end() while the body is still in flight, and a connection dropped mid-body still reports a completed request. The first two tests and the abort exit test here cover that and would fail with #35489 applied. On the native side this PR also keeps pause() / resume() / arming and the release of the request consistent for that window (body_still_arriving()), and it removes the never-set hasCustomOnData flag instead of leaving it in place. #35489 has been conflicting since the beginning of the month; this supersedes it.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR and the automated bug-hunt found no issues. Because it reworks native request-lifecycle state in the node:http server hot path — body_read_ref balance across several new release points, re-arming the shared per-connection uws data handler after end(), and the pause/resume gating once the response is over — a human look is still warranted.

Checked: no stale HAS_CUSTOM_ON_DATA / hasCustomOnData references remain anywhere; the new body_read_ref.unref in mark_request_as_done is idempotent and can't double-decrement; body_still_arriving() correctly excludes a parked fin so the re-arm in write_or_end and the clear_on_data in set_on_data never touch a pipelined successor's handler slot; the unconditional mark_request_as_done_if_necessary() at the fin is guarded by should_request_be_pending() so it is a no-op while the response is still in flight.

Extended reasoning...

Overview

This PR changes how the native node:http server handles a request body that is still arriving after res.end() has run. On the native side (NodeHTTPResponse.rs) it: keeps the body read state alive when a JS ondata reader is armed, re-arms the connection's uws inStream handler after end()'s markDone() dropped it, extends pause()/resume()/set_on_data to keep working while body_still_arriving(), adds body_read_ref releases in mark_request_as_done and in set_on_data's clear branch, and re-evaluates the pending state unconditionally at the fin. It removes the never-set HAS_CUSTOM_ON_DATA flag and its .classes.ts accessor. On the JS side, _dump() no longer clears the native callback, _read() no longer treats _dumped as EOF, and the dump decision moves from res.end() to the response's 'finish' listener (matching Node's resOnFinish). Eight new tests cover completion timing, body delivery with a synchronous res.end(), pause/resume after the response, peer-drop mid-body, and process exit.

Security risks

None identified. This is request-body flow control and lifecycle bookkeeping; no auth, crypto, or untrusted-length parsing is touched. The change re-arms a per-connection callback slot, but only while body_still_arriving() (state Pending and no parked fin), which is exactly the window in which the parser is inside this request's body and the slot cannot yet belong to a pipelined successor.

Level of scrutiny

High. This is the node:http server hot path, and the change threads through several ref-count / keep-alive balances (body_read_ref, IS_REQUEST_PENDING, the self-ref()/deref() at the fin) whose imbalance is either a leak (server.close() never resolves, process never exits) or a premature release. The PR description is unusually rigorous — it names every release point and why each is needed, and the new tests include two subprocess exit tests that would hang on a stranded ref — but the interaction with the shared HttpResponseData slot on keep-alive connections and the acknowledged pre-existing edge case (a fin parked while paused is still never released after res.end()) mean a maintainer familiar with the uws layer should confirm the re-arm is safe across every end() variant.

Other factors

The tests are strong: they await observable conditions (response bytes on the wire, server.close() resolving, subprocess exit), assert the full state object, and cover both the fix and its release paths. The author ran the broader http suite and the vendored Node test-http-* corpus. I verified the removed flag has no remaining references, that KeepAlive::unref is idempotent so the added body_read_ref.unref in mark_request_as_done cannot double-decrement when the fin path already released it, and that mark_request_as_done_if_necessary() is gated by should_request_be_pending() so the now-unconditional call at the fin does nothing while the response is still pending. Given the subtlety and the hot path, I'm deferring rather than approving.

@robobun

robobun commented Aug 13, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 3:47 AM PT - Aug 28th, 2026

❌ @robobun, your commit 3e88b4f has 1 failures in Build #107630 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 38196

That installs a local version of the PR into your bun-38196 executable, so you can run:

bun-38196 --bun

Comment thread src/js/node/_http_incoming.ts Outdated
Comment thread src/js/node/_http_incoming.ts Outdated
Comment thread src/js/node/_http_server.ts Outdated
Comment thread src/js/node/_http_server.ts Outdated
Comment thread src/js/node/_http_server.ts Outdated
Comment thread src/jsc/bindings/node/JSNodeHTTPServerSocket.cpp Outdated
Comment thread src/jsc/bindings/node/JSNodeHTTPServerSocket.cpp Outdated
Comment thread src/jsc/bindings/node/JSNodeHTTPServerSocket.h Outdated
Comment thread src/runtime/server/NodeHTTPResponse.rs Outdated
Comment thread src/runtime/server/NodeHTTPResponse.rs Outdated
Comment thread src/runtime/server/NodeHTTPResponse.rs Outdated
Comment thread src/runtime/server/NodeHTTPResponse.rs Outdated
Comment thread src/runtime/server/NodeHTTPResponse.rs Outdated
Comment thread src/runtime/server/NodeHTTPResponse.rs Outdated
Comment thread src/runtime/server/NodeHTTPResponse.rs Outdated
Comment thread src/runtime/server/NodeHTTPResponse.rs Outdated
Comment thread src/runtime/server/NodeHTTPResponse.rs Outdated
Comment thread src/runtime/server/NodeHTTPResponse.rs Outdated
Comment thread src/runtime/server/NodeHTTPResponse.rs Outdated
Comment thread src/runtime/server/NodeHTTPResponse.rs Outdated
Comment thread src/runtime/server/NodeHTTPResponse.rs Outdated
Comment thread src/runtime/server/NodeHTTPResponse.rs Outdated
…quest's connection

For an HTTP/1.0 or Connection: close request the response 'finish' listener
ends the server socket. When the handler responded before the body was
read, that end() runs while the read carrying the request is still being
parsed; the shutdown made the parser stop after the request head, so the
body bytes already in the buffer were dropped and the request never
completed. Hand the close to uWS's post-parse gate in that case (as is
already done when response bytes are still buffered): the body is delivered
first and the connection is closed right after, which is what Node does.
…onnection-close tests

Client and server share the test's event loop, so the client's 'close' can
fire before the server-side close has been observed; a listener added after
waiting on the latter could miss it.
… behind it

The deferral is now taken only for the end() that onResponseFinishHandleSocket
issues (passed down from _final), so a socket.end() from user code behaves as
before. While a deferred close is pending, the parser no longer dispatches a
request pipelined behind the current message: dispatching it would start a
new response and clear the connection's close mark, leaving the ended
connection open and serving. Such a request is reported as
HPE_CLOSED_CONNECTION like one behind a Connection: close request, and the
parse-error path now runs the close gate too, so the connection is closed
even when a 'clientError' listener does not destroy it.
…s; bind the proxy test to 127.0.0.1

The abort-events file's scenarios are verified against Node.js, so keep it free
of Bun-only helpers. The proxy test's listen("localhost") can bind ::1 while
the client connects to 127.0.0.1; bind the IPv4 loopback explicitly.
@robobun
robobun force-pushed the farm/be122dd5/http-req-complete-after-early-response branch from e88810b to 3e88b4f Compare August 28, 2026 09:24

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

Jarred-Sumner pushed a commit that referenced this pull request Sep 11, 2026
… once the response ends (#38207)

### Problem

- A `node:http` server request stays counted as in flight forever when
the last chunk of its body arrived while the `IncomingMessage` was
paused and the response was ended afterwards. `server.close()` never
calls back (the server's `'close'` event needs the in-flight count to
reach zero), the process it keeps alive never exits, and the native
response object leaks (`IS_REQUEST_PENDING` is a self-reference). Node
v26 closes cleanly; reproduces on bun 1.4.0 and main.
- Easy to hit without an explicit `pause()`: a body chunk larger than
the `IncomingMessage` `highWaterMark` that nobody reads pauses the
connection mid-segment (Node's `readStop` semantics), so the rest of the
same segment (remaining chunks + terminating chunk) is buffered natively
while paused.
- Cause, `src/runtime/server/NodeHTTPResponse.rs`:
- `on_buffer_request_body_while_paused(last = true)` sets
`IS_DATA_BUFFERED_DURING_PAUSE_LAST`, releases `body_read_ref`, and
deliberately leaves `body_read_state == Pending` so JS can still drain
the buffered tail (`drainRequestBody`).
- `should_request_be_pending()` read that `Pending` as "body still
arriving", so `res.end()` -> `on_request_complete()` kept
`IS_REQUEST_PENDING`. `write_or_end`'s dump-equivalent does not apply
either, because it keys on `body_read_ref` still being held.
- Nothing re-evaluates later: uws delivers nothing further for that body
(it nulls its handler at the fin and again at `end()`), and the later
`ondata = undefined` from `_dump()`/`_destroy()` only flips the state to
`Done`.
- The socket-close fallback in `handle_abort_or_timeout` only reaches
the connection's *current* response, so as soon as the keep-alive
connection serves one more request the strand is permanent.

### Fix

- `should_request_be_pending()` treats a body whose fin was buffered
while paused as complete (`Pending` and not
`IS_DATA_BUFFERED_DURING_PAUSE_LAST` is the only "still arriving"
state). The request is then released where every other body state is
released: at `res.end()` via `on_request_complete()`, or for an upgrade
tunnel with a body at the fin itself (the existing
`mark_request_as_done_if_necessary()` call in
`on_buffer_request_body_while_paused`, matching what the unbuffered fin
already does in `on_data_or_aborted`).
- `mark_request_as_done()` no longer frees
`buffered_request_body_data_during_pause` in that state while the
connection is still open: the `IncomingMessage` still owns that tail and
drains it on its next `_read()` (possibly after the response ended). It
is freed when drained, when the reader lets go (`set_on_data`'s clear
branch, reached from `_dump()` / `_destroy()` / a re-arm attempt that
follows `_read()`'s drain), or with the box. Closed/upgraded connections
free it immediately as before.
- Why this is correct: once the fin has been seen there is nothing left
for the accounting to wait for. Every other consumer of the flag already
treats it as "body complete" (`hasBody` reports done,
`pause()`/`resume()`/`ondata` refuse to re-arm), and the accounting
release at `res.end()` is what Node does too (`resOnFinish` drops the
request from the server's queue regardless of whether its body was
consumed). Releasing the accounting does not affect JS's ability to read
the tail because the wrapper holds its own reference to the box; only
the buffer's lifetime had to be decoupled.
- The `clear_on_data()` reached from `mark_request_as_done()` on this
new path is a no-op: uws already nulled the connection's handler when it
delivered the fin (and again in `end()`), and `ondata` is cleared on
this request's own wrapper (`armed_this_value`).
- Tests: `test/js/node/http/node-http-req-socket-pause.test.ts`, four
new tests, each also checked against Node v26 (all pass there):
- unread body, response ended later, second keep-alive request, server
`'close'` fires: times out on main.
- reader that starts reading as the response ends still gets the
buffered tail, then server `'close'` fires: times out on main, and fails
(tail missing) if the tail is freed at release.
- same with the next request pipelined in the same segment (covers the
variant where the tail was drained before `res.end()`): times out on
main.
- upgrade request with such a body, reader attached later gets the whole
body: passes on main by design, fails (tail missing) if the tail is
freed at the fin-time release; covers the tunnel arm.
- Also run on this build: the handoff repro exits with `server closed`;
`node-http.test.ts`, `node-http-backpressure*.test.ts`,
`node-http-transfer-encoding`, `node-http-server-abort-events`,
`node-http-connect`, `node-http-with-ws`,
`node-http-ondata-reregister-leak`, `node-http-server-socket-end-drain`,
`node-http-uaf`, `node-http-server-timeouts`, and 42 vendored
`test-http-{pause,no-read-no-dump,expect-continue,keep-alive,pipeline,upgrade-server-with-body,...}`
tests. The only failures are identical without this diff (the proxy
test's `localhost` resolution in this container and a few subprocess
tests exceeding 5 s on the ASAN build). `cargo clippy -p bun_runtime` is
clean.
- Related: #38196 (keep receiving the body after the response ended)
names this same predicate `body_still_arriving()` and leaves this strand
to a separate fix; in its model a paused reader can also get the fin
buffered after `res.end()`, and this change is what releases that
request. #34761 (pipelined successor's handler slot) is independent.

### Background

- Request body delivery: uws keeps one `HttpResponseData` per connection
with a single body-data handler slot. `NodeHTTPResponse` arms it with
`on_data_shim` (deliver to JS `ondata`) or, while the `IncomingMessage`
is paused, `on_buffer_paused_shim`, which appends chunks to
`buffered_request_body_data_during_pause`; JS later fetches that buffer
with `drainRequestBody()` from `IncomingMessage._read()`. uws clears the
slot itself after the chunk flagged as the fin.
- `body_read_state` is `None` (no body), `Pending` (uws may still call
back, or a buffered tail is still readable) or `Done`;
`IS_DATA_BUFFERED_DURING_PAUSE_LAST` records that the fin was among the
buffered chunks. `body_read_ref` is the event-loop keep-alive held while
chunks are still expected.
- `IS_REQUEST_PENDING` is a self-reference on the response box plus one
unit of the server's in-flight request count; `mark_request_as_done()`
drops both, and `should_request_be_pending()` decides whether an
ended/tunneled response may drop them yet. `server.close()` in
`node:http` completes through the native server's all-requests-done
promise, which is gated on that count.

<details>
<summary>Repro from the report (hangs on bun 1.4.0, exits with "server
closed" on Node and with this change)</summary>

```js
import { once } from "node:events";
import { createServer } from "node:http";
import { connect } from "node:net";
let n = 0;
const server = createServer((req, res) => { const i = ++n; if (i === 1) setTimeout(() => res.end("ok1"), 100); else res.end("ok2"); });
server.listen(0, "127.0.0.1"); await once(server, "listening");
const client = connect(server.address().port, "127.0.0.1");
let data = ""; client.on("data", d => (data += d)); await once(client, "connect");
const big = Buffer.alloc(70000, "x").toString();
client.write("POST / HTTP/1.1\r\nHost: x\r\nTransfer-Encoding: chunked\r\n\r\n" + big.length.toString(16) + "\r\n" + big + "\r\n3\r\nend\r\n0\r\n\r\n");
while (!data.includes("ok1")) await new Promise(r => setTimeout(r, 5));
client.write("GET / HTTP/1.1\r\nHost: x\r\n\r\n");
while (!data.includes("ok2")) await new Promise(r => setTimeout(r, 5));
client.destroy(); server.close(() => console.log("server closed"));
```

`BUN_DEBUG_NodeHTTPResponse=1` before: `onData(70000)` -> `doPause` ->
`onBufferRequestBodyWhilePaused(3, false)` ->
`onBufferRequestBodyWhilePaused(0, true)` -> `end('ok1')` ->
`onRequestComplete`, and no `markRequestAsDone()` for the first request.
After: `markRequestAsDone()` follows `onRequestComplete` directly, and
the later `drainBufferedRequestBodyFromPause 3` shows the tail still
being handed to JS.

</details>
@robobun

robobun commented Sep 13, 2026

Copy link
Copy Markdown
Collaborator Author

This bug showed up again during unrelated node:http work (2026-09-13). In this variant the handler calls res.end() from inside the first 'data' callback. 2000 of 3000 Content-Length bytes are still outstanding at that point. The client then sends a second request on the same keep-alive socket.

Repro
const http = require("http"), net = require("net");
const log = [];
const server = http.createServer((req, res) => {
  if (req.method === "GET") return res.end("second");
  let n = 0;
  req.on("data", c => {
    n += c.length;
    log.push("data total " + n);
    if (n === c.length) {
      res.writeHead(200, { "content-length": 5 });
      res.end("early");
      log.push("res.end()");
    }
  });
  req.on("end", () => log.push("end total " + n));
});
server.listen(0, "127.0.0.1", () => {
  const sock = net.connect(server.address().port, "127.0.0.1");
  let got = "", sent = false;
  sock.on("data", d => {
    got += d;
    if (!sent && got.includes("early")) {
      sent = true;
      setTimeout(() => sock.write(Buffer.alloc(1000, 97)), 30);
      setTimeout(() => sock.write(Buffer.alloc(1000, 98)), 60);
      setTimeout(() => sock.write("GET /second HTTP/1.1\r\nHost: x\r\n\r\n"), 90);
    }
    if (got.endsWith("second")) {
      console.log(log.join(", "));
      sock.destroy();
      server.close();
    }
  });
  sock.write("POST / HTTP/1.1\r\nHost: x\r\nContent-Length: 3000\r\n\r\n");
  sock.write(Buffer.alloc(1000, 99));
});
Runtime Output
Node v26.3.0 data total 1000, res.end(), data total 2000, data total 3000, end total 3000
Bun main (1.4.3-canary.1+b99371011) data total 1000, res.end(), end total 1000
This PR merged onto main f04caca same as Node
  • The script prints only after the client receives second. So all three serve the follow-up request on the same socket.
  • On main the uws parser still consumes the leftover bytes as body. It does not hand them to JS.
  • On main the early 'end' comes from write_or_end::<true>. It sets body_read_state to None while a reader is armed. The next _read() sees hasBody === none and emits EOF. The keep_reading_body check in this PR removes that.

The branch conflicts with main since #38207 landed. I merged it onto f04caca locally (not pushed) to run the script. Both conflicts are keep-both:

  • src/runtime/server/NodeHTTPResponse.rs, the clear branch of set_on_data: keep main's buffered_request_body_data_during_pause.clear_and_free(), then this PR's if stopped_reading_body { self.mark_request_as_done_if_necessary(); }.
  • test/js/node/http/node-http.test.ts: main added connectionListener queues pipelined responses like Node at the same insertion point. Keep both blocks.

Test results on that merge (debug build):

@robobun

robobun commented Sep 19, 2026 •

Copy link
Copy Markdown
Collaborator Author

Update: #43466 is closed without a merge. #43557 (merged as 5d5f03f) supersedes it, so the rebase note that was here does not apply any more.

Jarred-Sumner added a commit that referenced this pull request Sep 26, 2026
…inish, lifecycle) (#43557)

One pull request for the open `node:http` server pull requests. Each
root cause is fixed once, and each pull request's tests are carried
over. Node is the reference: every scenario was run under Node and under
Bun from one script, and the outputs were compared.

Fixes #4733
Fixes #18613
Fixes #40350
Fixes #43155
Fixes #43297
Fixes #43513
Fixes #43527
Fixes #25632
Fixes #31301
Fixes #43027
Fixes #43163
Fixes #43342
Fixes #43344
Fixes #43370
Fixes #43490
Fixes #43512
Fixes #43519

Each of these has a repro that is wrong on Bun 1.4.3, right on this
branch, and the same as Node.

| Issue | Not closed by this PR, because |
| --- | --- |
| #30501 (msal-node keeps Bun alive at exit) | Probably fixed. The repro
copies the teardown of msal-node. The package itself was not run. |
| #14430 (yarn: "does not support SSL") | Probably fixed.
`response.hasOwnProperty("socket")` is now `true`. yarn itself was not
run. |
| #39681 (`server.setTimeout` callback after destroy) | Probably fixed.
The repro is the deterministic case of #39686. The script in the issue
depends on timing and on Windows. |
| #43455 (`req.complete`, nine flows) | Partially addressed. Flows 1, 2,
3, 5 and 7 are fixed, and flow 6 was already right. Flow 8 (a socket
timeout while the body of an Upgrade request arrives) and flow 9 (a
request that `stream.pipeline()` destroyed never reports `complete`) are
not. Flow 4 differs only in `_readableState.ended`. |

### What changes for users

| Area | Before | After (same as Node) |
| --- | --- | --- |
| `req.pause()` | The socket stops at once. `req.complete` stays `false`
for a small body. | The body is received until the buffer is full. Then
the socket stops. |
| `res.end()` before the body arrives | `req` gets `'end'` and `'close'`
at once, and the body is lost | The request completes when its body
really ends |
| `res.destroy()` in the middle of a body | `'end'` with bytes missing |
`aborted`, then `ECONNRESET` |
| `socket.destroy()` inside the `'request'` listener | The body that
came with the head is dropped | That body is still delivered |
| `'finish'` and the `end()` callback | Fire when `end()` buffers the
bytes | Fire when the last bytes have left the socket |
| A response that closes the connection | The server half-closes and
waits for the peer | The socket closes right behind the FIN |
| `'drain'` after a later write flushed the backlog | Lost. `pipe(res)`
could hang. | Emitted |
| A pipelined request whose body continues after the previous response
ends | Body dropped, no response, `server.close()` hangs | Delivered |
| CONNECT and Upgrade tunnel sockets | Keep reading when paused or full
| Stop reading. `_read()` starts them again. |
| A tunnel write that waits for a drain when the client goes away | Its
callback, the callbacks of the writes behind it and the `end()` callback
never run | They run with an error before `'close'` |
| Upgrade request with a body, paused in its listener | The body flows
away | The request keeps its body |
| `ws` on a reused keep-alive socket | Writes after the Upgrade could
stall | Sent |
| A raw `socket.write()` behind a response that still drains (the 400
for a bad pipelined request, the reply of a `'clientError'` listener) |
Lands in the middle of that response | Sent after it |
| `server.close()` | Could report closed while connections were open |
Waits for every connection. An idle tunnel does not keep the process
alive. |
| `closeAllConnections()` on a listening server | Also stops the
listener and destroys tunnels and WebSockets | Destroys only the HTTP
connections |
| `Proxy-Connection: close` (node:http only) | Ignored. The connection
stays open. | Ends the connection, like `Connection: close` |
| A response larger than 16 KB, also in `Bun.serve` and over TLS | Up to
4 `send()` calls for each chunk. Slower than Node in most cases. | One
write for the writes of one tick. 1.1x to 2.7x the requests per second
of main, and faster than Node. |
| `socket.destroy()` and then `res.end()` in a listener | (this PR,
earlier) `req` ended as if it were complete | `'aborted'`, then
`ECONNRESET` |
| `emit('connection')` or http2 `allowHTTP1`: the response ends while
the listener still reads the body | The rest of the body is dropped |
The body is complete |
| `httpValidation: "relaxed"`, `Content-Length` or `Transfer-Encoding`
in trailers | Accepted | `HPE_INVALID_CONTENT_LENGTH`,
`HPE_INVALID_TRANSFER_ENCODING` |
| `req.complete` inside `'connect'`, and inside `'upgrade'` without a
body | `false` | `true` |
| `optimizeEmptyRequests`: `socket.parser.incoming` after the response |
Keeps the request alive on an idle connection | `null` |
| A HEAD or OPTIONS request with `Content-Length` | The body is dropped,
and `req.complete` is `true` before it comes | The request has its body
|
| `res.end(chunk)` after the client went away | `finished` and
`writableEnded` stay `false`, no `'prefinish'` | The response ends |
| An HTTP/1.0 request with an `Expect` header | `100 Continue`,
`'checkContinue'`, `'checkExpectation'` or a 417 | A plain `'request'` |
| The idle sweep of `close()` and `closeIdleConnections()` | Could
destroy a connection that was still receiving a request, or whose
response was still draining | Closes only idle connections |

### Design

| Piece | What it is |
| --- | --- |
| Request body state | `None / Pending / Complete / Aborted / Upgraded /
Detached`. Only the last chunk sets `Complete`. One function,
`leave_pending`, is the only other way out of `Pending`. |
| Read flow control | One path: `push()` returning false stops the
socket, `_read()` starts it. Both native pause buffers are removed: no
read is copied and replayed. |
| "This read is parsed" signal | `notifyWhenReadParsed()` sets a uws
state bit. uws delivers a `readParsed` event after the read. It replaces
a `setImmediate`. |
| Close during a parse | One uws bit defers a close to the end of the
current message. |
| Response finish | A response is finished when it has ended and the
socket has fully drained. |
| Idle connection | One rule, `HttpResponse::closeIfIdle()`. A
connection is idle when it receives no request (head or body) and no
response is in flight, queued or undrained. The sweep of `close()` and
`closeIdleConnections()` both use it. |
| Idle tunnel | A tunnel at read EOF with nothing left to send. uws
reports it to the server through the connection filter (`-3`, `+3`,
`-4`). It still counts for `'close'`, but it does not hold the event
loop, like a libuv handle in that state. |
| Server `'close'` | One native close promise per `listen()`. `close()`
records whether its sweep left nothing open. Then a `listen()` in the
same tick cannot hold `'close'` back, as in
`net.Server._emitCloseIfDrained`. |
| Raw socket writes | While uws holds response bytes (its buffer, the
zero-copy tail of a `res.write()`, the cork buffer), a raw write goes
through `AsyncSocket::write`, the path a 1xx line takes. So the order on
the wire is the order of the calls. |
| Upgrade verdict | One scanner and one verdict, shared by the parser
and the dispatcher. |
| llhttp | Updated from 9.3.0 to 9.4.2, as Node v26.5.0 vendors it, plus
one local patch (see below). Node v26.5.1 and later vendor 9.4.3. That
update is not in this PR. |

The parser changes also tighten request framing so that it agrees with
llhttp in more cases. There is no new API surface.

### A pause holds from the next read

The copy of the rest of a read (`nodeHttpPausedSpill`), its replay from
a posted task and the nested parse are removed. Like in Node, the rest
of the read that caused a pause is still parsed, and the socket stops at
the next read. usockets reads up to 512 KB in one call. libuv reads 64
KB.

| One paused, unread request (client sends 64 MB) | Bytes held |
| --- | --- |
| Node 25.6 | 131,018 |
| This pull request | 524,234 |

The price is in one case. A client sends 512 KB of small pipelined
requests (19,418 of them) and never reads. Each handler answers with its
own 64 KB body:

| Handler | Runtime | Requests dispatched | RSS |
| --- | --- | --- | --- |
| Answers at once | Node 26.3 | 2,425 | +177 MB |
| Answers at once | main | 41 | +9 MB |
| Answers at once | This PR | 2,425 | +171 MB |
| Answers one tick later | Node 26.3 | 4,850 | +336 MB |
| Answers one tick later | main | 2,426 | +181 MB |
| Answers one tick later | This PR | 4,850 | +330 MB |

Release builds on Linux x64. This PR now does what Node does. main held
fewer responses, mostly for a handler that answers at once. On macOS one
read can return all 512 KB. There, Bun 1.4.3 already reached +951 MB for
the handler that answers one tick later, and Node reached +1,294 MB.
`server.maxRequestsPerSocket` bounds it.

### Performance

#### Responses larger than 16 KB are faster, and now faster than Node

On main, a response that did not fit the 16 KB uWS cork buffer released
the cork. After that, each piece was its own `send()`: the buffered
head, the chunk-size line, the data, the `\r\n` and the last chunk. Over
TLS, each 2-byte piece was also its own record. Two changes fix that,
for `Bun.serve` and for node:http:

| Change | Effect |
| --- | --- |
| A write that does not fit goes out with the cork buffer and its
framing in one vectored write | No copy is added. Over TLS, the records
of all the pieces share the write batch that one `SSL_write` loop
already had. |
| The cork buffer holds 128 KB, up from 16 KB. Only a write of 16 KB or
less is copied into it, as before. | Several writes in one tick go out
in one write, like in Node. A longer write still goes out without a
copy. |

The bytes on the wire are the same. The vectored write uses `sendmsg()`
with the flags that `send()` uses.

Write syscalls for one response:

| Response | Node 26.3 | main | This PR |
| --- | --- | --- | --- |
| 4 x `res.write(16 KB)` | 1 | 16 | 1 |
| 40 x `res.write(2 KB)` | 1 | 16 | 1 |
| `res.end(64 KB)` | 1 | 2 | 1 |
| 256 KB file, `.pipe(res)` | 4 | 16 | 5 |

Throughput (req/s, the mean of 2 rounds). Node v26.3.0, main
`97246d044e`, this PR `fe0ed1fbea`, with the method below:

| Case | Node | main | This PR | main / Node | PR / Node | PR / main |
| --- | --- | --- | --- | --- | --- | --- |
| http, 4 x `res.write(16 KB)` | 17,735 | 6,983 | 19,160 | 0.39x | 1.08x
| 2.74x |
| https, 4 x `res.write(16 KB)` | 11,720 | 6,110 | 15,510 | 0.52x |
1.32x | 2.54x |
| http, 40 x `res.write(2 KB)` | 9,879 | 6,140 | 14,980 | 0.62x | 1.52x
| 2.44x |
| https, 40 x `res.write(2 KB)` | 6,981 | 5,541 | 11,780 | 0.79x | 1.69x
| 2.13x |
| http, `res.end(64 KB)` | 18,535 | 16,528 | 19,889 | 0.89x | 1.07x |
1.20x |
| http, 256 KB file `.pipe(res)` | 2,684 | 2,375 | 2,719 | 0.88x | 1.01x
| 1.15x |
| https, `res.end(64 KB)` | 11,894 | 14,586 | 16,426 | 1.23x | 1.38x |
1.13x |
| http, GET hello (control) | 55,994 | 70,989 | 71,958 | 1.27x | 1.29x |
1.01x |

main was slower than Node in six of these eight cases. This PR is faster
than Node in all eight.

`Bun.serve`, measured on `a771572a8d`, before the larger cork buffer
(req/s, the mean of 2 rounds):

| Case | main | PR | Change |
| --- | --- | --- | --- |
| Direct stream, 4 x 16 KB | 6,826 | 12,479 | +83% |
| 64 KB string | 16,944 | 20,145 | +19% |
| TLS, 64 KB string | 15,045 | 16,892 | +12% |
| hello (control) | 83,957 | 83,137 | -1.0% |

These runs are on loopback, where the kernel send buffer is 2.6 MB and
the work of the receiver runs inside `send()`. That is the best case for
fewer writes. A new connection over a real network takes about 46 KB in
its first write on Linux. The rest waits in the socket buffer, as it
would after separate writes.

#### Small responses are unchanged

A small response is already one `recvfrom` and one `sendto` on both
builds. `perf` puts 66% of the time of a hello-world server in the
kernel, on both builds.

CI release builds on Linux x64: main `97246d044e` (the merge base)
against this PR `8834cd0787`. Both use the same WebKit. The server runs
on one pinned core. `oha` sends 64 connections for 5 s after a 2 s
warm-up. There are 2 rounds, and the order of the builds alternates.
"Change" compares the means of the two rounds.

Framework servers from `bun-perf-tester` (req/s):

| Server | main, round 1 | main, round 2 | PR, round 1 | PR, round 2 |
Change |
| --- | --- | --- | --- | --- | --- |
| express | 49,803 | 51,103 | 49,968 | 50,263 | -0.7% |
| fastify | 61,214 | 61,105 | 60,705 | 60,668 | -0.8% |
| node:http | 70,934 | 71,405 | 73,178 | 71,053 | +1.3% |
| elysia | 84,696 | 85,096 | 85,027 | 84,882 | +0.1% |
| `Bun.serve` | 89,020 | 89,099 | 88,168 | 88,373 | -0.9% |

node:http paths that this PR changes (req/s):

| Case | main, round 1 | main, round 2 | PR, round 1 | PR, round 2 |
Change |
| --- | --- | --- | --- | --- | --- |
| GET hello | 70,226 | 70,341 | 70,151 | 72,359 | +1.4% |
| POST, 16 KB body | 47,446 | 47,789 | 48,191 | 48,785 | +1.8% |
| 64 KB response in four writes | 6,885 | 6,894 | 6,834 | 6,868 | -0.6%
|
| Pipelined keep-alive, depth 8 | 94,063 | 93,294 | 92,909 | 93,809 |
-0.3% |

p99 latency (ms), the higher of the two rounds:

| Server | main | PR |
| --- | --- | --- |
| express | 1.94 | 1.91 |
| fastify | 1.52 | 1.55 |
| node:http | 1.11 | 1.12 |
| elysia | 0.98 | 0.96 |
| `Bun.serve` | 0.82 | 0.83 |

RSS (MB), one pass of 8 s of load:

| Server | Build | Start | Under load | 5 s idle | 15 s idle |
| --- | --- | --- | --- | --- | --- |
| express | main | 39 | 94 | 61 | 57 |
| express | PR | 40 | 92 | 60 | 57 |
| fastify | main | 41 | 91 | 58 | 55 |
| fastify | PR | 41 | 91 | 58 | 55 |
| node:http | main | 20 | 64 | 43 | 40 |
| node:http | PR | 20 | 65 | 45 | 41 |
| elysia | main | 28 | 46 | 36 | 35 |
| elysia | PR | 29 | 46 | 37 | 36 |
| `Bun.serve` | main | 14 | 30 | 22 | 22 |
| `Bun.serve` | PR | 14 | 30 | 22 | 22 |

Every change is within 2%. fastify and `Bun.serve` hello are lower in
both rounds, by about 1%. `Bun.serve` hello shows the same -1.0% in the
control row above, so a small real cost there is possible. The RSS pass
ran at the same time as the throughput runs, on other cores. The commits
after `8834cd0787` change tests and add one version check to the
node:http dispatcher. They were not measured.

### Supersedes

| Theme | Pull requests |
| --- | --- |
| Request body | #43592 #43579 #38196 #43518 #43602 #43408 #43427 #43597
#43555 #43456 #43466 |
| Tunnels | #43570 #43485. #43596 is a duplicate of #43570. |
| Parser | #43182 #43161 #43326 #43327 #40505 #43363 #42532 #42194 |
| Response write | #39386 #43371 #43548 #43499 #43496 #43464 #42008
#43549 |
| Response finish | #40351 #43021 #41822 #43473 #42068 #35207 #43425
#43503 |
| Lifecycle | #43413 #39686 #43028 #42727 #42622 #42610 #35837 #35839
#37825 #37749 #43376 #35268 |
| JS API | #41691 #41738 #38036 #42462 #36527 #39718 #37964. #42947
merged on its own. |

The close drain, `resetAndDestroy()`, the pending write callback
handling and the response `'close'` ordering come from #42622 and #42727
by @steipete. The diagnosis and the tests for the stalled `ws` writes
come from his #42610.

Not included:

| Pull request | Reason |
| --- | --- |
| #33061 | main already enforces `headersTimeout` and `requestTimeout` |
| #41672 | It makes `http.createServer({ key, cert })` stop serving TLS.
That needs a product decision. |
| #37543 | A type refactor with no tests and no user-visible change |
| #35465 | It makes `http.Server` extend `net.Server`. Only the
prototype chains were joined. The `net.Server` constructor never ran, so
`_handle` and `_connections` were `undefined`, and
`_emitCloseIfDrained()` emitted `'close'` on a listening server. The
server is backed by uWS, not `node:net`. |
| The `AutoFlusher` removal in #42622 | It makes `flushHeaders()` flush
at once. That is a performance change with no relation to the rest. |

### Tests

| Check | Result on a debug build (macOS arm64) | Head |
| --- | --- | --- |
| Every test file that this PR touches (28 files) | 1,866 pass, 2 fail.
The 2 failures are `serve.test.ts` "bounds memory when proxying ... to a
stalled client". They fail the same way on a debug build of main. |
`83af4da4a3`, run before the last commit of main came in |
| `test/js/third_party/express` (9 files) and the `body-parser` test |
299 pass, 0 fail | `83af4da4a3`, run before the last commit of main came
in |
| Node 25.6 against Bun, 32 scenarios from two scripts (event order,
framing, lifecycle) | No regression against Bun 1.4.3 | `0ff1a23f61` |
| Every vendored Node `test-http-*` and `test-https-*` file, plus the
`test-net-*` and `test-tls-*` files for pause, write, end and close |
535 of 537 exit 0. `test-http-agent-keepalive.js` and
`test-https-timeout.js` fail on that debug build. Both pass on every CI
lane. | `daee05fcfd` (before the rebase) |
| The tests that depend on what the kernel takes in one send, on Windows
Server 2019 x64 and Windows 11 arm64 | pass | `3eef223328` (x64),
`a29289bcc1` (arm64) |
| CI build 120191 (Linux, macOS and Windows, release and ASAN) | every
lane passed | `daee05fcfd` (before the rebase) |

Each new test fails on Bun 1.4.3, or on the commit before its fix for a
fault that this branch introduced.

The two tests over the limit are `node-http-connect.test.ts` ("tests
should run on bun") and `node-http-syscall-fault.test.ts` ("racing a
queued drain"). Each starts a debug subprocess that needs more than 5 s
on this machine. Both pass on CI.

### Changes in the last push

The branch is rebased on main (`daee05fcfd` was the head before). It is
now linear.

Four regressions against main, each with a test that fails without its
fix:

| Case | main | Before this push | Now (same as Node) |
| --- | --- | --- | --- |
| `emit('connection')` or http2 `allowHTTP1`: `res.end()` on a request
that nobody reads | `'end'`, `'close'` | No events | `'end'`, `'close'`
|
| The same server, an unread 32 MB body | 0 bytes held | 32 MB held | 0
bytes held |
| A NUL in a header value with `httpValidation: "relaxed"` (client,
`HTTPParser`, `emit('connection')` server) | Accepted | The process
spins forever | `HPE_INVALID_HEADER_TOKEN` |
| `Connection: close`, body in the same read as the head, a 20 KB
response before the body is read | `'end'` with an empty body | No
events on `req` | `'end'` with the body, `'close'` |
| An empty line on an idle keep-alive connection, then `server.close()`
| 0 s | About 6 s | 0 s |

| Fix | Where |
| --- | --- |
| The finish listener of a fallback connection dumps an unread request,
like Node's `resOnFinish` | `http1_server_fallback.ts` |
| llhttp patch: `llhttp__internal__c_test_lenient_flags_20` is false for
a NUL. The relaxed state does not consume a NUL, and the next state sent
it back there. 9.4.3 has the same loop. | `llhttp.c`, noted in its
`README.md` |
| A node:http socket that `onData` is parsing gets the close gate of
`onData`, also when a large write released the cork | `HttpResponse.h`
`uncorkCompletedResponse()` |
| A read that starts no message leaves an idle connection idle |
`HttpContext.h` `onData` |

The open review threads are fixed in `8834cd0787`:
`closeAllConnections()`, `Proxy-Connection: close`, five comments cut to
one line, and the test of two overlapping listeners, which now waits on
events. With the generation gate in `emitCloseServer` removed, that test
fails in both cases. `AsyncSocketData` keeps its bools together, which
takes it from 56 to 48 bytes per socket.

`http.Server` no longer extends `net.Server` (see "Not included"). The
special case for it in `Ipc.ts` is gone too. `child.send(msg,
httpServer)` still throws `ERR_INVALID_HANDLE_TYPE`, and its test stays.

<details><summary>Changes since the first revision (2988a61)</summary>

Merged with main at `c8e1f6fa5b`. The one conflict was #43708
(`req.socket` emits `'end'` and `'error'`). Its state bit
`HTTP_NODE_PEER_ENDED` moved to bit 22, because bit 19 is
`HTTP_NODE_NOTIFY_READ_PARSED` here. Its 15 tests run in
`node-http-server-abort-events.test.ts` next to the tests of this branch
(103 pass).

CI on `2988a610c` had ten red tests from four causes. They are fixed:
- `ed882e5e99`: `write()` to a response without a body (HEAD, 204) does
not wait for unsent bytes.
- `811f817704`: an idle tunnel does not hold the event loop after
`server.close()`. Four vendored Node tests timed out on every platform.
- `0697deec11`, `d428824c08`, `7aec062d14`: the write callback tests use
a body that backs up a loopback socket, and accept what Winsock does.
- `c7af1c1615`: two tests from main asserted the old `close()` contract.

Review findings, each reproduced against Node v26.3.0 and fixed with a
test that fails without the fix:
- `d4d2b783ea`, `2cc79939bb`, `45141abca9`, `9a63dbc470`, `80a23a1822`:
the idle rule. A keep-alive connection is idle again when its body ends
after its response. A connection that owes a queued pipelined response,
that still receives a request head or body, or whose response still
drains is not idle.
- `87d8904aa3`, `7eca4f7806`: `close(cb)` followed by `listen()` in the
same tick reports `'close'`, also for an https server whose only
connection was idle.
- `3fd7b25382`: a paused pipelined request behind a response that still
drains stops the connection. The first revision read 512 MiB of 512 MiB
into memory.
- `2d1a5e2d8d`, `33e4683a8a`, `539cb41eb9`, `197c7dfc29`, `0490e3540b`:
raw socket writes stay behind every unsent response byte. The cases were
a CONNECT pipelined behind a response that still drains (its `200`
landed at offset 2.6 MB of a 64 MiB body), a zero-length tunnel write
(it hung the tunnel), the 400 replies above, the zero-copy tail of a
large `res.write()`, the cork buffer, and Windows 11, where the kernel
takes the whole response and refuses the next send.
- `0abd39c133`: an upgrade from the request's `'end'` listener keeps the
body bytes out of the WebSocket. The connection closed with 1006 right
after the 101.
- `5aafc61aa6`: the callback of a small `res.write()` that the kernel
refuses at the uncork runs on the drain. Reproduced on Windows 11 only.
- `a29289bcc1`: a tunnel write that waits for a drain settles its
callbacks when the connection closes.

Known differences from Node that this PR leaves:
- A handler that calls `res.end()` and then `server.close()` closes its
keep-alive connection at once. Node waits for the `keepAliveTimeout`.
- A pipelined Upgrade behind a response that still drains is served as a
plain request.
- An `end()` on a tunnel with no write pending, while the response
before the CONNECT still drains, closes both directions after the flush.
Node half-closes.
- A raw `req.socket.write(big)` and `req.socket.end()` with no
`res.end()` sends every byte but no FIN. main loses bytes here.
- A CONNECT socket that is given back with `server.emit('connection',
socket)` answers only the first of several pipelined requests. main
answers none.
- A large write from an `'upgrade'` listener stalls while the body of
that Upgrade request is still pending. A second `listen()` on a
listening server does not throw. Both are the same on main.
- A tunnel write that fails because the client went away fails its
callbacks but emits no `'error'`. Node emits `ECONNRESET`. A new
`'error'` could end a process that has no listener for it.

</details>

<!-- robobun:evidence:begin -->

---

**no test proof** · iteration 8 · platform-specific test(s) that do not
run on this machine, deferring to CI, which covers all platforms:
test/js/web/fetch/fetch.stream.test.ts, test/js/node/url/url.test.ts,
test/js/node/tls/tls-syscall-fault.test.ts,
test/js/node/net/node-net-server.test.ts,
test/js/node/http/node-http.test.ts,
test/js/node/http/node-http-syscall-fault.test.ts,
test/js/node/http/node-http-server-close-drain.test.ts,
test/js/node/http/node-http-connect.test.ts,
test/js/node/http/node-http-backpressure.test.ts,
test/js/node/child_process/child_process_ipc_handle.test.ts,
test/js/bun/http/serve.test.ts,
test/js/bun/http/serve-syscall-fault.test.ts,
test/js/bun/http/bun-server.test.ts

<!-- robobun:evidence:end -->

---------

Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
@Jarred-Sumner

Copy link
Copy Markdown
Collaborator

Superseded by #43557, which is merged (5d5f03f). It fixes this once for the whole node:http server and carries the tests over.

@robobun

robobun commented Sep 26, 2026

Copy link
Copy Markdown
Collaborator Author

Confirmed: #43557 (5d5f03f) is on main and covers this change, tests included. Nothing further to do here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

http body won't be received if res.end is called too early node:http IncomingMessage stream data cannot be read, events are not emitted

2 participants