Repository navigation
Conversation
…lient session ClientHttp2Session#onError detached the socket from the session before it called destroy(error). The socket teardown inside destroy() then had no socket to end or destroy, so a transport that reports an error without destroying itself stayed open and leaked the connection and its fd. Keep the socket attached and let destroy() release it after it has ended and destroyed it.
|
Status: ready for review. CI is green on e601830 (build #113732, 181/181). How I reproduced it: 100 client sessions over Verified with Reviewed: this PR should stay open because it is a one-line Node-parity fix for a measured fd leak, and it composes with #38195 in either merge order. |
|
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 transport error teardown
Suggested reviewers: Merge Risk: ⚪ Minimal · up to HTTP/2 client transport errors now complete session and connection teardown rather than leaving sockets open. The targeted regression coverage confirms the expected cleanup behavior, with no current merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/js/node/http2.ts— nit: the guard comment in ClientHttp2Session#destroy() still says the destroyed getter "reads 'socket detached', which #onError sets before calling in here" — this PR removes that detach, so the comment now describes behavior that no longer exists and misleads the next reader about why the#destroyinglatch is used instead ofthis.destroyed. Fix: update or delete the stale clause so the comment matches the new #onError.Extended reasoning...
src/js/node/http2.ts:5784-5786 (inside ClientHttp2Session#destroy) explains the
#destroyinglatch by pointing at #onError nullingthis[bunHTTP2Socket]before calling destroy(). This diff deletes exactly that null-out (the whole point of the fix), leaving the comment factually wrong. A future maintainer reading it may reintroduce the early detach or misjudge the guard's purpose. Pre-existing text made stale by this change; comment-only, no runtime impact.Verification: nit — the clause is now factually wrong after this diff. src/js/node/http2.ts:5784-5786 reads: "Guard on a latch, not the destroyed getter: that getter reads "socket detached", which #onError sets before calling in here, and the error-carrying destroy must still run once." The
destroyedgetter does read socket-detached (line 5470:return this[bunHTTP2Socket] === null;), but this PR's…
…, fail the connect wait on an early session error, and release the client end in finally; trim the #onError comment and fix the stale destroy() latch comments
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — thanks for addressing the earlier feedback; all three points are resolved in the current diff.
What was reviewed:
- The one-line behavioral change in
ClientHttp2Session.#onError: dropping the earlythis[bunHTTP2Socket] = nullsodestroy()still holds the socket — matches Node'ssocketOnError→session.destroy(error)flow, and#onClosealready does the detach after teardown. - Sibling
ServerHttp2Session.#onError(line 4331) checked for the same pattern — it never detached, so no server-side change is needed; the two condensed idempotence comments are behavior-neutral (the#destroyinglatch is unchanged). - The new test: hermetic loopback
net.Serveronport: 0,'connect'await now rejects on session'error'/'close', boundedsetImmediatepoll fortransport.destroyed, andfinallynow tears downtransport/sessionbeforeserver.close().
Extended reasoning...
Overview
The PR removes a single line — this[bunHTTP2Socket] = null; — from ClientHttp2Session.#onError in src/js/node/http2.ts, so that when a transport error triggers session.destroy(error), the socket is still attached and the teardown can end() and destroy() it. It also corrects a comment typo (the cross-reference now points at the server session's #onError), condenses two long idempotence comments on destroy() to one line each with the Node source citation retained, and adds one regression test to test/js/node/http2/node-http2.test.js.
Since the previous review, four commits landed that address all three earlier inline comments: the five-line change-narration comment is now a one-line invariant ("The socket stays attached so destroy() can end and destroy it."), the test's 'connect' await now wires sessionError/sessionClosed to reject, and the finally block now destroys transport and session before server.close().
Security risks
None. This is a resource-leak fix in the node:http2 client teardown path; no auth, crypto, permission, or input-parsing surface is touched. The change makes cleanup strictly more thorough (the socket is now closed on the error path where it previously leaked).
Level of scrutiny
Low-to-moderate. The behavioral delta is one deleted line whose effect is well-understood: destroy() reads this[bunHTTP2Socket] to run its socket step, and nulling it beforehand skipped that step. The PR description cites Node's socketOnError and finishSessionClose, and the retained comment links Http2Session#destroy at v26.3.0. I confirmed the sibling ServerHttp2Session.#onError never had this detach, so the "fix the whole class" check is satisfied. The comment condensations touch no code — the #destroying latch guard is byte-identical.
Other factors
The new test is hermetic (local net.createServer speaking a minimal SETTINGS preface, port: 0), awaits real events with failure paths wired to reject, polls transport.destroyed with a bounded 200-tick setImmediate loop rather than sleeping, asserts the peer observes the close via peerClosed, and releases resources in finally in the right order. The PR reports the full test/js/node/http2/ suite and the test-http2-* parallel tests pass. No outstanding CHANGES_REQUESTED from other reviewers; the bot-resolved threads were verified addressed by reading the current diff. Exit reason was dry_streak.
|
@robobun please run the tests this PR adds or changes on Node.js v26.3.0 and report whether each one passes or fails. |
|
Ran it. This PR adds one test and changes no others: That file goes through
So the test passes on both Node v26.3.0 and Bun with this change, and fails on Bun without it at the assertion the fix is about. Nothing needed changing for Node. Ported script// node --test h2-transport-error.node.test.mjs | bun test ./h2-transport-error.node.test.mjs
import assert from "node:assert";
import http2 from "node:http2";
import net from "node:net";
import { test } from "node:test";
test("destroys the transport when a transport 'error' tears the client session down", async () => {
function frame(type, flags) {
const header = Buffer.alloc(9);
header[3] = type;
header[4] = flags;
return header;
}
let connectionsOpen = 0;
const { promise: peerConnected, resolve: resolvePeerConnected } = Promise.withResolvers();
const { promise: peerClosed, resolve: resolvePeerClosed } = Promise.withResolvers();
const server = net.createServer(connection => {
connectionsOpen++;
connection.on("close", () => {
connectionsOpen--;
resolvePeerClosed();
});
connection.on("error", () => {});
connection.write(frame(4, 0));
connection.once("data", () => connection.write(frame(4, 1)));
resolvePeerConnected();
});
let transport;
let session;
try {
const port = await new Promise((resolve, reject) => {
server.once("error", reject);
server.listen(0, "127.0.0.1", () => resolve(server.address().port));
});
session = http2.connect(`http://127.0.0.1:${port}`, {
createConnection: () => (transport = net.connect(port, "127.0.0.1")),
});
const injected = Object.assign(new Error("transport failed"), { code: "EFAIL" });
const sessionError = new Promise(resolve => session.once("error", resolve));
const sessionClosed = new Promise(resolve => session.once("close", resolve));
const connected = new Promise((resolve, reject) => {
session.once("connect", resolve);
sessionError.then(reject);
sessionClosed.then(() => reject(new Error("session closed before 'connect'")));
});
await Promise.all([connected, peerConnected]);
assert.strictEqual(connectionsOpen, 1);
transport.emit("error", injected);
assert.strictEqual(await sessionError, injected);
await sessionClosed;
assert.strictEqual(session.destroyed, true);
for (let tick = 0; tick < 200 && !transport.destroyed; tick++) {
await new Promise(resolve => setImmediate(resolve));
}
assert.strictEqual(transport.destroyed, true, "transport.destroyed");
await peerClosed;
assert.strictEqual(connectionsOpen, 0);
} finally {
transport?.destroy();
session?.destroy();
server.close();
}
}); |
This branch has not been deployed
2 participants
Problem
node:http2client session that takes a transport error destroys itself and leaves the transport open. The peer never sees a close. 100 sessions leak 100 connections and 200 fds. Node destroys the transport and leaks nothing.ClientHttp2Session#onError(src/js/node/http2.ts:5422on main) setsthis[bunHTTP2Socket] = nullbeforethis.destroy(error). The socket step insidedestroy()(GOAWAY,end(), thendestroy()) finds no socket and does nothing.emit('error'),autoDestroy:false, a wrapper that forwards an inner error. Anet.Socketthat errors natively destroys itself, which hides the leak.Fix
destroy()and drops it after the teardown, as#onCloseand the server session already do.destroy()owns the socket teardown, as in Node:socketOnErrorcallssession.destroy(error)with the socket attached, andfinishSessionCloseends and destroys it. An already destroyed socket is unaffected:end()is a no-op and the delayed destroy checkssocket.destroyed.test/js/node/http2/node-http2.test.js(new test, stock bun fails it), all oftest/js/node/http2/, and the 278 vendoredtest-http2-*files.Background
Http2Sessionruns over a transport: a socket it dials, or whateveroptions.createConnectionreturns. The session holds it underbunHTTP2Socket, andsession.destroyedreads that reference.#onErrorhandles the transport's'error'and callsdestroy(), which sends the GOAWAY, ends the socket, destroys it a turn later, then drops the reference.Notes
Repro (loopback,
/procfor the fd count): 100 client sessions overcreateConnection: () => net.connect(), each transport emits one'error'after the session connects. The far end is a raw TCP server that speaks an empty SETTINGS frame plus an ACK.last.transportDestroyedis read when the session emits'close'. It is stillfalsethere because bun emits the session's'close'on the next tick, while node emits it from the socket's own'close'listener. The transport is destroyed a few turns later, which is what the new test waits for. #38195 moves the session's'close'behind the socket's.The error path now also puts the final GOAWAY on the wire before the FIN, as Node's best-effort
nghttp2_session_terminate_sessiondoes.The early detach came in with the 2024 "H2 fixes" batch (#14606). No commit argues for it, and the only text that referenced it was the
destroy()latch comment, now corrected. The#destroyinglatch itself is unchanged:destroy()is re-entered from inside its own teardown (stream'error'listeners, the'goaway'emit) before the socket detaches, so it cannot key off thedestroyedgetter.Follow-up, not in this PR:
session.destroyedmeans "socket detached" in bun and "destroy() ran" in node (a state flag). A flag would make the getter total during the teardown window. It would not close this leak on its own, so it is a separate change.Related, and not covered by this change:
destroy()(adestroy()with noclose()must hard-destroy the socket, not onlyend()it). The error path here never reached that step, so the two changes compose in either merge order: with both, a transport error ends and destroys the socket and the session reports itself closed afterwards.ECONNRESET(no'error'listener on the session) still takes the graceful branch ofdestroy(), which only callsend(). That branch is node:http2: destroy the socket on session.destroy() without close(), emit the session 'close' once the socket has closed #38195's subject.The server session's
#onErrordoes not detach, and a synthetic error on an accepted socket already destroys it (checked on main), so no server-side change was needed.Test hardening after the first CI run: the macOS lanes saw the client's
'connect'before the server's accept callback, so the test now waits for both ends before it counts connections. The connect wait also rejects on an early session'error'or'close', andfinallydestroys the transport and session so a failing run releases the connection it just proved open.[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
root cause · written by the author bot
The client HTTP/2 session's transport 'error' handler set its internal socket reference to null before calling destroy(), and because destroy() only performs its GOAWAY, end, and socket-destroy step when that reference is still set, the teardown was skipped and any transport that reported an error without destroying itself stayed open, leaking the connection and its file descriptor. The fix removes that premature detach so destroy() runs with the socket still attached and ends and destroys it, matching the server session's error path and Node's behavior. A regression test confirms the sessi…