Conversation
Connections served by internal/http1_server_fallback (http2 allowHTTP1
and sockets fed in through server.emit("connection")) only copied
server.maxRequestsPerSocket into the Keep-Alive header; nothing counted
the requests on the connection. Port the counting from Node's
parserOnIncoming: the response that reaches the limit gets
maxRequestsOnConnectionReached (rendered as Connection: close) and every
request past it emits 'dropRequest' and is answered with a 503 instead of
reaching 'request'.
|
Warning Review limit reached
Next review available in: 2 minutes 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 (4)
Comment |
|
Status: reproduced with the duplexPair snippet in the description (node v26.3.0 answers 200 + 503 with one 'dropRequest', bun 1.4.0 answers 200 + 200 with none) and over Self-review done: it turned up the duplicated response reader between the two test files (removed in 03216a8, the http2 test now follows its neighbours) and the untested Upgrade-after-limit cell (pinned in the same commit; the native dispatcher answering that case with a 503 is a pre-existing divergence from Node, reported separately). CI for the current head is running; on the previous build every lane that ran was green, with only unrelated retry flakes and two darwin 26 aarch64 shards that never got an agent. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, well-scoped port of Node's parserOnIncoming maxRequestsPerSocket counting into the JS HTTP/1 fallback path.
What was reviewed:
- The new block matches both Node's
lib/_http_server.jsparserOnIncomingand Bun's native dispatcher at_http_server.ts:904-920/1012-1015(sametypeof === "number" && > 0gate, same<=/<split, samedropRequestargs, 503 before Expect routing). - HTTP/1.0 keep-alive bypass concern was checked and ruled out — Node itself gates this block on
httpVersionMajor === 1 && httpVersionMinor === 1, so the version guard is a faithful port, not a divergence. renderNativeHeadersat_http_server.ts:2379and_http_outgoing.ts:514already turnmaxRequestsOnConnectionReachedintoConnection: close, so no further wiring was needed.- Tests await real response framing over
duplexPair/ TLS (no sleeps), cover per-connection isolation, Expect ordering, and theallowHTTP1TLS path, and clean up viausing/try/finally.
Extended reasoning...
Overview
This PR ports Node's per-connection maxRequestsPerSocket enforcement into src/js/internal/http1_server_fallback.ts — the JS-side HTTP/1 parser used for http2.createSecureServer({ allowHTTP1: true }) ALPN fallback and for sockets fed in via server.emit("connection", socket). Previously the fallback only advertised max=N in the Keep-Alive header without counting anything. The fix adds a closure-scoped requestsCount, sets res.maxRequestsOnConnectionReached at the limit, and answers over-limit requests with 'dropRequest' + writeHead(503); end() before the Expect routing. Four new tests cover both entry points.
Security risks
None. This is a Node.js-compat behavioral fix in a JS builtin. The change tightens (rather than loosens) request handling by enforcing an existing documented server limit. No new parsing of untrusted input, no auth/crypto/permissions surface.
Level of scrutiny
Low-to-moderate. The runtime change is ~20 lines in a JS fallback path, and it is a near-verbatim port of logic that already exists in two reference implementations: Node's parserOnIncoming in lib/_http_server.js and Bun's own native dispatcher at src/js/node/_http_server.ts:904-920 and :1012-1015. I diffed all three: the typeof === "number" && > 0 gate, the <= (reached) vs < (over) split, the dropRequest(req, socket) args, and the 503-before-Expect ordering all line up. maxRequestsOnConnectionReached is already consumed by renderNativeHeaders (_http_server.ts:2379) and _storeHeader (_http_outgoing.ts:514) to emit Connection: close, so the flag alone is sufficient — as the PR description states.
Other factors
- The finder-raised concern that HTTP/1.0 keep-alive requests bypass the limit was examined: Node's own
parserOnIncomingwraps this entire block inif (req.httpVersionMajor === 1 && req.httpVersionMinor === 1), so gating on 1.1 is the correct Node-compat behavior, not a bug. maxRequestsPerSocketis read fresh fromserveron each request (destructured inside the callback), matching Node's per-request read and the native dispatcher.- The counter lives in the connection listener's closure, so it is inherently per-connection; the second test verifies two concurrent emitted connections count independently.
- Tests are event-driven (buffer bytes and parse response framing, reject the awaited promise on
end/close), assert exact status/Connection/Keep-Alive/body tuples, cover the negative contract (nocheckContinue, no 100 Continue on the dropped Expect request), and release resources viausingandtry/finally. The http2 test starts a realallowHTTP1TLS server on port 0 and negotiateshttp/1.1ALPN. - No prior human reviews or outstanding comments; only a coderabbit rate-limit notice.
|
Updated 10:06 AM PT - Aug 12th, 2026
✅ @robobun, your commit f26690a1c82ea1aa29aa57344819b9c0c9893ca6 passed in 🧪 To try this PR locally: bunx bun-pr 37749That installs a local version of the PR into your bun-37749 --bun |
The http2 allowHTTP1 test now follows the shape of its neighbours instead of carrying a second copy of the response reader used by the http tests, and a test pins that an accepted Upgrade is handed off even once the limit is reached, which is where the fallback follows Node's order.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — focused Node-compat fix that ports parserOnIncoming's per-connection request counter into the JS HTTP/1 fallback path.
What was reviewed:
- The new counter/503/
dropRequestblock matches both Node's_http_server.jsand Bun's own native path at_http_server.ts:904-919(sametypeof === "number" && > 0guard, same<=/<split). - The HTTP/1.1 version gate was raised as a concern and ruled out — Node's
parserOnIncomingwraps the identical block inif (req.httpVersionMajor === 1 && req.httpVersionMinor === 1). - Placement before the
Expectrouting and after the upgrade hand-off matches Node; both orderings are pinned by tests. Http2SecureServeralready storesmaxRequestsPerSocket(http2.ts:6806) andrenderNativeHeadersalready turnsmaxRequestsOnConnectionReachedintoConnection: close(_http_server.ts:2379), so no further wiring was needed.
Extended reasoning...
Overview
The PR closes a Node-compat gap: server.maxRequestsPerSocket was enforced on natively accepted http.Server connections but ignored on the two paths that route through internal/http1_server_fallback — sockets injected via server.emit("connection", socket) and the HTTP/1.1 ALPN fallback of http2.createSecureServer({ allowHTTP1: true }). The fix is ~18 lines in connectionListenerHTTP1: a closure-scoped requestsCount, and in the headers-complete callback (after upgrade hand-off, before Expect routing) the same increment/compare block Node has in parserOnIncoming. Five new tests cover the response sequence at/past the limit, per-connection isolation of the counter, ordering vs Expect: 100-continue, ordering vs Upgrade, and the allowHTTP1 TLS path.
Security risks
None. Enforcing a previously-ignored per-socket request cap is strictly a hardening change; the 503 path uses the existing res.writeHead/res.end machinery and emits an event the server already documents. No parsing of untrusted input was added.
Level of scrutiny
Medium. This is a JS built-in Node-compat module, not native code, and the change mirrors two existing references line-for-line: Node's lib/_http_server.js parserOnIncoming and Bun's own native dispatcher at src/js/node/_http_server.ts:904-919. I verified the > 0 guard, the <= vs < split, the HTTP/1.1 version gate, and the placement relative to upgrade/Expect all match both references. maxRequestsOnConnectionReached is already consumed by renderNativeHeaders to emit Connection: close, and Http2SecureServer already carries maxRequestsPerSocket from http1Options, so the fix composes with existing plumbing without new wiring.
Other factors
The one candidate issue the finder raised — that gating on HTTP/1.1 diverges from Node — was checked against Node's source and refuted: Node wraps the identical block in the same version check. Test coverage is thorough (exact status/Connection/Keep-Alive values, dropRequest args including the socket identity, per-connection counting, and both entry points). Tests await real events with failure paths wired to reject, use duplexPair (already imported) rather than sleeps, and follow neighbouring allowHTTP1 test patterns in the http2 file. The comment-cop feedback about the long comment was addressed in cb1fb34 and the thread is resolved. The PR description documents that all five tests fail on the release build and pass with the change, and that the relevant upstream Node parallel tests still pass.
On hosts where localhost resolves to ::1 only, the proxy server bound ::1 while the client dialed 127.0.0.1 and the test failed with ECONNREFUSED regardless of the code under test. The exampleSite it proxies to already listens on 127.0.0.1.
There was a problem hiding this comment.
LGTM — focused Node compat port that mirrors both Node's parserOnIncoming and Bun's own native dispatcher block.
What was reviewed:
- The new limit check sits where Node's does (after upgrade handoff, before Expect routing) and gates on HTTP/1.1 +
> 0like Node and_http_server.ts:904. renderNativeHeadersat_http_server.ts:2379does readmaxRequestsOnConnectionReachedto forceConnection: close, andServerResponse#endcallsreq._dump()so the dropped request's body is discarded before the fallback'skOnBody(which guards on_dumped) sees it.requestsCountlives in the per-connection listener closure, so it is per-socket;maxRequestsPerSocketis re-read each request like Node.Http2SecureServeralready carriesmaxRequestsPerSocket, so the allowHTTP1 test needs no extra wiring.
Extended reasoning...
Overview
Ports server.maxRequestsPerSocket enforcement into src/js/internal/http1_server_fallback.ts, the JS HTTP/1 path used by server.emit('connection', socket) and http2.createSecureServer({ allowHTTP1: true }). The runtime change is ~20 lines: a per-connection requestsCount counter in the connectionListenerHTTP1 closure, and the same at-limit/over-limit block Node has in parserOnIncoming — set res.maxRequestsOnConnectionReached at the limit, and past it emit 'dropRequest'(req, socket) + writeHead(503); end() before the Expect routing. The rest is tests (four in node-http.test.ts over duplexPair-emitted connections, one in node-http2.test.js over ALPN http/1.1) plus a two-line hermeticity fix pinning node-http-proxy.js to 127.0.0.1.
Security risks
None identified. This tightens behavior (over-limit requests are rejected with 503 instead of dispatched), only on the JS fallback path, and only when the user has explicitly set maxRequestsPerSocket > 0. No parsing of untrusted input was added; the value comes from server configuration.
Level of scrutiny
Medium — it is runtime HTTP server logic, but in a narrow fallback path with a small, well-isolated change that duplicates an existing pattern. I compared the new block against three references: Node's parserOnIncoming (HTTP/1.1 gate, > 0 gate, placement before Expect, upgrades handed off before counting, no socket close), Bun's native dispatcher at _http_server.ts:904-920 (same <= / < split), and the downstream consumers (renderNativeHeaders reads maxRequestsOnConnectionReached to force Connection: close; ServerResponse#end calls req._dump(), and the fallback's kOnBody is gated on !req._dumped). All line up.
Other factors
The tests cover the variant matrix the review guide asks for: at-limit vs over-limit, per-connection isolation, ordering vs Expect: 100-continue, ordering vs Upgrade, and the allowHTTP1 TLS entry point. They await observable conditions (response bytes / socket 'end') rather than sleeping, wire failure events to reject, and clean up via using / finally. The one prior review comment (comment-cop on the long comment) was addressed in cb1fb34 and the thread is resolved. The proxy-test 127.0.0.1 pin is an unrelated but trivial hermeticity fix, called out in the description.
…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>
3 participants
Problem
server.maxRequestsPerSocketis ignored on the connections bun parses in JS: the HTTP/1.1 side ofhttp2.createSecureServer({ allowHTTP1: true }), and sockets handed to anhttp.Serverthroughserver.emit("connection", socket). With the limit set to 1, Node answers the second request on a connection with 503 and emits'dropRequest'; bun answers 200, emits nothing, and keeps advertising keep-alive.http.Serverconnections already enforce the limit, so only these two entry points diverge from Node.Keep-Aliveheader advertisemax=N. Nothing counted the requests on a connection, so the limit was never reached and every request was dispatched.Fix
Connection: close; every request past it emits'dropRequest'(req, socket)and is answered with a bare 503 instead of reaching'request'.Expectrouting (a dropped request gets no100 Continue), and only for HTTP/1.1. As in Node, the server does not close the connection itself. Nothing else needed wiring: the response layer already renders the reached flag asConnection: close, and the HTTP/2 server already stores the property.emit("connection"), one overallowHTTP1with anhttp/1.1ALPN client) fail on the current release build and pass with this change; the same scenarios were run against node v26.3.0 and match. They send requests one at a time because pipelining on these connections still hits the separate node:http: queue pipelined responses on fallback connections instead of throwing ERR_HTTP_SOCKET_ASSIGNED #36991.127.0.0.1on both ends. On hosts wherelocalhostresolves to::1only it failed withECONNREFUSEDregardless of the code under test. node:http: answer every request past maxRequestsPerSocket with its own 503 #33472 carries the same pin; whichever lands second drops it.Background
internal/http1_server_fallbackis bun's JS HTTP/1 parser. Connectionshttp.Serveraccepts itself are parsed natively; a connection that arrives as a JS stream (anemit("connection")socket, or an HTTP/2 server's TLS connection where the client negotiatedhttp/1.1) is parsed here. Per-request server behaviour therefore has to exist in both paths, and this one only existed natively.server.maxRequestsPerSocketis Node's cap on requests served over one keep-alive connection. Node advertises it asKeep-Alive: timeout=N, max=M, sendsConnection: closeon the response that reaches it, and answers anything past it with 503 plus a'dropRequest'event, without closing the socket itself.res.maxRequestsOnConnectionReachedis the flag Node sets on the response before'request'fires; bun's header rendering already turns it intoConnection: close, so setting it is the whole signal.Expect: 100-continue: Node hands Upgrade requests to'upgrade'before counting, and applies the cap before looking atExpect. The tests pin both orderings.[review] gate passed · iteration 1 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 1
evidence per changed file
Original description
What
server.maxRequestsPerSocketwas ignored on the connections served byinternal/http1_server_fallback: the HTTP/1.1 connections ofhttp2.createSecureServer({ allowHTTP1: true })and the sockets handed to anhttp.Serverthroughserver.emit("connection", socket). The natively acceptedhttp.Serverconnections already enforce it.The same thing happens over
allowHTTP1with a TLS client negotiatinghttp/1.1.Cause
connectionListenerHTTP1only copied the limit intores._maxRequestsPerSocket, which makes the Keep-Alive header advertisemax=N. Nothing counted the requests on the connection, sores.maxRequestsOnConnectionReachedwas never set and every request was dispatched.Fix
Port the counting from Node's
parserOnIncoming(lib/_http_server.js): a per-connection counter in the listener closure; when the limit is set, the response whose request reaches it getsmaxRequestsOnConnectionReached = true, and a request past it emits'dropRequest'(req, socket)and is answered withres.writeHead(503); res.end()instead of reaching'request'. The check sits where Node has it, before theExpectrouting, and like Node's block it applies to HTTP/1.1 requests.Nothing else was needed:
renderNativeHeadersin_http_server.tsalready turnsmaxRequestsOnConnectionReachedintoConnection: close, andServerResponse#endalready dumps the unread body of the dropped request. With the fix, the response at the limit and the 503 are byte for byte what Node writes (apart fromDate).Like Node (documented on
server.maxRequestsPerSocket: it setsConnection: close"but will not actually close the connection", and later requests get 503), the server does not close the connection itself; every further request on it gets its own 503 and'dropRequest'.Http2SecureServeralready storesmaxRequestsPerSocket, so the allowHTTP1 path needs no further wiring.Verification
test/js/node/http/node-http.test.ts,connectionListener maxRequestsPerSocket(4 tests overemit("connection")): keep-alivemax=on the response under the limit,Connection: closeplusmaxRequestsOnConnectionReachedon the one that reaches it, 503 plus'dropRequest'(with the rightreq.urland socket) on every request past it; the counter is per connection; an over-limit request carryingExpect: 100-continueis dropped without100 Continueor'checkContinue'; an Upgrade request after the limit is still handed to'upgrade', since Node hands upgrades off before counting (the native dispatcher 503s that case today, which is a separate pre-existing divergence, not changed here).test/js/node/http2/node-http2.test.js: the limit-reaching 200 and the over-limit 503 plus'dropRequest'over anallowHTTP1server with anhttp/1.1ALPN client.All five fail on the current release build and pass with this change; the scenarios they assert were run against node v26.3.0 and match.
node-http.test.ts, theallowHTTP1tests innode-http2.test.jsand the upstreamtest-http-keep-alive-{max-requests,drop-requests,pipeline-max-requests}.js,test-http-server-keep-alive-max-requests-null.js,test-https-keep-alive-drop-requests.js,test-http2-allow-http1.js,test-http2-https-fallback*.jsandtest-http2-server-http1-client.jspass as before.Also in here
test/js/node/http/node-http-proxy.js(therequest via http proxy, issue#4295test innode-http.test.ts) listened onlocalhostand dialedlocalhost. On hosts wherelocalhostresolves to::1only, the proxy binds::1while the client connects to127.0.0.1and the test fails withECONNREFUSEDregardless of the code under test (Node fails the same way, so the test was not hermetic). Both ends are now pinned to127.0.0.1, which is what theexampleSiteit proxies to already uses. #33472 carries the same two-line pin; whichever lands second drops it.Requests are sent one at a time in the tests because pipelined requests on these connections still hit the separate
ERR_HTTP_SOCKET_ASSIGNEDissue (#36991); the 503 reply composes with that PR's response queue like any other response written from the parser callback.