Repository navigation
Conversation
…rts EOF
A transport that is not a net.Socket (an options.createConnection Duplex,
a stream handed to server.emit('connection')) reported EOF and nothing
happened: the session subscribed to data, drain, close, error and timeout
only. A pending request never settled and the session stayed alive.
Node wraps such a transport in a JSStreamSocket, a net.Socket with
allowHalfOpen off, so the EOF ends the transport's writable side and
destroys it, and that close tears the session down. Mirror net's
destroySoon on the transport itself.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. WalkthroughChangesHTTP/2 generic transport EOF handling
Suggested reviewers: Merge Risk: ⚪ Minimal · up to Custom HTTP/2 transports now tear down cleanly after EOF while preserving pending writes, with client and server regression coverage. The change is ready to merge. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this and didn't find any bugs. Because it changes http2 session teardown semantics for user-supplied Duplex transports and picks instanceof net.Socket as the guard (a deliberate divergence from Node's _handle.isStreamBase check), a human look from someone familiar with the http2 lifecycle would still be worthwhile.
What was reviewed:
watchGenericTransportEnd/destroyGenericTransportSoonNTagainst Node'sonReadableStreamEnd/destroySoonsequence — the end→finish→destroy ordering andnextTickdeferral match the cited upstream.- The guard leaves
net.Socket/TLSSockettransports untouched, so the existingallowHalfOpenpath is unaffected;netis captured at module load. socket.once("finish", socket.destroy)—'finish'emits with no args, sodestroy()receives no spurious error;oncebindsthisto the socket.- New tests: bounded
setImmediatepoll instead of sleeps, error events wired to the recorded sequence, per-object ordering asserted separately (sidesteps the known cross-object ordering divergence),describe.concurrentfor independence.
Extended reasoning...
Overview
This PR adds an 'end' listener to non-net.Socket transports in both ClientHttp2Session and ServerHttp2Session constructors (src/js/node/http2.ts). When such a transport's readable side reaches EOF, the new destroyGenericTransportSoonNT helper ends the writable side and destroys the transport (immediately if writableFinished, else on 'finish'), which then flows into the existing 'close' handler to tear the session down. This mirrors what Node achieves by wrapping generic Duplex transports in a JSStreamSocket with allowHalfOpen: false. Two new tests in test/js/node/http2/node-http2.test.js cover the client and server sides over an in-memory duplexPair().
Security risks
None identified. The change adds no new user-controlled input parsing, no auth/crypto/permission logic, and no network-facing surface beyond what already exists. It only affects the (already niche) path where a caller supplies their own Duplex via createConnection or server.emit('connection', duplex). The instanceof net.Socket guard means the common TCP/TLS paths are untouched.
Level of scrutiny
Moderate. The production change is small (~24 lines across three named helpers plus two one-line call sites), well-cited against Node v26.3.0 source, and follows the file's existing conventions (process.nextTick, named functions rather than inline closures, net captured at module load). However, http2 session/stream lifecycle and event ordering is historically subtle in Bun — the PR description itself references three open divergences (#33802, #38124, and session.closed staying false). The instanceof net.Socket guard is a reasoned but non-literal port of Node's !socket._handle.isStreamBase check, and that design choice deserves a maintainer's eye. The tests deliberately assert per-object event order rather than the full interleaving to avoid depending on a known pre-existing divergence, which is the right call but also means the exact cross-object timing isn't locked down here.
Other factors
The tests follow the repo's conventions well: added to the existing node-http2.test.js, use a bounded setImmediate poll (settle) rather than sleeps, wire error events into the recorded sequence, use describe.concurrent, and assert a combined state object with .toEqual. No CODEOWNERS entry covers the changed paths. The bug hunt ran to dry_streak with no findings and no ruled-out candidates. Given the subtlety of http2 teardown timing and the guard being a deliberate architectural choice rather than a mechanical port, deferring to a human reviewer is the safer call over auto-approval.
|
On the guard the review flags for a human (
|
This branch has not been deployed
2 participants
Problem
http2.connect(url, { createConnection: () => duplex }): the peer ends its side while a request is pending. Bun emits nothing, ever. The stream and the session never close,session.destroyedstaysfalse, and the transport is not destroyed. Node closes the stream, closes the session, and destroys the transport.server.emit('connection', duplex)) behaves the same way. The client's EOF leaves the server stream and session alive.data,drain,close,errorandtimeout(src/js/node/http2.ts:4491and:5721). There is no'end'listener, so the EOF reaches nothing.Fix
'end'on a transport that is not anet.Socket, then apply net'sdestroySoonto it: end the writable side, and destroy it once that side finishes. The existing'close'path then tears the session down.JSStreamSocket, which is anet.SocketwithallowHalfOpenoff. A realnet.Socket(aTLSSockettoo) already applies its ownallowHalfOpen, so the guard leaves those transports on their current path.test/js/node/http2/node-http2.test.js, two new tests, both fail on stock bun. Also all 256 upstreamtest-http2-*tests, the 10 files intest/js/node/http2,serve-http2*, the http2 regression tests, and grpc-js.Background
Http2Sessionconstructor wraps a transport that has no StreamBase handle in aJSStreamSocket. Bun binds the user's Duplex directly, so the session has to apply the socket-layer behaviour itself.allowHalfOpenoff means the peer's EOF also ends our writable side and then destroys the stream. Bun'snet.Socketalready does this, which is why only non-socket transports are affected.stream.duplexPair), an SSH channel, a WebSocket stream, a proxy-tunnel Duplex,server.emit('connection', duplex), and grpc-jscreateConnectionInjector().injectConnection(duplex). A client behind an HTTP proxy does not: those agents hand bun anet.Socketor aTLSSocket.Notes
The repro
node v26.3.0 prints both lines. Stock bun prints nothing and exits 0 with the request unanswered.
Parity measured against node v26.3.0
Each shape was run on node v26.3.0, on stock bun 1.4.3, and on this build.
Duplex.from({ readable, writable })transportERR_HTTP2_INVALID_STREAMon the late respondstream.finished(req)after the peer's EOFERR_STREAM_PREMATURE_CLOSE, like nodeGuard checks, which must not change:
net.Sockettransport withallowHalfOpen: true, peer FIN: the session stays alive and a ping still reaches the peer. Client and server. Same on node, stock bun, and this build.net.SocketandTLSSockettransports with the defaultallowHalfOpen: false, client and server, plain TCP and TLS: unchanged.Divergences that remain, all of them older than this change
'close'can precede the stream's'close'. Reproducible over plain TCP on stock bun, so it is not from this change. node:http2: close streams with the session's destroy code and order their events before the session's #33802 covers it.Http2Streamdoes not emit'finish'and reportswritableFinished === false. Same on stock bun through the workaround path (duplex.on('end', () => duplex.destroy())).session.closedstaysfalseafter any transport closes, including a plain TCP socket. Handed off separately.session.destroy(err)over a Duplex propagates the error to the peer differently from node. Same on stock bun, different tail. node:http2: report an injected socket's error on the session instead of leaving it uncaught #38124 covers it.Why the guard is
instanceof net.SocketNode's condition is
!socket._handle || !socket._handle.isStreamBase. Bun's_handleis a native socket object with noisStreamBase, so a literal port of that test would wrap every socket.instanceof net.Socketselects the same population: in node aJSStreamSocketis itself anet.Socket, andtls.connect({ socket: duplex })returns aTLSSocket, which is anet.Socketin both runtimes.Reviews
Self-reviewed. Four concerns survived: three about how this body states the reach of the change, which are addressed above, and one about the execution, which the guard checks and the parity table answer.
[human-review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file