Repository navigation
Conversation
… request bytes
Node's http.Server feeds its parser from the connection socket's 'data'
event, so a user listener on that socket sees the raw HTTP request bytes
alongside the parser. In Bun the native uWS parser consumes the TCP payload
internally and the JS socket wrapper never received those bytes, so
socket.on('data') inside a 'connection' handler never fired.
Add a raw-bytes tap in HttpContext::onData that forwards each TCP payload
to a new onrawdata slot on JSNodeHTTPServerSocket before parsing. The slot
is only populated when the JS socket actually has a 'data'/'readable'
listener (checked after emit('connection') and on each _read()); when it is
null the native tap is a single null check. Tunnel mode (CONNECT/Upgrade)
keeps its existing ondata path and clears onrawdata on entry so bytes are
not delivered twice.
Fixes #11924.
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
WalkthroughAdds a raw socket data path for Node HTTP compatibility. Native HTTP payloads are forwarded before parsing, bridged into JavaScript, exposed through socket data/readable streams, coordinated with streaming mode, and covered by HTTP/HTTPS tests. ChangesRaw Node HTTP socket data
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/js/node/_http_server.ts:1918-1922—#resumeSocket()still doesthis.push(resumed)at line 1911 with the parsed request body drained fromresponse.resume(), while the new raw tap (#onRawData) also pushes the raw TCP payload containing those same body bytes into the same Duplex — so under IncomingMessage backpressure the connection-socket'data'listener receives the body twice (once inside the raw HTTP framing, once as a bare parsed chunk). Gate line 1911 on!handle.onrawdata(or drop it — the raw tap now owns the socket's readable side).Extended reasoning...
What the bug is
This PR adds
#onRawDatawhich pushes every raw TCP payload into theNodeHTTPServerSocketDuplex so a'data'listener on the'connection'socket sees the raw request bytes. But the pre-existing#resumeSocket()still containsthis.push(resumed)at line 1911, which pushes the parsed request body (drained fromresponse.resume()'sbuffered_request_body_data_during_pause) into the same Duplex._read()at lines 1916–1922 now calls bothmaybeEnableRawSocketData(this)(arming the raw feed) andthis.#resumeSocket()(the parsed-body feed), andresume()at line 2078 also calls#resumeSocket(). Nothing gates line 1911 on whetherhandle.onrawdatais set.Code path that triggers it
server.on('connection', s => s.on('data', ...))— afteremit('connection'),maybeEnableRawSocketData(socket)setshandle.onrawdata = #onRawData.bind(this).- A POST request arrives whose body is not immediately consumed.
onDataIncomingMessage(internal/http.ts:261–266) callsreq.push(chunk); when the IncomingMessage's 16 KB highWaterMark fills,push()returnsfalse→socket.pause(). NodeHTTPServerSocket.pause()(line 2067) callsresponse.pause()→do_pause()re-registers the nativeinStreamtoon_buffer_paused_shim, which appends every subsequent body chunk tobuffered_request_body_data_during_pause. This buffer becomes non-empty when (a) the request is chunked and more chunks in the same TCP packet follow the one that triggered the pause (the parser keeps running through the currentonDatabuffer), or (b) on Windows, wheredo_pauseskipspause_socket()so subsequent packets keep arriving.- Meanwhile the raw tap in
HttpContext::onDatafires before parsing: for that same TCP packet,onSocketRawData→JSNodeHTTPServerSocket::onRawDatahas already posted a task that willthis.push(rawPacketBytes)into the Duplex. On Windows, subsequent packets are also raw-tapped. - When the IncomingMessage drains,
IncomingMessage._read→socket.resume()(line 2077) →#resumeSocket()→response.resume()returns the buffered parsed body as a Buffer → line 1911 doesthis.push(resumed).
Step-by-step proof
Take a chunked POST whose single TCP packet contains
[headers][chunk-1: 20 KB][chunk-2: 4 KB][0\r\n\r\n], with a'connection'-socket'data'listener attached and a request handler that does not touchreq:HttpContext::onDatareceives the whole packet. The raw tap posts a task with a Buffer of the entire raw packet (headers +14000\r\n<20KB>\r\n1000\r\n<4KB>\r\n0\r\n\r\n).- The parser dispatches the request, then delivers chunk-1 via
inStream→onDataIncomingMessage→req.push(20KB)returnsfalse(HWM = 16 KB) →socket.pause()→response.pause()swapsinStreamtoon_buffer_paused_shim. - Still inside the same
onData, the parser delivers chunk-2 via the swappedinStream→on_buffer_paused_shimappends the 4 KB tobuffered_request_body_data_during_pause. - The posted raw-tap task runs:
#onRawData→this.push(<raw packet>)— the socket'data'listener receives the full raw packet, including the 4 KB of chunk-2. - Later,
IncomingMessage._read→socket.resume()→#resumeSocket()→response.resume()returnsBuffer<4KB>→req.push(4KB)andthis.push(4KB)(line 1911). The socket'data'listener now receives an extra 4 KB chunk — the parsed body of chunk-2, with no HTTP framing.
The listener's byte stream is
<raw packet><4KB body>instead of<raw packet>. On Windows this applies to every body packet after the pause, not just the tail of one.Why existing code doesn't prevent it
maybeEnableRawSocketDatagates the tap on!socket[kStreamingEnabled], and[kEnableStreaming](true)(tunnel mode) clearshandle.onrawdata. But nothing in#resumeSocket()checkshandle.onrawdatabefore line 1911. Before this PR,this.push(resumed)was the only thing feeding the socket's readable side (and it almost never fired, which is why #11924 exists). Now there are two feeds into the same Duplex, and they overlap on exactly the bytes the pause buffer captured. None of the new tests exercise this: test 2 doesreq.resume()so the IncomingMessage never backpressures.Impact
The PR's stated contract is that a
'data'listener on the'connection'socket "receives the raw request bytes" — i.e. the wire bytes. When triggered, the stream contains bytes that were never on the wire in that position (a bare body slice with no chunk framing, appearing after the raw packet that already contained it). Any consumer that concatenates the socket's'data'chunks and re-parses them (proxies, request loggers, HTTP capture tooling — the primary use case for this feature) will see a corrupted stream. The trigger — a POST body large enough to fill IncomingMessage's 16 KB HWM before the request handler reads it — is realistic, and on Windows it affects the entire body after the first pause.Fix
Gate the socket-side push on the raw tap not being armed:
if (req) { req.push(resumed); } if (!handle.onrawdata) this.push(resumed);
Or simply drop
this.push(resumed)— with the raw tap in place it is redundant (Node's socket carries raw bytes, not parsed body, so this line was never Node-correct anyway; the raw tap now covers what it was approximating). Thereq.push(resumed)on line 1909 must stay — that feeds the IncomingMessage, not the socket.
…et push, tighten tests - Move the raw-bytes tap above the HTTP_NODE_PARSING_STOPPED early-return so a user 'data' listener keeps seeing bytes after a custom 'clientError' handler leaves the connection open (Node removes only its own 'data' listener there). - Gate #resumeSocket's socket-side push on !handle.onrawdata: the raw tap owns the socket readable; pushing the parsed body and push(null) there would double-deliver body bytes and end the readable mid-keep-alive. - Count tunnel-mode bytes in socket.bytesRead (#onData), completing the pair. - Wrap tests 2, 4, 5 in try/finally so the server and client are released on assertion failure. - Trim multi-line comments flagged by the comment-cop check.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/js/node/http/node-http-connection-socket-data.test.ts (1)
1-186: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftMissing test coverage for backpressure and CONNECT/Upgrade tap-disable interaction.
Good coverage for the happy paths, but no test exercises a slow consumer causing
socket.push()backpressure (which is where a genuine data-loss bug exists — see the_http_server.tsreview) or verifies the raw tap doesn't double-deliver / lose bytes across a CONNECT/Upgrade tunnel switch. Consider adding cases for both, per the "every behavioral change needs a test" guideline.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/js/node/http/node-http-connection-socket-data.test.ts` around lines 1 - 186, Extend the HTTP socket data tests with cases covering a slow consumer that triggers socket.push() backpressure, asserting all raw request bytes remain observable without duplication or loss. Also add a CONNECT or Upgrade test that verifies the raw socket tap is disabled exactly once during tunnel transition and preserves bytes sent before and after the switch. Anchor the additions near the existing http.Server connection and req.socket data tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js/node/_http_server.ts`:
- Around line 1640-1654: Update the raw-data backpressure handling in `#onRawData`
and its kEnableRawData re-arm path so a false result from this.push(chunk)
pauses the underlying socket read rather than only clearing handle.onrawdata,
and resume it when demand returns. Coordinate the tap lifecycle with the
existing socket pause/resume mechanism and preserve bytes until _read()
re-enables delivery, including the corresponding path around the later raw-data
handling block.
---
Outside diff comments:
In `@test/js/node/http/node-http-connection-socket-data.test.ts`:
- Around line 1-186: Extend the HTTP socket data tests with cases covering a
slow consumer that triggers socket.push() backpressure, asserting all raw
request bytes remain observable without duplication or loss. Also add a CONNECT
or Upgrade test that verifies the raw socket tap is disabled exactly once during
tunnel transition and preserves bytes sent before and after the switch. Anchor
the additions near the existing http.Server connection and req.socket data
tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 954b8871-9683-4f20-9926-1e44acfd73f8
📒 Files selected for processing (9)
packages/bun-uws/src/App.hpackages/bun-uws/src/HttpContext.hpackages/bun-uws/src/HttpContextData.hsrc/js/node/_http_server.tssrc/jsc/bindings/NodeHTTP.cppsrc/jsc/bindings/node/JSNodeHTTPServerSocket.cppsrc/jsc/bindings/node/JSNodeHTTPServerSocket.hsrc/jsc/bindings/node/JSNodeHTTPServerSocketPrototype.cpptest/js/node/http/node-http-connection-socket-data.test.ts
…l entry The native !isConnectRequest gate (set synchronously by upgradeToTunnel()) already stops the raw tap for tunnel payloads, so clearing handle.onrawdata in [kEnableStreaming](true) only dropped the already-posted CONNECT/Upgrade request-head chunk. Disarming on push()==false in #onRawData dropped an interior slice of the wire stream (the parser keeps reading the socket regardless) and left handle.onrawdata transiently null so the #resumeSocket gate opened and pushed the parsed body a second time. With the tap never disarmed, handle.onrawdata is a stable ownership predicate and the Readable buffer is bounded by what the connection already holds. Adds a CONNECT test that asserts the request line reaches the 'data' listener exactly once and tunnel bytes follow without duplication.
…en CONNECT assertions Node's https.Server emits the raw net.Socket on 'connection' (its 'data' never fires there) and the TLSSocket on 'secureConnection'; the decrypted-bytes test now listens on 'secureConnection' so it actually matches Node. The CONNECT test drops the tunnel-byte assertion (Node sets flowing=null before 'connect' so a 'connection'-time listener stops there) and polls for EXTRA instead.
…after secureConnection
When an Upgrade-with-body's body-fin and first tunnel bytes share one TCP read,
the raw tap delivers the whole read and the tunnel path would deliver the tail
again via onSocketData. onSocketRawData now returns whether the JS callback was
armed; the dataHandler lambda captures that and skips its onSocketData call
when the raw tap already covered this read. CONNECT and body-less Upgrade reach
the tunnel path with isConnectRequest already true at onData entry, so the raw
tap was skipped there and onSocketData remains the sole producer.
Also move maybeEnableRawSocketData() after emit('secureConnection') so a
listener attached there arms the tap synchronously like the 'connection' path.
There was a problem hiding this comment.
The latest push looks good — rawTapCoveredThisRead in the data-handler lambda cleanly resolves the Upgrade-with-body tunnel-tail double-delivery, and maybeEnableRawSocketData now runs after emit('secureConnection') so both emit sites arm synchronously. No new issues found; the remaining 🟡 on HttpContext.h:310 is the pre-existing split-CONNECT parser flag write and is out of scope here.
That said, this threads new state through HttpContext::onData, adds a GC-visited callback slot with postTask delivery, and changes who owns the socket Duplex's readable side in #resumeSocket(), with two documented intentional divergences from Node. Worth a human pass on the design tradeoffs before landing.
What was reviewed:
rawTapCoveredThisReadcapture-by-value into the data-handler lambda: verified it correctly gatesonSocketDatafor the straddling packet while leaving subsequent tunnel reads (and the unarmed-tap case, whereonRawDatareturns false) on the existing#onDatapath —bytesReadis incremented exactly once per byte.onRawData'sgcProtect/postTask/gcUnprotectmatches the siblingonDatapattern;functionToCallOnRawDatais aWriteBarriervisited invisitChildrenImpl.#resumeSocket()'s!handle.onrawdatagate is now stable (the tap never disarms), so parsed-body/EOF pushes are correctly skipped only when the raw tap owns the readable side.
Extended reasoning...
Overview
This PR makes a node:http server's 'connection' socket emit 'data' with the raw request bytes, matching Node's observable contract (Node feeds its parser from that same event). It adds an onSocketRawData tap in HttpContext::onData (before parsing and before the parser-stopped gate), a new onrawdata callback slot on JSNodeHTTPServerSocket (WriteBarrier + getter/setter + visitChildren), and JS-side wiring in _http_server.ts that arms the tap only when a user actually listens for 'data'/'readable'. The tap is a passive observer; the native parser keeps consuming regardless. Seven tests cover HTTP, HTTPS via 'secureConnection', keep-alive with bodies, 'readable'+read(), late-attached listeners, CONNECT head-once, and Upgrade-with-body tunnel-tail dedup.
Files: packages/bun-uws/src/{App.h,HttpContext.h,HttpContextData.h}, src/jsc/bindings/NodeHTTP.cpp, src/jsc/bindings/node/JSNodeHTTPServerSocket{.h,.cpp,Prototype.cpp}, src/js/node/_http_server.ts, plus a new test file.
Security risks
None identified. The tap is opt-in (only reachable when a user attaches a 'data'/'readable' listener on the connection socket) and copies bytes the parser is already consuming — it exposes no new data, adds no parsing of untrusted input, and touches no auth/TLS decisions. The one memory-growth vector (Readable buffer accumulating while the consumer is paused) is bounded by what the connection already holds and is called out in the PR body.
Level of scrutiny
High. HttpContext::onData is the per-read hot path for every node:http server connection, and the change captures a stack bool into a lambda that the parser calls back mid-parse. The C++ side adds a GC-visited slot with a gcProtect→postTask→gcUnprotect handoff (mirroring the existing onData sibling, which reduces risk). On the JS side, #resumeSocket() now skips its socket-readable pushes when the raw tap owns the readable — a real ownership change whose correctness depends on the tap never transiently disarming (which was the subject of an earlier 🔴 and is now fixed: #onRawData always pushes, and only [kEnableRawData] writes handle.onrawdata). The PR also documents two intentional divergences from Node (pause() doesn't throttle parsing; no push(null) on non-tunnel FIN) that are architectural rather than incidental.
Other factors
The PR has iterated through five fix commits in response to earlier 🔴 findings (dual-producer in #resumeSocket, backpressure disarm dropping bytes, CONNECT/Upgrade head loss via cleared callback, and the tunnel-tail double-count). Each is now addressed and covered by a test in the new file. The one open 🟡 (isConnectRequest written prematurely on split-CONNECT short-read) is a pre-existing HttpParser.h defect that already breaks split-CONNECT dispatch independently of this PR; fixing it here would be scope creep into a file the PR doesn't touch. Given the number of subtle interactions surfaced during review and the documented Node-divergence tradeoffs, a human should sign off on the design before merge.
|
CI on 98320ea (build 81201) is red only on lanes unrelated to this diff:
|
|
Closing this PR because it has been inactive for more than 90 days. |
A
node:httpserver's'connection'event hands over a socket whose'data'event never fires for normal requests. In Node that socket is thenet.Socketthe parser reads from, so every'data'listener (the parser's included) receives the raw HTTP request bytes.Cause
Bun's node:http server parses HTTP natively in uWS:
HttpContext::onDataconsumes the TCP payload and dispatches the parsed request straight toonNodeHTTPRequest. The JSNodeHTTPServerSocketwrapper is a Duplex whose readable side is only fed in CONNECT/Upgrade tunnel mode (viahandle.ondata), so for an ordinary request nothing ever pushes into it and'data'/'readable'never fire.Fix
Add a raw-bytes tap in
HttpContext::onDatathat forwards each TCP payload, before HTTP parsing (and before the parser-stopped gate, so a custom'clientError'handler that keeps the socket open keeps seeing bytes), to a newonrawdatacallback slot onJSNodeHTTPServerSocket. The slot is only populated when the JS socket actually has a'data'/'readable'listener (checked right afteremit('connection')and on each_read()); when it is null the native tap reduces to a single null-check, so the common path (no user listener) pays no Buffer allocation or JS call. The JS side pushes the chunk into the Duplex, so'data'/'readable'fire with the raw request bytes, andsocket.bytesReadadvances.Once armed the tap stays armed; the native
!isConnectRequestgate (set synchronously byupgradeToTunnel()) stops it for tunnel payloads, and#resumeSocket()skips its parsed-body socket push when the tap owns the readable side.Why this is the right fix
In Node,
connectionListenerInternalattachessocketOnDataas a'data'listener on the connection'snet.Socket; the parser is fed from that event, so a second user listener sees the same bytes. Replicating that exactly would mean round-tripping every HTTP byte through JS, defeating the native parser. Tapping the raw payload only when a user actually subscribes preserves the native fast path while matching the observable contract: a'data'listener on the'connection'socket receives the raw request bytes (request line, headers, body framing and all), same as Node.Verification
New
test/js/node/http/node-http-connection-socket-data.test.ts(all six fail on main, pass with this change, and the first five match Node v26 exactly):'connection'socket'data'carries the full request line + headers and updatesbytesRead'readable'+read()yields the same byteshttps.Server'secureConnection'socket delivers the decrypted request bytes (Node's https'connection'hands over the rawnet.Socket, whose'data'never fires there; the TLSSocket is on'secureConnection')req.socket.on('data')added inside the request handler sees subsequent raw bytesNo regressions in
node-http.test.ts, the CONNECT/Upgrade suites,node-http-req-socket-pause,node-http-backpressure,node-http-transfer-encoding,node-http-server-timeouts, or Node's upstreamtest-http-connect*/test-http-upgrade*/test-http-server-unconsume*tests.Known differences from Node
socket.pause()on the'connection'socket does not throttle HTTP parsing (Node pauses the fd, which stops its parser too). The Readable buffer grows while paused, bounded by what the connection already holds.push(null)on peer FIN for a non-tunnel connection;'close'fires as before.Fixes #11924.