node:http: end HTTP/1 fallback connections after a response that must close them - #37767
node:http: end HTTP/1 fallback connections after a response that must close them#37767robobun wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughThe HTTP/1 fallback now records keep-alive state, closes sockets when required, and handles peer EOF through the shared connection state. Tests cover HTTP/1.0 closure, response controls, half-closes, HTTP/1.1 reuse, and the HTTP/2 HTTP/1 fallback. ChangesHTTP/1 connection lifecycle
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: reproduced on bun 1.4.0 and current main with the Current revision (bf97f18, rebased on main): the fallback stamps the request's keep-alive decision from the parser like the native dispatcher, seeds |
|
Updated 3:06 PM PT - Aug 12th, 2026
❌ @robobun, your commit 761bb6e has 5 failures in
🧪 To try this PR locally: bunx bun-pr 37767That installs a local version of the PR into your bun-37767 --bun |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes HTTP/1 connection-close semantics on the Node-compat fallback path (with a called-out deliberate divergence from Node for the HTTP/1.0 + handler-set Connection: keep-alive case), a human look would still be worthwhile.
What was reviewed:
- The seeding/reading of
kMustCloseConnectionmirrors the native dispatcher exactly (_http_server.ts:867-868andonResponseFinishHandleSocketat :2476), so the transport now follows the same decision as the renderedConnectionheader. - The new
require("node:_http_server")is lazy (insideconnectionListenerHTTP1), so no circular-require issue with_http_server.ts:92. - The dropped llhttp
shouldKeepAliveargument's request-level cases (HTTP/1.0 no keep-alive, HTTP/1.1 +Connection: close) are re-covered byrequestShouldKeepAliveand guarded by explicit test rows. - The keep-alive positive case is asserted, so this doesn't accidentally start closing HTTP/1.1 kept-alive connections.
Extended reasoning...
Overview
The PR aligns the JS HTTP/1 fallback path (server.emit('connection', socket) and the http2 allowHTTP1 ALPN branch) with the native node:http dispatcher's close-after-response decision. It stops consulting llhttp's shouldKeepAlive argument and instead seeds res[kMustCloseConnection] from requestShouldKeepAlive(req), then ends the socket in handle.onfinished when that flag is set — the same flag renderNativeHeaders writes to for response-level close reasons. Two internals (kMustCloseConnection, requestShouldKeepAlive) are added to node:_http_server's default export for the fallback to consume. ~15 lines of logic change plus ~130 lines of new tests across the http and http2 test files.
Security risks
None identified. This governs whether an already-answered connection is ended, not authentication, input parsing, or resource limits. The change makes the transport stricter (closes where it previously stayed open), which if anything reduces the surface for a client relying on a connection the server advertised as closed.
Level of scrutiny
Medium-high. It's a small, pattern-following change (the seed-then-read of kMustCloseConnection is copied line-for-line from the native dispatcher at _http_server.ts:867-868 / :2476), and the fallback path is a secondary compat surface rather than the primary Bun.serve/native http.Server path. But it is HTTP connection-lifecycle semantics in the Node compat layer, and the description explicitly names a deliberate divergence from Node (HTTP/1.0 request + handler-set Connection: keep-alive now closes on the fallback, matching Bun's native path rather than Node). That's the kind of compat trade-off a maintainer should sign off on rather than an automated review.
Other factors
- I checked for a circular require:
_http_server.tstop-level-requires the fallback (line 92), but the fallback's newrequire('node:_http_server')is inside theconnectionListenerHTTP1function body, so it's lazy and safe. - The
keepAlivelocal passed tocreateHttp1FallbackResponseHandleonly affects thehead === nullheader-rendering branch (off the ServerResponse path per its comment); on the ServerResponse path the auto-header bits fromrenderNativeHeadersdrive theConnectionheader, so header and transport now derive from one source. - Tests are a table over duplexPair with a positive keep-alive case guarding against over-closing, plus regression rows for the two request-level cases that moved off the llhttp flag, and a real TLS
allowHTTP1case. The duplexPair test relies on'end'arriving before thesetImmediatethat sends the second request (the author comments on this ordering); it looks sound for duplexPair's push-in-nextTick semantics but is worth a second pair of eyes. - The description notes an interaction with #36991 (pipelining); the change here is independent but a maintainer may want to sequence them.
- Adding
kMustCloseConnection/requestShouldKeepAlivetonode:_http_server's exports follows the existing precedent ofkConnectionsCheckingIntervalin the same object.
|
On the ordering the review asks about in the The HTTP/1.0 + handler-set |
6ce6acb to
39a0da8
Compare
… close them
The JS HTTP/1 fallback (server.emit('connection', socket) and http2's
allowHTTP1) ended a connection after a response only when llhttp's
keep-alive flag for the request was false. That flag honors an HTTP/1.0
request's Connection: keep-alive, while renderNativeHeaders answers every
HTTP/1.0 request with Connection: close, so the connection stayed open
after a response that said close. The close reasons recorded on the
response itself were never acted on either: kMustCloseConnection as set by
renderNativeHeaders (a handler-set Connection: close header,
res.shouldKeepAlive = false, 204/304 with Transfer-Encoding, ...) and the
_last that onHttp1SocketEnd wrote for a peer FIN under httpAllowHalfOpen,
which nothing reads.
Give the fallback the native dispatcher's structure: stamp
req[kReqShouldKeepAlive] from the parser's verdict (forced off for
HTTP/1.0, like the native stamp), so renderNativeHeaders renders the
Connection header from the same decision that seeds kMustCloseConnection;
have onHttp1SocketEnd set kMustCloseConnection instead of _last; and end
the socket from the response's 'finish' listener whenever the flag is set,
which is where Node's resOnFinish consumes _last. Stamping from the parser
rather than re-reading req.headers keeps a Connection: close request
closing even when req.headers does not retain the header.
39a0da8 to
bf97f18
Compare
|
Reworked after self-review; the description is updated. Changes since the first revision:
|
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes HTTP/1 connection-lifecycle behaviour on the fallback path, adds kMustCloseConnection to node:_http_server's exports, and documents both a deliberate divergence from Node (HTTP/1.0 + handler-set keep-alive still closes) and an overlap with #36991's pipelining work, a human look would still be worthwhile.
Reviewed: the kReqShouldKeepAlive stamp and kMustCloseConnection seed match the native dispatcher's at _http_server.ts:790 / :874, and requestShouldKeepAlive() reads the stamp so the advertised header and the close now share one source. Confirmed renderNativeHeaders sets kMustCloseConnection for every response-level case the new table asserts (explicit Connection: close, shouldKeepAlive=false with and without the header, 204/304 + TE, close-delimited). Checked that moving socket.end() from handle.onfinished to the 'finish' listener does not lose the close-delimited case — handle.end() still ends the socket itself for that. The _last → kMustCloseConnection swap in onHttp1SocketEnd reaches the same consumer, and _last had no reader on this path.
Extended reasoning...
Overview
The PR aligns the JS HTTP/1 fallback (server.emit('connection', socket) and the allowHTTP1 side of http2.createSecureServer) with the native node:http dispatcher's connection-close decision. Production changes: ~20 lines in src/js/internal/http1_server_fallback.ts (stamp req[kReqShouldKeepAlive] from llhttp forced off for HTTP/1.0, seed res[kMustCloseConnection], move socket.end() to the response 'finish' listener gated on that flag, and set the same flag from onHttp1SocketEnd instead of the unread _last), a comment-only edit in _http_incoming.ts, and one added export in _http_server.ts. Tests: a 12-row duplexPair table in node-http.test.ts and a TLS allowHTTP1 case in node-http2.test.js.
Security risks
Low and in the safe direction. The change makes the fallback close connections it previously left open when the response advertised Connection: close, which reduces the surface for a client holding a connection the server believes it has closed. No parsing changes; the keep-alive verdict still comes from llhttp. No auth, crypto, or path handling touched.
Level of scrutiny
Medium. This is Node-compat connection-lifecycle logic on a real server path, not a config tweak. The fix is structurally the same as the native dispatcher (verified line-for-line: the stamp at _http_server.ts:790, the seed at :874, and the 'finish'-time socket.end() at onResponseFinishHandleSocket), and renderNativeHeaders already writes kMustCloseConnection for every response-level close reason the tests assert, so the new consumer covers them without new per-case code. But the description calls out a deliberate divergence from Node (HTTP/1.0 request answered with a handler-set Connection: keep-alive still closes here, matching Bun's native path rather than Node), and #36991 is stated to touch the same listener — both are decisions a maintainer should sign off on rather than a bot.
Other factors
Test coverage is thorough (request-level, response-level, half-open, and keep-alive guard rows; all outcomes cross-checked against Node v26.3.0 per the description). Adding kMustCloseConnection to node:_http_server's default export widens a semi-public module's surface with a Bun-internal symbol; harmless for consumers but worth a maintainer's nod. The comment-cop bot flagged several multi-line comments; they read as Node-source citations rather than workaround justifications, so I did not weigh them as blocking. CI on the timeline shows build-infra failures on an intermediate commit rather than test failures on this diff.
|
Nothing further to change from the review. For whoever picks this up: the build that ran against bf97f18 (#93418) failed in three build lanes on GitHub tarball downloads (mimalloc / WebKit / lol-html returning 503), which took the linux x64 and darwin aarch64 test lanes down with it; the lanes that did run were green on the new tests. Re-ran CI with an empty commit now that downloads work again. The comment-cop threads are resolved: the first batch by trimming the comments in bf97f18, the remaining three are the pre-existing resOnFinish / kReqShouldKeepAlive comments plus the three-line note explaining the stamp, which I kept on purpose. |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The fallback now mirrors the native dispatcher's kReqShouldKeepAlive stamp and kMustCloseConnection seed/consume exactly, and the test table pins each close reason against Node. Because it changes HTTP server connection-lifetime semantics and interacts with the same 'finish' listener as the pending pipelining work in #36991, a human look is still worthwhile.
Checked: the versionMajor === 1 && versionMinor === 0 guard matches the native isAncientHTTP stamp (llhttp already reports shouldKeepAlive === false for 0.9); moving socket.end() from handle.onfinished to the 'finish' listener runs after detachSocket but is otherwise the same synchronous res.end() flow, and the closeDelimited branch in handle.end() still ends the socket independently; kMustCloseConnection / kReqShouldKeepAlive are both exported and destructure correctly.
Extended reasoning...
Overview
The PR makes the JS HTTP/1 fallback server path (server.emit("connection", socket) and http2.createSecureServer({ allowHTTP1: true })) end connections after responses that must close them, where previously it only consulted llhttp's raw shouldKeepAlive flag. Runtime changes are ~30 lines across three files: http1_server_fallback.ts stamps req[kReqShouldKeepAlive] from the parser (forced off for HTTP/1.0), seeds res[kMustCloseConnection], and reads that flag in the response 'finish' listener to call socket.end(); onHttp1SocketEnd sets the same flag instead of the unread _last. _http_server.ts exports kMustCloseConnection. _http_incoming.ts only re-words a comment. Tests add a 12-row duplexPair table in node-http.test.ts and one TLS case in node-http2.test.js.
Security risks
None identified. The change makes the fallback close connections more eagerly (matching what the response already advertised), never less. No parsing of untrusted input is added; the keep-alive decision comes from llhttp's existing verdict.
Level of scrutiny
Medium-high. The runtime diff is small and structurally copies what the native dispatcher already does at _http_server.ts:790 and :874/:2483, so there is no novel logic — but HTTP server connection lifetime is a production-critical path where a wrong close decision either leaks connections or truncates responses. The description also notes an intentional divergence from Node (HTTP/1.0 + handler-set Connection: keep-alive still closes, matching Bun's native path) and overlap with #36991's pipelining work on the same listener; both are worth a maintainer confirming.
Other factors
Test coverage is thorough: 9 of 12 table rows fail on main and all match Node v26.3.0; guards pin the cases that moved off the raw llhttp flag (HTTP/1.0 no-keep-alive, HTTP/1.1 Connection: close) and the keep-alive path staying open. The setImmediate-vs-'end' ordering in the table was explicitly reasoned through and loop-tested. All comment-cop threads are resolved. I traced requestShouldKeepAlive() reading the new stamp, renderNativeHeaders setting kMustCloseConnection for the handler-set / 204+TE / _removedTE / shouldKeepAlive=false cases the tests cover, and confirmed both symbols are exported from their modules.
Problem
http.Serverwithserver.emit("connection", socket), or the HTTP/1 side ofhttp2.createSecureServer({ allowHTTP1: true }), answers an HTTP/1.0 request that sentConnection: keep-alivewithConnection: closebut leaves the connection open and keeps serving requests on it. Node and bun's nativehttp.Serverend it after the response.res.setHeader("Connection", "close"),res.shouldKeepAlive = false, a204/304withTransfer-Encoding, or a peer half-close underserver.httpAllowHalfOpenall leave the connection open. Node and the native path end it in every one of these cases.Connectionheader it sent was computed by a different rule, and the flag on which the response records its own close reasons was never read. The peer-FIN handler set a field nothing reads.Fix
Connectionheader and the close decision come from one value. The parser's verdict is used instead ofreq.headersbecausereq.headerscan drop fields of a large request (node:http2: deliver all headers in the allowHTTP1 fallback past 31 headers #33540).resOnFinishconsumes_last.Connection: keep-aliveis still ended, as on the native path; Node would keep it open when aContent-Lengthis present.maxRequestsPerSocket(node:http: enforce maxRequestsPerSocket on HTTP/1 fallback connections #37749) and pipelining (node:http: queue pipelined responses on fallback connections instead of throwing ERR_HTTP_SOCKET_ASSIGNED #36991) are not touched.duplexPair, nine rows failing on main and three passing as guards, each expected outcome also checked against Node v26.3.0; anallowHTTP1TLS test that times out on main; the upstream node http tests listed in the original run against a debug build.Background
http.Serveris normally backed by a native server, but a socket given to it directly (or thehttp/1.1ALPN branch of anallowHTTP1http2 server) is parsed in JS with llhttp and dispatched to the sameIncomingMessage/ServerResponseclasses.shouldKeepAliveverdict counts HTTP/1.0 plusConnection: keep-aliveas keep-alive. This server never reuses HTTP/1.0 connections, so the native dispatcher overrides the verdict for 1.0 and this PR makes the fallback do the same.kReqShouldKeepAliveis stamped on the request and drives the renderedConnectionheader;kMustCloseConnectionis set on the response by anything that decides the connection must close and is consumed when the response finishes. Together they are bun's equivalent of Node'sres._last.httpAllowHalfOpenkeeps the connection open after the peer sends FIN so an in-flight response can still be written; Node ends the connection once that response finishes.Original description
Problem
Connections served by the JS HTTP/1 fallback (
src/js/internal/http1_server_fallback.ts: sockets fed into anhttp.Serverwithserver.emit("connection", socket), and thehttp/1.1ALPN side ofhttp2.createSecureServer({ allowHTTP1: true })) answer every HTTP/1.0 request withConnection: close, but keep the connection open when the request carriedConnection: keep-alive, and go on serving further requests on it:bun 1.4.0 / main prints two
200responses, both carryingConnection: close, and never ends the connection. Node v26.3.0 (and this branch) printserver ended connection, the first response, and""for the second request. Bun's nativehttp.Serverpath also ends HTTP/1.0 connections after the response.The fallback also ignores every close reason recorded on the response itself. With an HTTP/1.1 request,
res.setHeader("Connection", "close"),res.shouldKeepAlive = false(with or without theConnectionheader removed), or a204/304carrying aTransfer-Encodingheader all leave the connection open, and withserver.httpAllowHalfOpen = truea peer that half-closes while a response is in flight never gets the connection ended after that response. Node and the native path end the connection in all of these cases.Cause
connectionListenerHTTP1decided whether to end the connection from one input only: llhttp'sshouldKeepAliveargument, which is true for HTTP/1.0 +Connection: keep-alive, andhandle.onfinishedended the socket only when it was false. MeanwhilerenderNativeHeadersrenders theConnectionheader fromrequestShouldKeepAlive()(HTTP/1.0 is never kept alive on this server) and records the response-level close reasons inres[kMustCloseConnection], which the native dispatcher seeds from its request-level decision and acts on when the response finishes; the fallback never read or seeded that flag. ItsonHttp1SocketEndported Node'ssocketOnEndby settingres._last, which nothing in the handle-backedServerResponsereads.Fix
Give the fallback the native dispatcher's structure:
req[kReqShouldKeepAlive]at dispatch from llhttp's verdict, forced off for HTTP/1.0 exactly like the native stamp in_http_server.ts.renderNativeHeadersalready reads the stamp back throughrequestShouldKeepAlive(), so the advertisedConnectionheader and the close decision come from one value. The stamp is taken from the parser rather than by re-readingreq.headersbecause the parser saw every header field:req.headerson this path does not keep all of them for large requests (node:http2: deliver all headers in the allowHTTP1 fallback past 31 headers #33540), and aConnection: closeamong the dropped fields must still close the connection (and is now also advertised asclose; on main the transport closed but the header saidkeep-alive).res[kMustCloseConnection]from that stamp, letonHttp1SocketEndset the same flag instead of the unread_last, and end the socket from the response's'finish'listener whenever the flag is set, which is where Node'sresOnFinishconsumes_last. That one consumer covers the request-level reasons, everythingrenderNativeHeadersrecords, and a FIN that lands before or during the response.kMustCloseConnectionis added tonode:_http_server's exports for this.This is correct to have because the transport now follows the header the response actually advertised, both derived from one decision, and because a peer FIN under
httpAllowHalfOpennow reaches the same consumer as in Node. One consequence to be aware of: a handler that writes its ownConnection: keep-aliveheader on a response to an HTTP/1.0 request still gets the connection ended on the fallback, as it does on the native path (this server never reuses HTTP/1.0 connections); Node would keep it open when the response also has aContent-Length. Not touched here:maxRequestsPerSocket, which like Node only advertises close (#37749), and how the connection is ended (socket.end(), as before). #36991 adds a similarkMustCloseConnectioncheck to this listener as part of its pipelining work; this PR is the standalone fix for the close decision and is independent of it.Tests
test/js/node/http/node-http.test.ts: aserver.emit("connection")table over aduplexPairrecording the advertisedConnectionheader, the number of responses received, whether the server ended the connection, and the requests that reached the handler (a second request is sent once the first response is in, unless the client half-closed). Failing on main: HTTP/1.0 + keep-alive;Connection: closecarried among 40 other request headers (main advertiseskeep-alive); handler-setConnection: close;shouldKeepAlive = false, with and without theConnectionheader removed (the latter pins the no-header wire shape);204+Transfer-Encoding; HTTP/1.0 with a handler-setConnection: keep-alive(pins the consequence above); and the twohttpAllowHalfOpencases (handler answers after the peer's FIN, and a synchronous answer to a request that arrived together with the FIN). Passing on main as guards: HTTP/1.0 without keep-alive and HTTP/1.1 +Connection: close(the cases that moved off the llhttp flag), and a plain HTTP/1.1 request whose connection must stay open and serve the second request. Every row's expected outcome is also what Node v26.3.0 produces for the same scenario.test/js/node/http2/node-http2.test.js: anallowHTTP1server answering an HTTP/1.0 keep-alive request over TLS must sendConnection: closeand end the connection (times out waiting for'end'on main).Also ran the full
node-http.test.ts/node-http2.test.jsfiles and the upstreamtest-http-generic-streams,test-http-server,test-http(s)-*-per-stream,test-http-server-unconsume-consume,test-http2-allow-http1andtest-http2-https-fallback*tests against the debug build; the only failure was the pre-existinglocalhostproxy test in this container.