Conversation
A CONNECT with Content-Length or Transfer-Encoding was marked as having a body, but the parser tunnels every byte after a CONNECT head. The body state stayed pending for the whole tunnel, so pause() on the 'connect' socket parked a copy of each tunnel chunk in the request-body pause buffer, and a reader on the request received the tunnel bytes too. Node ends the request of a 'connect' event with no data.
The parser enters tunnel mode at the request line only for an authority-form target, so these rows pin the choice of the method as the criterion.
Contributor
|
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 11 seconds 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 (2)
Comment |
Collaborator
Author
|
Updated 6:56 PM PT - Sep 19th, 2026
✅ @robobun, your commit 81d7ec0ec183fd04cb577122c3099c7b419961ae passed in 🧪 To try this PR locally: bunx bun-pr 43555That installs a local version of the PR into your bun-43555 --bun |
Collaborator
Author
|
Status: ready for review. How I reproduced it: a node:http server whose
The 8 new tests in CI: green at the PR head. Build 118713 (
Merge note: #43461 edits the same condition and computes main's value for a CONNECT. The PR that lands second must keep both changes (framing headers for every method except CONNECT). The 8 tests here fail if the CONNECT check is lost. Open point: my self-review of this diff is incomplete. It named two concerns about the implementation that I could not read, so I did not address them. The PR body says which review items I did act on. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…timeout If the tunnel closes before its bytes reach the paused socket, the wait loop now throws with the buffered byte count. The catch destroys the socket: the server's dispose waits for it, and that wait turned a rejection into a bare 5 s timeout.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…T tests The previous commit reports a tunnel that closes. A tunnel that stays open and delivers nothing still ran into the test timeout with no message. The wait now has a deadline below the default test timeout and throws with the buffered byte count.
Contributor
There was a problem hiding this comment.
I re-reviewed the two follow-up commits and found no new bugs; the bounded poll with a byte-count message in the paused rows resolves the earlier timeout concern. A maintainer should still make the call on the one behavior change the description accepts: a pipelined origin-form CONNECT with a body now delivers its bytes to no listener (neither req nor head) until #43376 routes it to 'connect'.
What was reviewed:
- Confirmed the
/xrows are legitimate:socketHandle.upgradeToTunnel()in_http_server.ts:748setshttpResponseData->isConnectRequestsynchronously inside the request handler, and the parser reads that same field by reference atHttpParser.h:1370, so post-head bytes tunnel for origin-form targets too. - Checked that
has_body = falseleaves no dangling request-body state: noinStreamis installed, so no pause buffer, no pending-body count onserver.close, andreqends immediately. - Test wiring:
error/closeon both sockets reachreject, the poll deadline (4s) sits under the default timeout, andsocket.destroy()on failure keeps the server's dispose from hanging.
Extended reasoning...
Overview
The diff is a one-condition change in src/runtime/server/NodeHTTPResponse.rs (NodeHTTPResponse__createForJS no longer enters the request-body path for CONNECT) plus an 8-case describe.each x test.each matrix in test/js/node/http/node-http-connect.test.ts covering authority-form and origin-form targets, Content-Length and chunked framing, flowing and paused sockets. Since the previous review the author shortened the Rust comment to one line and added a wall-clock deadline plus a tunnelClosed break to the paused-socket poll, which addresses the earlier inline note about a message-less timeout.
Security risks
None introduced. The change removes body-reading state for CONNECT rather than adding parsing; the parser's own framing validation (Content-Length overflow, Transfer-Encoding smuggling checks at HttpParser.h:1296-1328) runs before the request handler regardless of has_body. The tunnel path continues to deliver bytes to the JS socket without a second native copy, which is a memory reduction, not a new attack surface.
Level of scrutiny
Moderate. The code change is tiny and the reasoning holds: after a CONNECT head the uWS parser either enters tunnel mode (authority-form at parse time, or origin-form once upgradeToTunnel() flips isConnectRequest during the handler) or, for the pipelined case, parses the declared body as HTTP and drops it because no inStream is installed. That last shape is the one a human should weigh. On main the pipelined origin-form CONNECT's body reached req; after this PR it reaches nothing. Node delivers it as head via 'connect'. The author documents this as accepted and tracked in #43376, but it is a deliberate regression on a documented-but-exotic shape, so it is a maintainer decision rather than a mechanical merge. The PR description also states self-review is incomplete with two unretrieved concerns.
Other factors
The tests are hermetic (listen(0) on loopback, net.connect client), wire every failure event to reject, attach req/socket data listeners synchronously before any await in the flowing case, and assert a single toEqual object. The pre-existing HEAD/TRACE body drop noted in a prior comment is unchanged and outside this PR's scope. No CODEOWNERS entry covers the changed files. The bug hunt exited on dry_streak with no findings this run.
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>
Collaborator
Collaborator
Author
|
Confirmed. I built main at 5d5f03f and checked it against this PR:
#43552 (the same request on Bun.serve) still reproduces on main. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Content-Lengthabove 0 or withTransfer-Encodingkeeps a second native copy of its tunnel bytes inbuffered_request_body_data_during_pauseafter onesocket.pause(). Nothing drains it. A 256 MB upload that JS reads in full: 842 MB RSS withContent-Length: 10, 383 MB without (same debug build).reqin the'connect'listener also gets every tunnel byte. Node v26.3.0 ends thatreqwith no data.NodeHTTPResponse__createForJS(src/runtime/server/NodeHTTPResponse.rs:2601) setshas_bodyfor a CONNECT, but the parser delivers no body after a CONNECT head. Found by code inspection. No issue reports it.Fix
NodeHTTPResponse__createForJSdoes not sethas_bodywhen the method is CONNECT.CONNECT /xwith a body goes to'request'on main, and this PR drops that body (Notes, node:http: emit 'connect' for a CONNECT that is pipelined behind a pending response #43376).src/runtime/server/mod.rs:739, Bun.serve: a CONNECT with an authority-form target and a Content-Length never delivers its body, req.text() stays pending #43552).CONNECT /xwith a body works there, and this check would empty it.test/js/node/http/node-http-connect.test.ts, 8 new tests, all fail without the fix. Other suites: Notes. Self-review: incomplete, see Notes.Background
'connect'withreqand the raw socket. The parser passes every later byte to the socket unparsed.body_read_stateisPending,pause()and a read ofreqinstall a body callback.packages/bun-uws/src/HttpContext.h:616).Notes
Self-review: incomplete. A review of this diff returned "merge after changes". From it: the
/xtest rows, the like-for-like memory table, the Bun.serve site named as excluded and tracked in #43552, the note onMethod::has_request_body(), and the "Not fixed here" list. The review also named two more concerns about the implementation. I could not retrieve their text, so they are not addressed. I do not know what they are.Memory, like for like. One script, one machine. The
'connect'listener callssocket.pause(). The client uploads 256 MB. "paused" never reads. "pause, then resume" callsresume()at once and discards the data, so JS holds nothing at the end. Each row compares the same build with and without the header. Debug builds have ASAN, so read the difference in a row, not the absolute values.Content-Length: 10This PR removes the second copy only. In the "paused" rows JS still buffers all 256 MB once, with or without the fix.
Event order. CONNECT with
Content-Length: 5, then the client writeshello tunneland ends. Output of the same script:Transfer-Encoding: chunked, a paused socket, and the target/xgive the same result on each runtime. With a paused socket on main, the bytes thatreqreceives are the ones the pause buffer held: the debug log showsonBufferRequestBodyWhilePaused(12, false), thendrainBufferedRequestBodyFromPause 12when the test readsreq. The paused tests use this to observe the copy without an RSS threshold.Why the check uses the method. For an origin-form target (
CONNECT /x) the parser has not set its tunnel flag whenNodeHTTPResponse__createForJSruns. JS sets it later, from the'connect'path. A check onraw.is_connect_request()would miss that target. The/xtest rows cover it.Why not a guard in
do_pause.do_resumehas|| raw.is_connect_request()for this reason (#34432). The same guard indo_pausestops the pause buffer, butset_on_datastill arms the body callback when something readsreq, andshould_request_be_pendingstill sees a pending body. Withhas_bodyfalse none of these sites sees a tunnel with a pending body, so that guard would be dead code. Thedo_resumeguard stays: for an Upgrade request with a body it still decides which call drains a tail buffered before the switch to tunnel mode.Why
Method::has_request_body()is unchanged. Its other callers are on the client side:fetch()uses it to decide if a method may send a body (src/runtime/webcore/fetch.rs:1468), and the HTTP client uses it to frame a body and to send it again after a redirect (src/http/lib.rs:2540,:5276). They answer what a client may send. This bug is about what a node:http server receives after a CONNECT head.Bun.serve, not changed (#43552).
src/runtime/server/mod.rs:739makes the same has-body decision in front of the same parser. Probe: the handler awaitsreq.text(), the request is a CONNECT withContent-Length: 5andhello. On main and on this branch alike, the targetexample.com:80leavesreq.text()pending and the client gets no response, and the target/xanswersCONNECT:hello. A check on the method would turn thathellointo an empty body. The open question for Bun.serve is if the parser should enter tunnel mode at all for a server with no tunnel API, and that is a separate decision.server.close(cb). A tunnel releases the pending-request count of the server at the end of the dispatch (Flags::TUNNELED,src/runtime/server/mod.rs:1513), unless a body is pending. The body of a CONNECT never completed, so that count stayed at 1 andserver.close(cb)waited for this one kind of tunnel. Node waits for every open connection. Bun does not (documented atis_closed()). After this PR a CONNECT with framing headers behaves like every other tunnel.A pipelined CONNECT. On main a CONNECT behind a pending response goes to
'request', not to'connect'(#43376 is open for that). Probe: one write carriesGET /firstand a CONNECT withContent-Length: 5andhello. The listener holds the response to/first.example.com:443'connect', headhello'request',reqnever ends, the client gets no response'request',reqends with no data, both responses arrive/x'connect', headhello'request',reqbodyhello'request',reqends with no dataSo one pipelined shape loses a body that main delivers: on this branch the parser reads those bytes and no listener gets them. Node gives neither request a body, but it passes the bytes as
head. #43376 routes both to'connect', andheadthen carries the bytes.Interaction with #43461. #43461 edits the same condition. It removes the method check, so the framing headers decide
has_bodyfor every method. That fixes HEAD and TRACE. For a CONNECT it computes the same value as main, so it keeps the second copy that this PR removes. The two changes combine into one condition: read the framing headers for every method except CONNECT. The PR that lands second must resolve the conflict that way. If the resolution drops the CONNECT check, the 8 tests of this PR fail.Not fixed here.
req.completeisfalseinside the'connect'listener for every CONNECT (Node:true). node:http: complete CONNECT and Upgrade requests at dispatch, deliver HEAD and TRACE bodies, clear parser.incoming on finish #43461 and node:http: set req.complete once a request without a body is dispatched #43456 cover it.req.text()(Bun.serve: a CONNECT with an authority-form target and a Content-Length never delivers its body, req.text() stays pending #43552).Content-Length: 5andhello: Node v26.3.0 delivershellotoreq, Bun delivers nothing, on main and on this branch alike. It is the opposite fault on the same line (has_bodyfalse where it must be true). node:http: complete CONNECT and Upgrade requests at dispatch, deliver HEAD and TRACE bodies, clear parser.incoming on finish #43461 fixes it.Suites run with the debug build.
test/js/node/http/node-http-connect.test.ts,node-http.test.ts,node-http-req-socket-pause.test.ts,node-http-backpressure.test.ts,node-http-server-abort-events.test.ts,node-http-server-socket-end-drain.test.ts,node-https-agent-checkserveridentity-reuse.test.ts,test/js/web/websocket/test-ws-bidir-proxy.test.ts, and 23 files fromtest/js/node/test/parallel:test-http-connect*,test-http-after-connect,test-http-eof-on-connect,test-http-proxy.js,test-http-pause*,test-http-upgrade-*,test-http-server-request-timeout-upgrade. All pass, with one exception that main has too: innode-http-connect.test.ts,tests should run on bunexceeds its 5 s default limit under a local debug build, because the sub-suite it spawns takes about 6 s (#40897 has the fix). An https server shows the same failure before the fix and the same pass after it (manual probe, not in the test file).no test proof · iteration 3 · 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