node:http: complete CONNECT and Upgrade requests at dispatch, deliver HEAD and TRACE bodies, clear parser.incoming on finish - #43461
Conversation
… HEAD and TRACE bodies, clear parser.incoming on finish
|
Warning Review paused — included plan limit reachedKeep your review moving with free on-demand reviews.
On-demand reviews are free for one more day.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Promotion and pricing detailsOn-demand reviews are free for one more day. After that, they cost $0.25 per reviewed file. Review limit detailsOr wait 3 minutes for your next included review. Limit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Comment |
|
I worked on item 1 of #43455 at the same time, on the branch Set the flag together with the EOF push. The 'connect' and 'upgrade' lines of this PR are the same as the first version of my branch. A review of that version found a state that Node cannot produce. For a CONNECT request with The branch does this instead:
Results, compared with Node v26.3.0:
The commit has two tests in
Both tests fail on bun 1.4.3-canary and pass as a script on Node v26.3.0. With the change, |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked two things this diff raises and ruled them out: a HEAD/TRACE request that declares Content-Length but never sends the bytes now leaves req waiting for 'end' instead of ending at dispatch — that matches Node, where llhttp frames the body by the headers for every method; and the parser.incoming overwrite on pipelined dispatch (_http_server.ts:734) is pre-existing and unchanged, the new clearing in emitResponseFinish only nulls it when it still points at the finishing request.
Extended reasoning...
The Rust change removes the method gate so has_body is derived from Content-Length/Transfer-Encoding alone, which is what llhttp does; the consequence that a HEAD with a declared-but-unsent body stalls the request stream is Node-consistent rather than a regression introduced here. The parser.incoming clearing is guarded by identity (parser.incoming === req), so it cannot drop a later pipelined request that has already replaced it; the existing line-734 overwrite behavior is unchanged by this PR. The confirmed inline finding about req.complete remaining false for optimizeEmptyRequests requests is the substantive open item, so a human should still weigh in.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/js/node/_http_server.ts— With optimizeEmptyRequests, users still read req.complete === false after the response finishes, while Node reports true; this PR now also drops parser.incoming for that same request at _http_server.ts:2387-2389, so the object looks finished to the parser but unfinished to the user. _dumpAndCloseReadable (_http_incoming.ts:745-752) marks the readable ended and closed but never sets complete, and the pre-dumped request never reaches emitEOFIncomingMessageOuter where complete is set (src/js/internal/http.ts:122). Fix: set req.complete = true wherever the message is known fully parsed at dispatch (the pre-dump at :945 and the no-body dispatch path), not only the CONNECT/upgrade branches this PR touched.Extended reasoning...
Server created with optimizeEmptyRequests: true. A GET without Content-Length or Transfer-Encoding dispatches; _http_server.ts:940-946 calls http_req._dumpAndCloseReadable(). readableEnded becomes true (endEmitted at _http_incoming.ts:748), complete stays false from the constructor (:82). Handler ends the response; emitResponseFinish at :2387 sees readableEnded true and nulls parser.incoming. Any code reading req.complete afterwards (or the PR's own complete.js script, whose item for the normal body-less path stays false) sees false. Node's parserOnMessageComplete sets complete before the response ever finishes. The PR sets complete on the CONNECT (:760) and upgrade (:980) hand-offs using the same 'llhttp completes at end of headers' rule but not on this sibling site, the exact 'fix the whole class' gap. The dismissing finders deferred to #43456, an unmerged claim; if it lands first it gates on hasBody and does not cover the pre-dumped request either. Population: every request on an optimizeEmptyRequests server, at request rate. Remedy: set http_req.complete = true next to…
Verification: pre-existing. Trigger: a server created with
optimizeEmptyRequests: truereceives a request with neither Content-Length nor Transfer-Encoding. Mechanism verified:/home/claude/bun/src/js/node/_http_server.ts:940-946callshttp_req._dumpAndCloseReadable(), which at/home/claude/bun/src/js/node/_http_incoming.ts:745-752sets_readableState.ended/endEmitted/destroyed/closed/closeEmitted…
|
On the optimizeEmptyRequests finding: |
|
Updated 10:21 AM PT - Sep 19th, 2026
✅ @robobun, your commit 0124bb161ace008f568381463041429753c50d6d passed in 🧪 To try this PR locally: bunx bun-pr 43461That installs a local version of the PR into your bun-43461 --bun |
|
I worked on the HEAD and TRACE body (item 2 of #43455) at the same time, on the branch That branch removes the method gate in Two differences from this PR:
I also compared the native change with Node v26.3.0 for HEAD and TRACE in these flows. Each result is the same as Node's:
An Upgrade request with a body now gives the same result for HEAD and TRACE as for GET and POST. The bytes after the body reach the socket as data, not as the |
|
This PR and #43555 edit the same condition in
The combined condition is: read the framing headers for every method except CONNECT. let method = HttpMethod::which(request_ref.method()).unwrap_or(HttpMethod::OPTIONS);
if method != HttpMethod::CONNECT {
*has_body = req_len > 0 || request_ref.has_transfer_encoding();
}The PR that lands second must resolve the conflict this way. This PR also deletes the One more point, from the two diffs and not from a run of this branch: the |
…g received (#43561) ### Problem - A request whose body stalls behind a pending pipelined response never gets 'timeout'. Its `req.setTimeout(ms, cb)` callback never runs and the server destroys the socket. Node v26.3.0 emits 'timeout' on it (#43455, item 6). - `onNodeHTTPServerSocketTimeout` (`src/js/node/_http_server.ts:221`) reads `socket[kRequest]`. That slot held the request whose response owns the socket. With pipelining that request is already complete. ### Fix - The dispatcher sets `kRequest` for every request, and `advanceResponsePipeline` no longer overwrites it. `detachSocket` still clears it when that request's response detaches. - The handler does not read Node's `parser.incoming`. That slot must outlive the response, and a request that `stream.pipeline()` destroyed never ends. In the first version of this PR it kept getting 'timeout' and held the idle socket open. - Self-reviewed: 5 concerns, 2 closed by probes, 3 left to separate work (Notes). Two edges are new. A pipelined request now gets 'timeout' while paused with its whole body received (#43557), or after `pipeline()` destroyed it. A request that is not pipelined already does. - Verified: three new tests in `test/js/node/http/node-http-server-timeouts.test.ts`. Two fail on 1.4.3-canary, the third on the first version. Also the timeout and pipelining fixtures, and `node-http.test.ts`. ### Background - `req.setTimeout` and `server.timeout` arm one inactivity timer per socket. The server forwards its 'timeout' to the request, the response and the server. With no listener, it destroys the socket. - With pipelining, the next request arrives before the previous response ends. Later responses wait for the socket. - `stream.pipeline()` destroys a failed server request with `req.socket = null`, so the connection survives for the error response. <details><summary>Notes</summary> #### Repro From #43455, item 6: `GET /a` is never answered, `POST /b` with `Content-Length: 100` sends 10 bytes, and only the POST calls `req.setTimeout(200, cb)`. ``` node v26.3.0: ["POST /b 'timeout' (complete=false)"] 1.4.3-canary: ["client socket closed by the server"] this branch: ["POST /b 'timeout' (complete=false)"] ``` #### The first version, and why `kRequest` stays The first version (cfc4149) made the handler read `this.parser?.incoming`, like Node's `socketOnTimeout`, and removed `kRequest`. The review found a regression against the base. An upload handler calls `req.setTimeout(ms, cb)` and `stream.pipeline(req, dest, cb)`. The destination fails, so `pipeline()` destroys `req` with `req.socket = null`. `_destroy` releases the native handle, so `req.complete` never becomes `true`, and 'end' never fires. The handler answers 500, the client sends the rest of the body and idles. The destroyed request stayed in `parser.incoming`, got 'timeout' at the keep-alive timeout, and its listener kept the idle socket open. Node and the base close that socket. `parser.incoming` cannot be cleared when the response detaches. `test-http-server-keepalive-end` reads it inside an 'end' listener after a synchronous `res.end()`, and expects the request there. So the timeout target needs its own slot with its own end of life, and that is what `kRequest` is. 0e3a237 restores it and fixes its lifecycle for pipelining. The result for a connection without pipelining is the same as on main by construction: set at dispatch, cleared at the detach of that response. #### Probe, 15 scenarios Who sees 'timeout', and does the server close the socket. Each build is compared with Node v26.3.0. The first version and the current version give the same results here. | Scenario | 1.4.3-canary | This branch | | --- | --- | --- | | Stalled POST pipelined behind a pending GET, only the POST listens | differs | same as Node | | Same wire, listeners on both requests, both responses and the server | differs (no `req POST /b`) | same as Node | | Two pending GETs, then a stalled POST | differs | same as Node | | Early `res.end()` for a POST whose body never arrives, then the keep-alive timeout | differs | differs | | Single complete POST that the listener paused | differs | differs | | Pipelined complete POST that the listener paused | same as Node | differs | | Nine more (see below) | same as Node | same as Node | The nine: single stalled POST, single complete GET, single complete POST (read, and unread), pipelined complete GET, pipelined complete POST (read, and unread), first response answered then stalled POST, `optimizeEmptyRequests`. More scenarios that match Node on this branch and differ on the canary: an HTTPS server, `server.setTimeout(ms)` with a request listener that keeps the socket, a chunked POST that stalls inside a chunk, a pipelined request that `maxRequestsPerSocket` drops ('dropRequest'), a pipelined `Expect: 100-continue` request in 'checkContinue', a pipelined POST larger than the high water mark that nobody reads, and a pipelined POST that gets 'timeout', keeps the socket, then completes (the client receives both responses, the canary never answers). #### A request that `stream.pipeline()` destroyed | Flow | Node v26.3.0 | 1.4.3-canary | This branch | | --- | --- | --- | --- | | Response finished, the rest of the body arrives | closes the idle socket | closes | closes | | Response finished, the body stalls | 'timeout' on the request | closes | closes | | Response pending, pipelined, the rest arrives | socket destroyed | socket destroyed | 'timeout' on the request | | Response pending, pipelined, the body stalls | 'timeout' on the request | socket destroyed | 'timeout' on the request | | Response pending, not pipelined, the rest arrives | socket destroyed | 'timeout' on the request | 'timeout' on the request | | Response pending, not pipelined, the body stalls | 'timeout' on the request | 'timeout' on the request | 'timeout' on the request | With a pending response, a pipelined request now behaves like a request that is not pipelined. A `!req.destroyed` check in the handler fixes the "rest arrives" rows and breaks the "stalls" rows, so it is not in this PR. The cause is that `req.complete` never becomes `true` after `_destroy` releases the handle. #### The differences that remain all come from `req.complete` - Early `res.end()`: Bun ends a request when its response ends, whether the listener reads `req` or not. `req.complete` is `true` and 'end' fires while the declared body is still missing. Node keeps `complete === false` until the parser finishes the message. This is tracked separately. - Paused POST: the native handle keeps the body and its end to itself until `req` resumes, so `req.complete` stays `false` (item 3 of #43455, fixed in #43557). A check of the native body state in the handler would hide this for 'timeout' only. The other readers of `req.complete` would keep the difference. - Destroyed request: see the table above. #### Self-review, the 5 concerns 1. Does the slot keep the last request alive on an idle keep-alive connection? No. A probe with `FinalizationRegistry` and forced GC gives the same result on the canary and on this branch for six request shapes, and no symbol slot of the socket holds the request. Only the `optimizeEmptyRequests` request stays reachable through `parser.incoming`, on both (item 5 of #43455, fixed in #43461). 2. Do the new tests depend on how the wire data is split across reads? No. One write, three writes, one byte per write, and a split inside the POST head all give the same events, under Node and on this branch. The new tests also passed every stress run on the debug build at a host load above 100. 3. A pipelined request that is paused after its whole body arrived now gets 'timeout'. Left to item 3 of #43455. 4. An early `res.end()` ends the request in Bun, so that request still gets no 'timeout'. Left to separate work. 5. The 'connect' and 'upgrade' hand-off removes the 'timeout' listener even while the body of an Upgrade request still arrives. Node keeps its listener until that body ends. This is the same before and after this change. Left to separate work. #### Review findings - Regression with a request that `pipeline()` destroyed: fixed, see above. - Two code comments were longer than one line: one is now one line, the other is back to the text on main. - The third test passed on the base and could pass on an early close: it now uses the pipelined shape and asserts `Connection: keep-alive` on both responses. It still passes on the base. It fails on the first version, and it guards the reason `kRequest` stays. - Two optional findings (paused request, destroyed request with a pending response) are the `req.complete` edges above. No change here. Seen on the way, not related to this change: when the socket is destroyed, a queued pipelined response emits 'close' before the socket's 'close'. Node emits it after. The canary and this branch agree with each other. Merge check: `git merge-tree` of this branch with #43456, #43461 and #43557 reports no conflicts. #### Suites run with the debug build - `test/js/node/http/node-http-server-timeouts.test.ts`, `node-http.test.ts`, `node-http-server-abort-events`, `node-http-req-socket-pause`, `node-http-transfer-encoding`, `node-http-uaf` - `test/js/node/test/parallel`: `test-http-set-timeout-server`, `test-http-set-timeout`, `test-http-timeout`, `test-http-timeout-overflow`, `test-http-outgoing-settimeout`, `test-http-server-consumed-timeout`, `test-http-server-keepalive-end`, `test-http-server-keep-alive-timeout`, the four `test-http-keep-alive-timeout*`, `test-http(s)-server-close-destroy-timeout`, the seven `test-http-server-request-timeout-*`, the four `test-http-server-headers-timeout-*`, `test-https-server-headers-timeout`, the nine pipelining fixtures (`test-http-pipeline-*`, `test-http-get-pipeline-problem`, `test-http-incoming-pipelined-socket-destroy`, `test-http-keep-alive-pipeline-max-requests`, `test-http-many-ended-pipelines`), and the fallback fixtures (`test-http2-allow-http1`, `test-http2-https-fallback*`, `test-http-generic-streams`, `test-http-insecure-parser-per-stream`, `test-http-max-header-size-per-stream`, `test-http-server-unconsume-consume`) - `test/js/node/test/sequential/test-http-server-request-timeouts-mixed.js` - `test/js/bun/test/parallel/test-http-should-emit-timeout-event.ts`, `test-http-should-emit-timeout-event-when-using-server-setTimeout.ts`, `test-http-timeout-destruction-should-be-visible-using-kConnectionsCheckingInterval.ts` </details> <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 1 · 2 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 2 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/node/http/node-http-server-timeouts.test.ts bun test v1.4.3 (367d939) test/js/node/http/node-http-server-timeouts.test.ts: (pass) node:http server timeout enforcement > headersTimeout closes a connection that never completes its request headers [776.10ms] (pass) node:http server timeout enforcement > requestTimeout closes a connection that stalls mid-body [436.27ms] (pass) node:http server timeout enforcement > server.setTimeout() fires the 'timeout' event for an inactive connection [327.54ms] (pass) node:http server timeout enforcement > keepAliveTimeout closes an idle keep-alive connection after the response [1383.04ms] (pass) node:http server timeout enforcement > emits 'clientError' once per stalled request when the listener keeps the socket open [1065.35ms] (pass) node:http server timeout enforcement > headersTimeout answers 408 when there is no 'clientError' listener [350.55ms] (pass) node:http server timeout enforcement > requestTimeout does not fire while a slow handler streams a response [612.56ms] 312 | const cl ... (truncated) release without fix: all passed bun test v1.4.3-canary.1 (b3bf769) test/js/node/http/node-http-server-timeouts.test.ts: (pass) node:http server timeout enforcement > headersTimeout closes a connection that never completes its request headers [265.18ms] (pass) node:http server timeout enforcement > requestTimeout closes a connection that stalls mid-body [354.35ms] (pass) node:http server timeout enforcement > server.setTimeout() fires the 'timeout' event for an inactive connection [204.31ms] (pass) node:http server timeout enforcement > keepAliveTimeout closes an idle keep-alive connection after the response [1204.71ms] (pass) node:http server timeout enforcement > emits 'clientError' once per stalled request when the listener keeps the socket open [1006.91ms] (pass) node:http server timeout enforcement > headersTimeout answers 408 when there is no 'clientError' listener [254.45ms] (pass) node:http server timeout enforcement > requestTimeout does not fire while a slow handler streams a response [406.27ms] (pass) node:http server timeout enforcement > a pipelined request that is still being received gets 'timeout' and can keep the socket [204.20ms] (pass) node:http server timeout enforcement > wi ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/node/http/node-http-server-timeouts.test.ts bun test v1.4.3 (367d939) test/js/node/http/node-http-server-timeouts.test.ts: (pass) node:http server timeout enforcement > headersTimeout closes a connection that never completes its request headers [836.05ms] (pass) node:http server timeout enforcement > requestTimeout closes a connection that stalls mid-body [415.19ms] (pass) node:http server timeout enforcement > server.setTimeout() fires the 'timeout' event for an inactive connection [349.50ms] (pass) node:http server timeout enforcement > keepAliveTimeout closes an idle keep-alive connection after the response [1430.25ms] (pass) node:http server timeout enforcement > emits 'clientError' once per stalled request when the listener keeps the socket open [1124.03ms] (pass) node:http server timeout enforcement > headersTimeout answers 408 when there is no 'clientError' listener [347.78ms] (pass) node:http server timeout enforcement > requestTimeout does not fire while a slow handler streams a response [594.68ms] (pass) node:http s ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 1054ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/126] gen generated_host_exports.rs generated_host_exports.rs: 121 exports (host=5, lazy=10, generic=106, rust=0); 245 extern-C blocks audited [2/126] gen JS modules (bundle-modules) Preprocess modules (8567ms) Bundle modules (102ms) Postprocesss modules (307ms) Bundle Functions (607ms) Generate Code (64ms) [9.66s] Bundled "src/js" for production 2606 kb 197 internal modules 13 native modules 50 internal functions across 16 files [2/9] cargo bun_runtime → libbun_runtime.a �[1m�[33mwarning�[0m�[1m: binary `bun_shim_impl` should have a kebab-case name�[0m �[1m�[94m|�[0m �[1m�[94m 1�[0m �[1m�[94m|�[0m /workspace/bun/build/release/rust-target/.../bun_shim_impl �[1m�[94m|�[0m �[1m�[33m^^^^^^^^^^^^^�[0m �[1m�[94m|�[0m �[1m�[94m= �[0m�[1mnote�[0m: `cargo::non_kebab_case_bins` is set to `warn` by default �[1m�[96mhelp�[0m: to change the binary name to `bun-shim-impl`, convert `bin.name` �[1m�[94m--> �[0msrc/install/windows-shim/Cargo.toml:41: ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/js/node/_http_server.ts | 6 +- .../js/node/http/node-http-server-timeouts.test.ts | 136 +++++++++++++++++++++ 2 files changed, 138 insertions(+), 4 deletions(-) ``` </details> **gate history** · 2 passed · 0 rejected · iteration 1 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/js/node/_http_server.ts 11 12 22 test/js/node/http/node-http-server-timeouts.test.ts 3 3 22 ``` </details> <!-- robobun:evidence:end -->
|
Update to my note above: it no longer applies. #43557 is merged (5d5f03f), and #43555 is closed. On main, I built main and checked two of the changes of this PR there:
I did not check the other changes of this PR against main. |
This branch has not been deployed
1 participant
Problem
node:httpserver, three flows report the completion of a request message differently from Node v26.3.0. Items 1, 2 and 5 of node:http server: req.complete and request completion still differ from Node (flows 4, 8 and 9 remain) #43455.req.completeisfalseinside a 'connect' or 'upgrade' listener. The two hand-off paths ofonNodeHTTPRequest(src/js/node/_http_server.ts) never set it.NodeHTTPResponse__createForJS(src/runtime/server/NodeHTTPResponse.rs:2601) reads the framing headers only whenmethod.has_request_body() || method == GET. HEAD and TRACE dispatch withhasBody === false, and the body bytes never reachreq. Node delivers them.optimizeEmptyRequests: true,socket.parser.incomingkeeps the last request alive while the keep-alive connection idles. OnlyemitEOFIncomingMessageOuterclears it, and a pre-dumped request never reaches that function.Fix
req.complete = truebefore the emit. The 'upgrade' path does the same for a request without a body. llhttp completes both at the end of the headers, so Node's listeners seetrue.NodeHTTPResponse__createForJSreadsContent-LengthandTransfer-Encodingfor every method. The uWS parser already consumes the body for every method, so the bytes now flow toreqinstead of being dropped.emitResponseFinishclearsparser.incomingwhen the request already ended, like Node'sclearIncominginresOnFinish. A request that ends later is still cleared byemitEOFIncomingMessageOuter.test/js/node/http/node-http.test.ts(all fail on 1.4.3, all pass under Node v26.3.0). Alsonode-http-connect,node-http-with-ws,node-http-server-abort-events,node-http-server-timeouts,node-http-transfer-encoding,node-http-backpressure, and the upgrade, connect, HEAD and optimizeEmptyRequests fixtures intest/js/node/test/parallel.Background
hasBodyis computed natively once per request. It decides whether the request holds abody_read_refon the event loop and whetherhandle.ondatafeedsreq.req.completeis Node's "message fully parsed" flag. Node sets it inparserOnMessageComplete, which llhttp calls at the end of the headers for CONNECT and for an Upgrade without a body.socket.parserin Bun is a shim that mirrors Node's parser surface. Itsincomingfield is the last dispatched request.reqis paused, lazy EOF push) follow the current design and are not changed here.Notes
Results of the script from #43455 (
complete.js) on this branch:HEAD / HTTP/1.1withContent-Length: 5and the 5 bytes:reqnow delivershello(was an empty body).#43456 sets
req.completeon the normal dispatch path for a request without a body. It relies onhasBody, so with this change it no longer reportstrueearly for HEAD and TRACE with a declared body. The two changes touch different lines.node-http-connect.test.ts: "should handle backpressure" and "tests should run on bun" time out at 5 s in this ASAN debug build on main too (64 MB through a tunnel).