Repository navigation
Conversation
…nnel req.destroy() of an accepted CONNECT or Upgrade request called abort() on its native response. The response of a handoff is never written, so abort() ended it on the raw socket. The peer got "HTTP/1.1 200 OK\r\nConnection: close\r\n\r\n" behind the reply of the listener, or as the answer to a tunnel that the listener refused. abort() now returns for a tunneled response. IncomingMessage._destroy destroys the socket right after, as Node.js does.
Node emits 'upgrade' from onParserExecuteCommon, after its parser has consumed the read. Bun emitted it from the end-of-headers dispatch, before uWS parsed the body of the request. When bytes follow the head of an accepted Upgrade request with a body, the dispatcher parks the handoff on the server socket. uWS stops the parse behind a body that ends in the read and runs the handoff once, inside the parse window. A complete request gets the raw socket, req.complete === true, its trailers, and the rest of the read as head. An incomplete body keeps the coupled stream, with the part that arrived already in req. The after-read callback moves from the response to the server socket. The route through the ondata of the response is removed. The shut-down gates of HttpContext::onData treat an accepted Upgrade request like a tunnel, so socket.end() in the listener no longer stops the reads of its body and of its tunnel.
|
Updated 11:49 AM PT - Oct 17th, 2026
✅ @robobun, your commit 53bc9dfb3b35ca5bad3a92e6a0fd90abe1852d85 passed in 🧪 To try this PR locally: bunx bun-pr 45058That installs a local version of the PR into your bun-45058 --bun |
|
Status: ready for review. CI is green on 53bc9df. The branch contains main How it was reproduced: the script under Notes in the description. An Pull request: #45058 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughNode HTTP Upgrade handling now separates request-body parsing from trailing tunnel bytes. Parser callbacks carry the unread head and completion state. The server parks applicable upgrade requests until the current read is parsed, then hands off the remaining bytes. ChangesNode HTTP Upgrade Body Handoff
Suggested reviewers: Priority: ➖ Normal Merge Risk: 🔵 Low · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Title check | |
The title clearly and concisely describes the main change: aligning node:http Upgrade requests with a body to Node v26 behavior. |
| Description check | |
The description explains the problem, implementation, limitations, and verification results. It does not use the exact template headings, but it provides the required change summary and test verificat… |
Full details: Description check
-
Explanation
The description explains the problem, implementation, limitations, and verification results. It does not use the exact template headings, but it provides the required change summary and test verification in sufficient detail.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![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 findings, I also checked the us_socket_ref/unref balance around the new parked-callback branch in HttpContext::onData (an upgrade or close inside the callback rewrites result so the normal returnedData unref runs; the complete branch unrefs once, matching a fully parsed read), that functionToCallOnReadParsed is declared, visited in visitChildrenImpl, and cleared in detach() in the same change, and that no stale callers of the removed Bun__NodeHTTPResponse_onReadParsed/notify_when_read_parsed remain.
Extended reasoning...
The change reworks node:http Upgrade-with-body handling across uWS HttpContext.h, the C++ server-socket binding, Rust NodeHTTPResponse, and _http_server.ts; it touches no auth or crypto surface but does touch socket ref counting and a parser callback buffer. Four confirmed findings are posted inline, so the note records only what else was ruled out.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/jsc/bindings/node/JSNodeHTTPServerSocket.cpp— A server whose body handler upgrades the socket with the builtin ws before 'upgrade' is emitted keeps the dispatch promise pending for the whole WebSocket lifetime and then emits 'upgrade' on a dead socket. onUpgraded at JSNodeHTTPServerSocket.cpp:780-784 leaves functionToCallOnReadParsed set; HttpContext.h:738 skips the parked callback because upgradedWebSocket == s, so it only fires from onClose at line 858 when the WebSocket closes, with an empty head and complete=false. Fix: onUpgraded must settle the parked handoff at once (run it with complete=true, or clear it and resolve the promise) so no dispatch stays pending past the adoption.Why this was flagged
Trigger: an Upgrade request with a body arrives in one read; a 'connection' or shouldUpgradeCallback hook attaches req 'data'/'end' listeners and calls wss.handleUpgrade() from them. The dispatcher parked the request at _http_server.ts:1502 and stored the callback. The body handler adopts the socket at HttpResponse.h:504; onUpgraded (JSNodeHTTPServerSocket.cpp:780) sets upgraded but does not touch functionToCallOnReadParsed. HttpContext.h:684 returns nullptr and line 738 skips the callback since upgradedWebSocket == s. The promise from parkUpgrade stays unresolved until the WebSocket closes, when line 858 calls onReadParsed({}, false): handOffParkedUpgrade then emits 'upgrade' with a destroyed socket and an incomplete req. For long-lived WebSocket connections the pending dispatch keeps the NodeHTTPResponse pinned and counted as in flight, so server.close() waits on them. On the base branch 'upgrade' was emitted synchronously at dispatch and no promise was outstanding.
Verification: Trigger: a shouldUpgradeCallback attaches a req 'data' listener that calls wss.handleUpgrade() while parkUpgrade (_http_server.ts:1074-1075) holds the handoff. onUpgraded (JSNodeHTTPServerSocket.cpp:780-804) never touches functionToCallOnReadParsed; HttpContext.h:738 is gated on upgradedWebSocket != s, so the callback fires only at close via JSNodeHTTPServerSocket.cpp:858. On base 'upgrade' fired synchronously.
…and a read() in 'data' does not end the request first A 'data' listener of the request that made the socket a WebSocket left the parked handoff waiting until the WebSocket closed: uWS parses no more of the read for a WebSocket. onUpgraded() now posts the handoff. A read() inside the 'data' of the last chunk reached _read() before the EOF mark was set, so the request ended before 'upgrade'. The mark is set before the chunk is pushed.
|
About the finding outside the diff (
|
There was a problem hiding this comment.
I re-reviewed the two commits pushed since my last pass and found no new bugs; because this now runs the 'upgrade' listener inside uWS's parse window in HttpContext.h, a human look at that block is still worthwhile.
What was reviewed:
- 5305e71: onDataParkedUpgrade sets eofInProgress before the push (the readInData case in node-http-req-complete.test.ts covers it), and the handoff posted from onUpgraded is a no-op when the listener itself upgraded, since the callback is copied and cleared before it runs.
- HttpContext.h:737-752 ref balance: the onData ref is released at :750 on the complete path and through returnedData on the incomplete one; functionToCallOnReadParsed is visited in visitChildrenImpl and cleared in detach; createBuffer failure is reported rather than cleared silently.
- Ruled out: a microtask of the parked listener upgrading a different connection would make :745 reset this socket's parse result; it needs the listener to upgrade another stored socket, and the neighbouring :807/:887 checks already tolerate that shape.
Extended reasoning...
The change moves the node:http 'upgrade' emit for an Upgrade request with a body from the end of the headers to the end of the parsed read, touching the uWS HttpContext::onData parse loop (both SSL instantiations), JSNodeHTTPServerSocket (a new GC-visited callback slot and a prototype method), NodeHTTP.cpp (a new dispatch bit), _http_server.ts, and deletes the old NodeHTTPResponse-side notifyWhenReadParsed path; no auth, crypto or injection surface is involved. The latest commit addresses the re-entrant read() ordering I flagged earlier and adds tests for the WebSocket-upgrade-in-listener cases, and this run's hunt ran dry with no findings. I deferred rather than approved because user JS now runs mid-parse in native code with manual ref and parse-result bookkeeping on several exit paths, and the DISPATCH_READ_HAS_MORE nit from my earlier review was answered by the author rather than changed.
…ade-after-read # Conflicts: # src/js/node/_http_server.ts
main now counts the head that a 'connect' or 'upgrade' listener gets in socket.bytesRead. The handoff that waits for the end of the read does the same. The builtin ws now waits for the close frame of the peer before it ends the connection: the two ws tests that close a WebSocket answer that frame, or terminate.
…4419) ### What does this PR do? Fixes a `node:http` regression: a handler that rejects an upload early and then half-closes the socket never sees its request end. ```js http.createServer((req, res) => { req.on("data", () => {}).on("end", () => console.log("req end")).on("close", () => console.log("req close")); res.writeHead(401, { "content-length": 2 }); res.end("no"); req.socket.end(); }); ``` A raw client sends the POST head with `Content-Length: 1000` and sends the body once it has seen the server's FIN. | | `req` events | body bytes | | --- | --- | --- | | Node v22.22.0 | `end`, `close` | 1000 | | Bun 1.4.2 | `end`, `close` | 0 | | Bun 1.4.3, main | none, ever | 0 | | this PR | `end`, `close` | 1000 | **Cause.** `HttpContext::onData` drops every read of a socket that is shut down, unless it is a tunnel. An `http.Server` socket has `allowHalfOpen`, so `socket.end()` only ends the write side. Since #43557 a request ends at its real last chunk, and that chunk is never read. The response has finished, so the close of the socket does not abort the request either. **Fix.** - `socket.end()` marks the connection (`HTTP_NODE_HALF_CLOSED`, set in `deferShutdownUntilResponseDrains`). The three shut-down checks of `HttpContext::onData` let the body of the request in flight through (`readsBodyBehindOwnFin`). The last chunk stops the parse as before, so no other request is read behind the FIN. - `HttpParser::hasIncompleteRequestBody()` no longer counts a chunked body whose last chunk was delivered. A data handler that stops the parse there leaves `STATE_IS_TRAILERS_DONE`. Without this, https reports `HPE_INVALID_EOF_STATE` for a complete body and the connection never counts as idle. - `onDataIncomingMessage`: behind the FIN, a reader that is slower than the client pauses the reads like on any other connection. A body that nothing reads is dropped from the next read on, so the client's FIN still closes the connection (the existing `server.close()` test in this file needs that). The new `finSent` getter tells "the FIN has left" from "`end()` was called and the FIN waits for queued bytes", where nothing changes. - `socket.destroySoon()` goes through the same code. The request now gets the body that came in the read of its head, as in Node. The socket still closes behind that read. Source change against main: +35/-8 lines. **Unchanged, and still different from Node:** - `socket.end()` while response bytes are still queued (a response larger than the send buffer, or a client that does not read). With a finished response the connection closes at the flush and the body is lost. #44244 covers the case of an unfinished response. This PR no longer builds on it. - The body of an accepted Upgrade request is not read behind the FIN (#45058). - A request that arrives behind the FIN is not dispatched. Node dispatches it, and its response cannot leave. - Over plain http, the peer's FIN behind ours closes the socket with no socket `'end'` and no `HPE_INVALID_EOF_STATE` for a body that was cut short. https reports the cut as on main. - A reader that attaches after the buffer of `req` filled behind the FIN gets only the buffered part. Node pauses the reads and gives it all. **Changed as a consequence:** a handler that ends the socket and never ends `res` (`req.socket.end("HTTP/1.1 400 ...")`). On main `requestTimeout` closes that connection, because the body is never read. Now the request completes, so only the client's FIN closes it, as in Node. With `res.end()` the keep-alive timeout applies as usual. ### How did you verify your code works? `test/js/node/http/node-http-server-socket-end-drain.test.ts`: 36 new tests, over http and https. - 32 fail on a debug build of main's sources. The other 4 are guards for behavior that must not change: the unread body of an Upgrade request, and a late reader while the FIN waits. - All 45 tests of the file pass on this branch (debug build with ASAN, Linux x64). - Each clause of the fix was removed in turn. Every removal fails at least one test, except the `HTTP_CONNECTION_SCOPED` entry. That one only matters when a request is dispatched between `end()` and a FIN that a pending TLS write delays. - 35 scenarios, the script above among them, ran under Node v22.22.0, main's sources and this branch, over http and https: Content-Length and chunked bodies, a body that starts in the read of the head, a body that is cut short, a malformed chunk, `requestTimeout` on a stalled body, `server.close()`, pipelined requests, CONNECT, Upgrade, `Connection: close`. What `req` emits and reads matches Node in every rejection scenario. The scenarios listed as unchanged give the same output as main. --- _Generated by [Claude Code](https://claude.ai/code)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
Problem
'upgrade'listenerreq.complete === true, the body inreq, and the rest of the read ashead. Bun givesfalseand both empty. Asocket.end()in the listener loses the body and the tunnel bytes.onNodeHTTPRequest(src/js/node/_http_server.ts:1068) emits at the end of the headers.HttpContext::onData(packages/bun-uws/src/HttpContext.h:340) drops the reads of a shut-down socket that is not yet a tunnel.Fix
HttpContext::onDatatreat an accepted Upgrade request like a tunnel.node-http-req-complete.test.ts(65 new tests, 54 fail on main, all pass under Node),test/js/node/http,ws.test.ts.Background
Downsides
reqand the bytes behind it inhead..text+1,792 bytes, bundled JS +600 bytes, one test ofrequest->head.empty()per dispatch.Notes
Stack. The first three commits are #45026 (
abort()of a handed-off request writes nothing). The listener now runs when the connection is a tunnel, soreq.destroy()in it needs that fix. The commits behind them are this change: +141/-108 lines insrcandpackages.Repro.
What the pair (
req.complete,head) is. It is what Node v26.3.0 gives when the body shares the read of the head. It is not in Node's documentation, and it moves with the size of a read: 64 KB for TCP and one record for TLS in Node, up to 512 KB in Bun. A listener must not rely on which side of the line a large request falls.socket.end()in the listener, 14 cells (CONNECT, a body-less Upgrade, and five body arrivals, each with a flowing and a paused socket). main5866b5f13ddelivers fewer bytes than Node in 12. This branch delivers fewer in 4: the paused tunnels of CONNECT, of a body-less Upgrade and of the two bodies that came with the head. One line avoids the loss on main:req.on("end", () => setImmediate(() => socket.end())).Mechanism.
NodeHTTP.cppadds the dispatch bitkDispatchReadHasMore: bytes follow the request head in the read. The dispatcher parks an accepted Upgrade with a body only with this bit. CONNECT, a body-less Upgrade, and a head that arrives alone keep the synchronous emit.HttpContext::onDatareturnsnullptrbehind the last chunk of the body while the handoff waits. That is the existing contract of the parser: it stops, andconsumedBytes()is the offset of the bytes that belong to the caller. Those bytes are thehead.isParsingHttpis cleared. A listener that makes the socket a WebSocket there takes the existingupgradedWebSocketexit:onDatatouches noHttpResponseDataof a socket that has none, and bytes behind the body reach the WebSocket.AsyncContextFrame::withAsyncContextIfNeeded, so the listener runs in the async context oflisten(), as on main.onUpgraded()posts. uWS parses no more of that read.headtosocket.bytesRead, like the two synchronous handoffs of WebSocket: real closing handshake, one failure path,wsrewritten on native transports (consolidates the open WebSocket PRs) #45223.notifyWhenReadParsed,on_read_parsedandAbortEvent::ReadParsed. Their only user was the paused request of an Upgrade, which now decides at once.onEndandonClosealready treatHTTP_NODE_TUNNEL_AFTER_BODYas a half-open tunnel. The three tests ofus_socket_is_shut_downinonDatanow do too.Still different from Node, by mechanism.
server.emit('connection')or from http2allowHTTP1goes throughsrc/js/internal/http1_server_fallback.ts. It does not parse the body of an Upgrade request:headholds the body bytes.onParserExecuteCommon. For a body that completes in a later read, Node pushes the bytes behind it at the end of that read and hands anUpgradeStream. Here they arrive behind the'end'of the request, on aSocket.shouldUpgradeCallbackrun before the listener when the read holds chunk framing and no body data (5\r\n,0\r\n\r\n). Node and main run the listener first. The Rust dispatch runs a tick checkpoint when it returns (src/runtime/server/mod.rs), which node:http: run nextTicks and promise jobs where Node's parser does #43736 rewrites.'clientError'follows'upgrade'when the body fails to parse (node:http: do not fire 'clientError' for a parse error in the body of an accepted Upgrade request #43337).resetAndDestroy()inshouldUpgradeCallbackstops the parse, and a socket that a'data'listener of the request destroys in the last chunk gives an emptyhead.For a listener that was written for Bun 1.4 and gets the body with the head:
socket.unshift(head)puts the bytes behind the body back into the socket, andreq.resume()makes'end'come.'end'does not come while nothing reads the request, as on Node.Not done from the self-review. The port of Node's change to the second dispatcher (
http1_server_fallback.ts), and the second moment ofonParserExecuteCommonfor a body that completes in a later read. Both are in the list above. Also weighed: the parked handoff for every Upgrade request with a body. It needs no dispatch bit, but the ticks ofshouldUpgradeCallbackthen run before the listener for a head that arrives alone.Other pull requests.
'upgrade'event for a body sent with the head as open. This change does that part.socket.end(). This change does that for an accepted Upgrade request only, at the same three lines.headBuffer only for CONNECT and a body-less Upgrade. This change does not need that Buffer for an Upgrade with a body: the dispatch bit says whether bytes follow the head.wsrewritten on native transports (consolidates the open WebSocket PRs) #45223 (merged) rewrote the builtinws. It readsheadnow, so ahandleUpgrade()from a later task keeps the frames that came behind the body. Itsclose()waits for the close frame of the peer, and two tests here answer that frame or callterminate().abort()for plain requests. node:http: req.destroy() of an accepted CONNECT or Upgrade request writes no 200 OK into the tunnel #45026 is the handoff case.Node.
node test/js/node/http/node-http-req-complete.test.tson v26.3.0: 91 tests pass, 0 fail. Rows that Bun answers in another way are marked in the file with Bun's value.Measurements (release builds of main
5866b5f13dand of this branch at5305e71cda, linux x64, the server in its own process). The branch has since merged main017b2439a7and gained one statement, thesocket.bytesReadline of the handoff:onNodeHTTPRequestbytecode: 749 to 763 instructions. 15 are one new block insideif (hasBody)of the upgrade arm, and the arm without a body loses 1. The CONNECT arm has no difference.onDataIncomingMessage(runs for every request body chunk): 81 to 62 instructions.BUN_JSC_useGC=0, slope ofheapStats().objectTypeCounts, medians of 5 runs of main and of the branch): GET 90.1 and 90.1, POST 103.6 and 103.6, body-less Upgrade 84.2 and 84.4, CONNECT 78.8 and 78.6. One build alone spreads over 0.7 cells.process.nextTickcalls per request: GET 10, POST 12, body-less Upgrade 8, CONNECT 8, the same on both builds.JSNodeHTTPServerSocket:sizeof+8 bytes. The GC cell is 208 bytes on both builds..text81,071,045 to 81,072,837 bytes, bundled JS 2,644,744 to 2,645,344 bytes, every other section the same. The stripped file grows by one page, 106,501,896 to 106,505,992 bytes. The numbers include the 5 bytes of node:http: req.destroy() of an accepted CONNECT or Upgrade request writes no 200 OK into the tunnel #45026.53bc9dfb3bagainst its merge base017b2439a7: +4.0 KB on linux-x64 and freebsd-x64, +2.5 KB on windows-x64, +2.0 KB on windows-aarch64, +16.1 KB on darwin-aarch64, and no change on the other seven targets.HttpContext<false>::onData<true>: 2,761 to 2,917 bytes, its data handler 797 to 879, its request handler 1,124 to 1,163. TheBun.serveinstantiations do not change.perf_event_openfails in the build VM, and valgrind is not installed.Suites. Debug build of the branch with main
017b2439a7merged: every file intest/js/node/http(39 files), every file intest/js/first_party/ws(21 files), and the vendoredtest-http-upgrade*,test-http-connect*andtest-http-parser-freed*files. Tests with a fixed time limit that start a secondbun(the nestednode-http-connect.node.mtsandnode-http-proxy-url.node.mtsruns,node-http-uaf, two ofnode-http-syscall-fault) exceed it on the debug build of the test machine. They pass when run alone with a longer limit.Fail before. Tests that fail on the release build of main
5866b5f13dand pass on the release build of this branch: 54 of 92 innode-http-req-complete.test.ts, 11 of 98 innode-http-connect.test.ts, 4 of 102 inws.test.ts, 4 of 14 innode-http-upgrade-body.test.ts.no test proof · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/http/node-http-connect.test.ts