node:net: reset the stream state when connect() reuses a half-closed socket - #38980
node:net: reset the stream state when connect() reuses a half-closed socket#38980robobun wants to merge 4 commits into
Conversation
…socket Socket.prototype.connect only ran initSocketHandle (which _undestroy()s the Duplex) when it created a new handle, i.e. on a fresh socket or one that had already been destroyed. A socket whose previous connection was still being torn down (end() called, or the peer's FIN received on an allowHalfOpen socket) kept its handle, so the native side replaced the connection but the stream kept the ended/finished flags of the previous one: 'connect' fired and the first write failed with ERR_STREAM_WRITE_AFTER_END, and a readable side that had ended never emitted 'end' again. Run initSocketHandle on every connect(), and also drop the cached _peername there so remoteAddress/remotePort describe the new peer. The "should allow reconnecting after end()" test reconnected 3ms after end()'s callback and only passed when the previous connection had finished closing by then (it flaked on slow debug builds); it now reconnects from the callback itself, which reuses the half-closed handle deterministically.
|
Warning Review limit reached
Next review available in: 13 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 (2)
Comment |
|
Status: reproduced and fixed; ready for review (HEAD 5314d9e). Reproduced on Linux with the released bun: reconnecting a The follow-up commits only apply the review nits (dead |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the two inline findings are non-blocking nits (redundant _peername/_sockname clears that the unconditional initSocketHandle now covers, and the listen() helper not wiring server.once('error', reject)); neither affects correctness.
Extended reasoning...
Overview
The runtime change is two lines in src/js/node/net.ts: initSocketHandle(this) is moved out of the if (!this._handle) block so it runs on every connect(), and initSocketHandle now also clears _peername. The rest of the diff is test coverage in test/js/node/net/node-net.test.ts: the flaky "should allow reconnecting after end()" test is rewritten to reconnect deterministically from end()'s callback (before the peer's FIN is read), and four new tests under "connect() while the previous connection is half-closed" cover the writable side (TCP + unix socket), the readable side on an allowHalfOpen client, and remotePort reporting the new peer.
Security risks
None. This is stream-state bookkeeping in the node:net compat layer — no auth, crypto, path handling, or untrusted-input parsing is touched. _undestroy() and the _peername/_sockname/kclosed/kended clears are all reset-to-default operations on the socket's own fields.
Level of scrutiny
Medium. Socket.prototype.connect is a hot path, but the change is narrowly scoped: on paths where initSocketHandle already ran (fresh socket, destroyed socket, kReinitializeHandle autoSelectFamily retry) it is idempotent — _undestroy() on an already-fresh Duplex is a no-op, and the field assignments overwrite with the same values. Only the reuse-a-half-closed-handle case changes, which is exactly the case the tests exercise. I traced lines 2123-2145 to confirm nothing between the destroyed block and the new initSocketHandle call reads the stale stream state, and checked that the added _peername = null in initSocketHandle is also correct for its other caller (kReinitializeHandle, where the peer changes on retry).
Other factors
- The PR description documents that the full
node-net.test.ts,socket-reconnect-live.test.ts,double-connect.test.ts, the autoSelectFamily test,node-net-allowHalfOpen.test.js,handle-leak.test.ts, and thetest-net-*.js/test-tls-*.jsNode parallel suites were run with the change. - The new tests await observable conditions (
once(socket, 'connect'),once(socket, 'finish'),once(socket, 'end')), collect errors into an array asserted at the end, and clean up intry/finally— they follow the file's existing patterns and REVIEW.md's guidance. - The two inline findings are pure hygiene: the redundant
_peername/_socknameclears in theif (this.destroyed)block are now dead but harmless, and thelisten()helper's missing error-reject matches how the pre-change test (and several neighboring tests in this file) already listen. Neither affects correctness or should hold the fix. - No CODEOWNERS entry covers these files, and there are no outstanding human review comments.
… listen errors in the reconnect tests
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Unlike node (where connecting a handle that is still connected fails with | ||
| // EISCONN), a handle whose previous connection is still open or half-closed is | ||
| // reused: doConnect closes that connection and connects the same handle again. | ||
| // The stream state has to be re-initialized for it as well, otherwise a | ||
| // connect() issued after end() but before the previous connection finished | ||
| // closing keeps its ended/finished state and the first write on the new | ||
| // connection fails with ERR_STREAM_WRITE_AFTER_END. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Called before every connect(): on a new Socket, and whenever a Socket is | ||
| // re-used for another connection (after a close, or while the previous | ||
| // connection is still being torn down). |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Also for a reused handle: doConnect replaces the connection it still | ||
| // carries, so the stream state of that connection has to go with it. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // A reused handle gets a new connection from doConnect, so it is reset as well. | ||
| initSocketHandle(this); |
There was a problem hiding this comment.
🟡 The hostname variant of this fix has a race window: initSocketHandle(this) resets state synchronously here, but for a non-IP host lookupAndConnect defers internalConnect → detach_for_reconnect to an async DNS callback, so the old still-attached connection can deliver end/close (SocketHandlers2 has no self.connecting guard) between the reset and the detach and re-set kended/self.write=writeAfterFIN/push(null). Not a regression — before this PR the hostname case failed 100% of the time — but every new test uses "127.0.0.1" (the process.nextTick fast path), so the gap is untested. Guarding finishSocketEnd/close on self.connecting, or repeating the reset in internalConnect right before doConnect, would close the window; fine as a follow-up.
Extended reasoning...
What the gap is
Socket.prototype.connect now runs initSocketHandle(this) unconditionally at net.ts:2137, which _undestroy()s the Duplex and clears kended/kclosed/_peername. That happens synchronously. The old native connection, however, is not detached until internalConnect → doConnect → detach_for_reconnect() runs. For an IP-literal host that is scheduled via process.nextTick (net.ts:2896) — no I/O poll happens between a nextTick queue flush and the code that scheduled it, so the window is closed. For a unix path it is synchronous. But for a hostname host, lookupAndConnect defers internalConnect to an asynchronous dns.lookup callback (net.ts:2938-2953), and the event loop does poll I/O in between.
The code path that re-corrupts the reset state
During that DNS window the old (still-attached) connection can deliver its FIN or close to SocketHandlers2:
SocketHandlers2.end(net.ts:1304-1309) callsfinishSocketEnd(self)with noself.connectingguard.finishSocketEnd(net.ts:589-606) checks onlyself[kended]— whichinitSocketHandlejust cleared tofalse— so it runs:self[kended] = true,self.write = writeAfterFIN(defaultallowHalfOpen: false),self.push(null)(readable side re-ended), andsocket.unref().SocketHandlers2.close(net.ts:1326-1368) similarly has noself.connectingguard, setsself[kclosed] = true, and callsfinishSocketEnd. A knock-on: withkclosedalready true, the new connection's later close hitsif (self[kclosed]) return;at line 1330 and is silently dropped.
The this.write restore at net.ts:2119-2122 already ran before this, so it does not undo the re-installed writeAfterFIN. internalConnect and afterConnect do not repeat the reset.
Why the existing tests don't cover it
Every new test in this PR — the rewritten "should allow reconnecting after end()" and all four "connect() while the previous connection is half-closed" cases — connects to "127.0.0.1" (or a unix path). isIP("127.0.0.1") is truthy, so lookupAndConnect takes the process.nextTick fast path at line 2896, and no I/O poll can interleave between initSocketHandle and detach_for_reconnect. A test that reconnects to "localhost" (or via a custom options.lookup that resolves asynchronously) while the previous peer's FIN is still in flight would exercise the window.
Step-by-step trace
socket.end(); server keeps its side open (allowHalfOpen: true), so the client is half-closed andsocket.destroyed === false.socket.connect(port, "localhost")runs.this.write === writeAfterFINis restored (line 2119).this.connecting = true.initSocketHandle(this)runs_undestroy(), setskended = false,kclosed = false.lookupAndConnectseesisIP("localhost") === 0and callsdns.lookup(...).- Control returns to the event loop; the loop polls I/O. The old connection's peer now closes its side (or the FIN that was already on the wire is read).
SocketHandlers2.endfires →finishSocketEnd(self):kended = true,self.write = writeAfterFIN,self.push(null). - The DNS callback fires →
internalConnect→doConnect→detach_for_reconnect(old connection finally detached) → new connection established →afterConnectemits'connect'. socket.write("data")is nowwriteAfterFIN.push(null)on a!allowHalfOpenDuplex triggered auto-end(), sowritableEndedis true again and the write fails — same user-visible symptom this PR set out to fix, just via a narrower race and a different code (EPIPEinstead ofERR_STREAM_WRITE_AFTER_END).
Why this is a nit, not a blocker
- Not a regression. Before this PR
initSocketHandlewas never called on the reused-handle path, so the hostname case failed 100% of the time withERR_STREAM_WRITE_AFTER_END. After this PR it fails only when the old FIN/close races into the DNS window. Strict improvement on every path. - Bun-specific extension. Node rejects this whole pattern with
EISCONN(per the PR description); reconnecting a half-closednet.Socketto a hostname while the old peer's FIN is in flight is a narrow edge of a Bun-only behavior. - The proper fix is a bit larger than this PR's scope: either guard
finishSocketEnd/ theclosehandler onself.connectingfor the reused-handle case (so late old-connection events are ignored), or detach the old connection synchronously inconnect(), or repeatinitSocketHandleinsideinternalConnectjust beforedoConnect(afterdetach_for_reconnecthas nulled the old ext slot so no further JS callbacks can arrive from it). Any of these is a reasonable follow-up.
Per REVIEW.md's "Cover the variant matrix, not just the repro" this is worth noting — the hostname variant is an untested gap — but not worth blocking a PR that fixes the flaky test it set out to fix and strictly improves every case.
|
Updated 10:53 AM PT - Aug 15th, 2026
❌ @robobun, your commit 5314d9e has 1 failures in 🧪 To try this PR locally: bunx bun-pr 38980That installs a local version of the PR into your bun-38980 --bun |
|
I was handed the same flake (the Windows debug failure of "should allow reconnecting after end()") and landed on the same cause, so I am not opening a second PR. Two things I found while checking the reuse path, in case they are useful here. Both were measured by emulating this PR's reset from userland ( 1. The cached address getters on the reused handle are stale. (server created with 2. The lookup window from the review comment above is deterministic when the lookup answers after the previous connection's FIN, which is the normal ordering on a release build (loopback FIN well under 1ms, DNS answer later; with Both go away if Happy to leave all of this with this PR; #39008 routes TLS sockets into the same path, so whatever is decided here covers both. |
1 participant
Problem
net.Socket#connect()on a socket whose previous connection is still being torn down (afterend(), or after the peer's FIN on anallowHalfOpensocket) emitsconnectfor the new connection, but the first write on it fails witherror: write after end/code: "ERR_STREAM_WRITE_AFTER_END"; a readable side that had ended never emitsendagain, andremoteAddress/remotePortkeep describing the old peer.test/js/node/net/node-net.test.ts"net.Socket write > should allow reconnecting after end()" hits exactly this: it reconnected 3ms afterend()'s callback, which only works when the previous connection has finished closing by then. On a Windows debug build it failed about half the time with the error above.Socket.prototype.connect(src/js/node/net.ts) only calledinitSocketHandle()(which_undestroy()s the Duplex) when it created a new handle, i.e. for a fresh socket or one that had already been destroyed. A socket that is not destroyed yet keeps its handle; since node:net: handle socket.connect() on a socket that still has a live native handle #32739 the native connect tears the previous connection down and connects that handle again (connect_finish->detach_for_reconnectinsrc/runtime/socket/), but nothing reset the stream state that went with the previous connection.Fix
connect()runsinitSocketHandle()whether or not it had to create the handle, andinitSocketHandle()also clears the cached_peername(it already cleared_sockname). The_peername/_socknameclears inconnect()'sif (this.destroyed)block are deleted sinceinitSocketHandle()now covers them; only_handle = nullstays.initSocketHandleis node's own reset for that situation (_undestroy+ clearing the cached names). It is a no-op on the paths that already ran it (fresh socket, destroyed socket, autoSelectFamily retry), so only the reuse case changes.connect()on a socket that wasend()ed but not destroyed fails withconnect EISCONN(the kernel rejectsconnect(2)on the still-connected fd), so the pattern only works there once the socket has been destroyed. Bun chose in node:net: handle socket.connect() on a socket that still has a live native handle #32739 to replace the connection instead; this makes that work at the stream layer too, so reconnecting afterend()no longer depends on whether the peer's FIN happened to be processed first.test/js/node/net/node-net.test.tsand all failing before / passing after the change withbun bd test:end()'s callback (deterministically before the peer's FIN is read), checks that everywrite()/end()succeeds and that noerroris emitted. Before the change it fails on the second iteration withwrite after end.allowHalfOpensocket (endemitted again),remotePortreports the new peer.node-net.test.ts(its remaining failures in this environment, e.g.www.example.comlookups andlocalhostbinding to::1, are identical without the change),socket-reconnect-live.test.ts,double-connect.test.ts,connect-autoselectfamily-stale-timer.test.ts,node-net-allowHalfOpen.test.js,handle-leak.test.ts, and the Nodetest-net-*.js/test-tls-*.jssuites intest/js/node/test/parallel(includingtest-net-reconnect.js, the reconnect-after-closepath, and the remote/local address tests).Background
net.Socketis aDuplexstream over a native handle (socket._handle). Ending the writable side (end()) and the peer ending the readable side (endevent) are recorded as flags on the stream's writable/readable state;write()on a stream whose writable state is ended throwsERR_STREAM_WRITE_AFTER_ENDregardless of what the handle underneath is doing._undestroy()is the streams-internal routine that clears those flags (ended/ending/finished, endEmitted, destroyed, errored, ...) so a stream object can be used for a new session. Node'sinitSocketHandle()calls it and clears the socket's cached address, and node calls it wheneverconnect()attaches a handle to the socket; this change calls it on the reused handle as well.allowHalfOpen: false, anet.Socketdestroys itself once both directions are done, so betweenend()and the arrival of the peer's FIN (or, withallowHalfOpen: true, indefinitely) the socket is half-closed andsocket.destroyedisfalse. That is the window in whichconnect()took the reuse path.tls.connect()socket in this state throws aTypeErrorbefore reaching this code and is also a separate issue.