Repository navigation
Conversation
A connect() on a net.Socket whose native handle still owns a socket closed that socket with a reset, with no event, and dialed a new connection on the same handle. Node refuses most of these calls. Listener::connect_inner now asks the handle first. For the calls that node's handle refuses, NewSocket::connect_refusal answers with the same errno from the state of the socket: EALREADY while a TCP connect is in flight, EINVAL for a path on a TCP socket, EISCONN for an accepted socket, for a socket that sent its FIN, and for a unix socket, and EBUSY for an fd. node:net destroys the socket with that error. Nothing is closed without an event and nothing is dialed. A connected TCP client socket that has not sent its FIN keeps what it did before: node emits 'connect' again for it on Linux. In net.ts a bind on a handle that owns a socket is EINVAL, and the walk over the addresses of a name stops when the handle still owns a socket.
|
Updated 11:54 AM PT - Oct 15th, 2026
✅ @robobun, your commit a6a786991cfceddc2b96c6d55e73dd190565bdc9 passed in 🧪 To try this PR locally: bunx bun-pr 45346That installs a local version of the PR into your bun-45346 --bun |
|
Status: draft. The code and the tests are final. The cost numbers are still to come (release builds of main and of this branch). How I reproduced it, on Linux x64: import net from "node:net";
const server = net.createServer(c => c.on("error", () => {})).listen(0, "127.0.0.1", () => {
const s = new net.Socket();
s.on("connect", () => { console.log("connect"); s.destroy(); server.close(); });
s.on("error", e => { console.log(e.code); server.close(); });
s.connect(server.address().port, "127.0.0.1");
s.connect(server.address().port, "127.0.0.1");
});
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // 'e' across the afterConnect(Multiple) / rejected-promise / attempt-timeout / | ||
| // refused-by-the-handle paths. |
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
There was a problem hiding this comment.
The comment is back to its text on main in a6a7869.
| // https://github.com/nodejs/node/blob/v26.3.0/lib/net.js#L1161-L1170 | ||
| // Node names the local address of every handle whose connect fails in the call. Here a handle whose own | ||
| // dial failed has no socket left: only a handle that refused the connect has an address to name. |
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
| // The handle refused the last attempt, or was never asked (a bind, a blocked address), and still owns | ||
| // its socket. Node closes that socket here without an event and dials the next address on a new handle. |
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 `node:net` handle that refuses the connect dials nothing: the caller gets the errno by | ||
| // value. Past the last option getter: script cannot give the handle a socket after this. |
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
| /// The errno of a `connect()` that node's handle refuses when it still owns a | ||
| /// socket. `None`: the caller dials. Nothing is closed or dialed for an answer. |
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
| // TCPWrap::Connect parses its address as IPv4 before libuv sees the handle: | ||
| // https://github.com/nodejs/node/blob/v26.3.0/src/tcp_wrap.cc#L433-L465 |
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
| // One connect per handle: https://github.com/nodejs/node/blob/v26.3.0/deps/uv/src/unix/tcp.c#L288-L289 | ||
| // libuv has no such check on Windows, where ConnectEx answers EISCONN. |
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 client socket that has not sent its FIN: connect(2) answers 0 for it once on | ||
| // Linux, and node emits 'connect' again. `connect_finish` swaps the connection. |
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
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 12 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if (ownsSocket(self._handle)) { | ||
| self.destroy(context.errors[context.errors.length - 1]); | ||
| return; |
There was a problem hiding this comment.
🔴 Callers whose host resolves to two addresses now get an EALREADY/EISCONN error and a destroyed socket where both Node and the base branch deliver 'connect'. At src/js/node/net.ts:3475-3477 the multi-address walk destroys the socket with the last error as soon as the handle refuses one attempt, instead of closing that socket silently and dialing the next address as Node's kReinitializeHandle path does. Fix: on a refusal with addresses still left, close the handle's socket without emitting an event and continue the walk, destroying only when the address list is exhausted. Trigger: net.connect(port) with the default host (localhost -> ::1 and 127.0.0.1) called twice while the first dial is in flight (#21524's program).
Why this was flagged
A net.Socket whose handle still owns a socket calls connect() with a host that the lookup resolves to more than one address, so lookupAndConnect takes internalConnectMultiple (src/js/node/net.ts:3448). The first attempt is refused by NewSocket::connect_refusal (src/runtime/socket/socket_body.rs:1393) with EALREADY or EISCONN, kConnectTcp returns the errno, the error is pushed and internalConnectMultiple recurses (src/js/node/net.ts:3551). With current > 0 the new check at src/js/node/net.ts:3475 sees ownsSocket(self._handle) true and calls self.destroy(context.errors[context.errors.length - 1]) at src/js/node/net.ts:3476, so no further address is dialed. On the base branch the same recursion reached self[kReinitializeHandle] and the next address was dialed; the socket emitted 'connect'. Node v26 also closes the refused handle's socket in kReinitializeHandle and dials the next address, emitting 'connect'. Users with retry code that calls connect() twice on localhost get an uncaught 'error' and a closed socket after merging.
Verification: On the recursion current > 0, and the new check at src/js/node/net.ts:3475-3477 destroys the socket with the EISCONN/EALREADY error instead of the kReinitializeHandle(newDetachedSocket(...)) on line 3479 that would dial the next address. On the base branch this same call ran detach_for_reconnect and dialed the first address, delivering 'connect'.
|
|
||
| /// Discard a still-live native socket so this wrapper can be reused for a | ||
| /// fresh connect. `node:net` permits `socket.connect()` on an | ||
| /// already-connected socket; without this the previous `us_socket_t`'s |
There was a problem hiding this comment.
🟡 (optional) Programs that call end() and then connect() on the same net.Socket before 'close' now get an EISCONN error and a destroyed socket; on the base branch the socket redialed. connect_refusal at src/runtime/socket/socket_body.rs:1441-1444 answers EISCONN whenever is_shutdown is set. For end(); connect(ip) in one tick, _final queues endNT (src/js/node/net.ts:2588) ahead of the connect's nextTick (net.ts:3175), so shutdown(2) always lands first. Node differs there: libuv defers shutdown(2) to the poll phase, so connect(2) runs first and emits 'connect' again. Fix: gate the EISCONN answer on the FIN having actually been sent (or defer the shutdown like libuv), and keep the original 'should allow reconnecting after end()' test instead of rewriting it to wait for 'close'.
Why this was flagged
Trigger: a client socket calls socket.end() then socket.connect(port, '127.0.0.1') before 'close' (same tick or a few ms later). end() calls _final synchronously, which queues endNT on nextTick (src/js/node/net.ts:2588); endNT calls handle.shutdown() (net.ts:439) which runs shutdown(2) immediately. connect() queues internalConnect on a later nextTick (net.ts:3175), so by the time connect_inner runs the socket is established, client, and is_shutdown, and src/runtime/socket/socket_body.rs:1441-1444 returns EISCONN. internalConnect then destroys the socket with 'connect EISCONN' (net.ts:3430-3431); a program with no 'error' listener exits with an uncaught exception. On the base branch connect_finish called detach_for_reconnect and dialed again, so these programs worked. Node's uv_shutdown only performs shutdown(2) in uv__drain during the next poll phase, after internalConnect's connect(2) has already returned 0 and 'connect' was emitted; Node only answers EISCONN once the FIN was really sent.
Verification: _final queues endNT on nextTick (src/js/node/net.ts:2588). endNT calls handle.shutdown() (net.ts:439). The connect's own nextTick (net.ts:3175) was queued after it, so connect_refusal returns Some(E::EISCONN) at src/runtime/socket/socket_body.rs:1436-1441. On the base branch this path went through connect_finish → detach_for_reconnect and emitted 'connect'.
| // Linux, and node emits 'connect' again. `connect_finish` swaps the connection. | ||
| if !self.is_server() && !socket.is_shutdown() { | ||
| return None; |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: Pre-existing gap the fix leaves open: a cluster worker's accepted socket, or one received over IPC, still has its connection silently reset and redialed by connect(), where node answers EISCONN. The accepted-socket rule at src/runtime/socket/socket_body.rs:1435 keys on is_server(), which is true only for Listener-accepted handles. A worker adopts its accepted connection through socket.connect({ fd, fdIsRawSocket: true }) at src/js/node/net.ts:4469, so its handlers are SocketMode::Client and connect_refusal returns None. Fix: refuse with EISCONN for every handle whose socket was adopted from an fd (cluster workers, Ipc.ts parseHandle 'net.Socket'), not only Listener-mode ones, e.g. record the Fd adoption in a flag and treat it like is_server() here.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
A cluster worker in round-robin mode gets each connection from the primary as an fd; onClusterConnection at src/js/node/net.ts:4459-4469 wraps it with socket.connect({ fd: clientHandle.fd, fdIsRawSocket: true }) and sets socket.isServer = true only on the JS object. Listener::connect_inner builds those handlers with SocketMode::Client (src/runtime/socket/Listener.rs:1149), so NewSocket::is_server() is false for that handle. When the worker's code later calls connect(port, host) on such an accepted socket, connect_refusal reaches src/runtime/socket/socket_body.rs:1435: the socket is established, not shutdown, and not is_server(), so it returns None and connect_finish runs detach_for_reconnect (src/runtime/socket/Listener.rs:1630), which closes the accepted connection with CloseCode::Failure and dials a new one with no event. The same applies to a net.Socket received over IPC (src/js/builtins/Ipc.ts:69-70). Node's handle for such a socket is a TCP wrap over a kernel socket in SS_CONNECTED state, and connect(2) answers EISCONN. The base branch behaves the same way, so the consequence is unchanged there.
Verification: The base already resets and redials; the new gate does not reach the fd-adopted accepted socket. onClusterConnection adopts it via socket.connect({ fd, fdIsRawSocket: true }) (src/js/node/net.ts:4469) with Client-mode handlers (Listener.rs:1149); a later connect(port, host) reaches connect_refusal (socket_body.rs:1435-1437), which returns None, and detach_for_reconnect (Listener.rs:1630) resets it, where connect(2) answers EISCONN.
There was a problem hiding this comment.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Still open from earlier reviews (3):
- 🔴
src/js/node/net.ts:3477—Callers whose host resolves to two addresses now get an EALREADY/EISCONN error and a destroyed socket where both Node a… - Also unresolved: 2 minor or pre-existing.
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
| expect(stderr).toBe(""); |
There was a problem hiding this comment.
🟡 nit (optional): when the node fixture process fails, maintainers see only a bare stderr or exit-code mismatch and lose the other outputs needed to diagnose it. inProcessOf at test/js/node/net/socket-reconnect-live.test.ts:178-179 asserts stderr and exitCode in two separate expects, so the first failing one hides the rest. Fix: assert one combined object, e.g. expect({ stderr, exitCode }).toEqual({ stderr: "", exitCode: 0 }) before parsing stdout, as the subprocess convention asks, and keep stdout for the JSON parse.
Why this was flagged
The node-side rows of describe.each spawn the fixture once through inProcessOf (test/js/node/net/socket-reconnect-live.test.ts:170-182) and cache the result in fromNode. At lines 178-179 the helper runs expect(stderr).toBe("") and then expect(exitCode).toBe(0) as separate assertions. When node prints a warning or crashes, the first expect throws with only stderr, and the exit code and stdout are never shown in the failure, so the twelve node tests all fail with one truncated message. The repository review rules ask subprocess tests to assert a combined { stdout, stderr, exitCode } object so a failure reports every output at once. The base branch has no node run of this fixture, so this is new test code only.
Verification: nit. The node fixture process writes to stderr or exits non-zero. inProcessOf at test/js/node/net/socket-reconnect-live.test.ts:179 and 181 asserts expect(stderr).toBe("") and expect(exitCode).toBe(0) as separate expects. The first failing expect throws, so a stderr mismatch hides exitCode and stdout. Sibling tests use the combined shape the REVIEW.md subprocess rule asks for.
Problem
connect()on anet.Socketwhose handle still owns a socket resets that connection with no event and dials again. Node v26.3.0 refuses most such calls:EALREADYin flight,EISCONNfor an accepted socket and afterend(),EINVALfor a path on a TCP socket.connect_finishcallsdetach_for_reconnectfor every handle (src/runtime/socket/Listener.rs:1613, node:net: handle socket.connect() on a socket that still has a live native handle #32739). Found by a fuzz run, not by a user.Fix
Listener::connect_innerasks the handle first (NewSocket::connect_refusal). Where node's handle refuses, ours answers the same errno from the state of its socket.node:netdestroys the socket with it and dials nothing.'connect'again there on Linux. That case needs a decision (Notes).test/js/node/net/socket-reconnect-live.test.ts. Node prints the same for 12 scenarios.Background
connect(2)on the handle's fd. The kernel answersEISCONNfor an accepted socket and after a shutdown.Downsides
connect()twice in a row now getsEALREADY, as in node. On 1.4.x it got a second dial (bun crashes when JackdClient becomes out of scope #21524's program is one).Notes
Which calls are refused
connect()on a handle that still owns a socket. "Node" is v26.3.0 on Linux x64. "Before" is main.EALREADYEALREADY(EISCONNon Windows)EINVALEINVALEISCONNEISCONNend())EISCONNEISCONNlocalAddressorlocalPortbind EINVALbind EINVALconnect(2)says for the new path (EISCONN,ENOENT), one loop turn laterEISCONNin the call{ fd }(Bun only)open EBUSY'connect'again on the same connectionThe case this PR leaves, and the question for a maintainer
A connected TCP client socket that has not sent its FIN (also after the peer's FIN, and a
TLSSocket). On Linuxconnect(2)answers 0 for it once, so node emits'connect'again and keeps the connection. Other kernels answerEISCONN. Bun resets the connection and dials a new one (detach_for_reconnect). The fuzz run compares against node, so it still reports this case.@Jarred-Sumner, which of these should the follow-up be?
CloseCode::FailuretoCloseCode::FastShutdownindetach_for_reconnect(src/runtime/socket/socket_body.rs:1414on main).EISCONNfrom the state, as this PR does for the other handles. It deletesdetach_for_reconnect(about 45 lines). It is node's answer on macOS and Windows. On Linux it destroys a socket that node keeps: a client that callsconnect()from its'end'listener with no'error'listener exits with an uncaughtEISCONN(measured on an earlier draft of this PR).connect(2)on the fd of the handle, and emit'connect'again when it answers 0. It is node's answer on every kernel. An earlier draft did it with a new uSockets poll type (about 440 lines). A sketch with no poll type is about 90 lines, and it is not built.The second part of the question is the walk over the addresses of a name (next section): stop, as here, or go on as node does.
Where this PR differs from node
connect(port)with the default host is one). After a refusal node goes on to the next address:kReinitializeHandlecloses the socket of the handle with no event, and the next address is dialed on a new handle. This PR destroys the socket with the last error of the walk and dials nothing. The check is at the one call ofkReinitializeHandleininternalConnectMultiple.connect(2)for the new path with the request, one loop turn later, and emits'connectionAttemptFailed'. This PR answersEISCONNin the call, also for a path that is not there (node:ENOENT).EISCONN: libuv has no check of its own there and reports whatConnectExsays (node:net: fail a TCP connect with EINVAL or EALREADY where libuv does #41530 has the same answer). A named pipe answersEISCONNin the call, where libuv reportsEBUSYwith the request. I ran nothing on Windows or macOS.connect(port); connect({ path })in one tick. Node's handle is a TCP handle from the first call, so the path isEINVAL. Here the handle has no kind before its first dial: the path is dialed, and the first call then getsEISCONN.connect()waits for its lookup fails withERR_SOCKET_CLOSEDbefore'error'. In node it fails withERR_SOCKET_CLOSED_BEFORE_CONNECTIONafter'error'.- Local (...)part: its handle has no socket left.Tests
socket-reconnect-live.test.tskeeps the three tests of main. The third one ("while the first connect is still in flight") acceptedconnectorEALREADY. It now expects the refusal, and it checks that the trace span of the refused connect ends. The three now run one at a time: a debug build needs seconds to start each of these processes.socket-reconnect-live-fixture.mjs, a file that node runs too. 12 scenarios print the same in node v26.3.0 on Linux, and the test runs them in node where node is installed (not on Windows).'connect'again.node-net.test.ts: "should allow reconnecting after end()" connected again 3 ms afterend(). It now waits for'close': before that the handle owns the socket, and the call isEISCONN(in node too).Suites run (debug build, Linux x64)
test/js/node/net/(12 files: 329 pass, and the 2 that fail need the public internet). Vendoredtest-net-*,test-pipe-*,test-socket-*,test-tls-*(354 files: one fails,test-tls-client-allow-partial-trust-chain.js, which needsbun test).test/js/bun/net/(11 files),test/js/node/tls/(28 files), vendoredtest-http-*,test-https-*,test-http2-*(719 files). What failed there also fails on a debug build of main: tests that need the public internet,test-http-abort-queued,test-http-agent-free-socket-data-guard,test-http-agent-keepalive,test-http-client-timeout-option,test-http-outgoing-end-cork,test-https-timeout, andtest-http-client-close-with-default-agent(a 1 s timer, 2 of 5 runs).bun run rust:check-all: 12 targets pass.Cost (release builds, main 5f5799e and this PR)
net.connect()dials, both builds: 1000socket, 1000connect, 3002epoll_ctl, 2000setsockopt, 1000close. Per 1000Bun.connect()dials, both builds: the same with 3000setsockopt. Counted with anLD_PRELOADinterposer.BUN_JSC_dumpGeneratedBytecodes=1):kConnectDispatch26 to 33 instructions, and a fresh dial executes 2 more (is_number,jfalse).Socket.prototype.connect470 to 484 andinternalConnect151 to 164: a fresh dial executes none of the new ones (they are in thefd, bind and error arms).objdumpofbun-profile): a freshnode:netdial executes 29 more instructions and 3 conditional branches inconnect_inner(30 and 4 for aTLSSocket), 21 of them inconnect_refusal(288 bytes).Bun.connectexecutes 4 more instructions, 2 branches.connect_innergrows from 2,806 to 2,993 bytes.readyStatereads: 0 in 200 freshnet.connect()and 200 freshtls.connect()dials on both builds (gdb breakpoint count). A dial by a name whose first address fails reads it once per later attempt (50 in 50 dials, 0 on main).size: text 106,006,149 to 106,007,404 bytes (+1,255). The file is 106,178,312 bytes on both.perf,strace,valgrindandbloatyare not on the machine. The instruction counts are from the listings, not from a counter.Self-review
37 concerns were raised against an earlier draft that refused every handle.
'connect').Related open PRs
readyState === 2). This PR answers it from the native state for every call, so that check is not needed. The two conflict.connect()is called afterend()on a half-closed socket. That call isEISCONNhere, as in node.unsafewith typed ownership #40139 movesdetach_for_reconnectand calls it from the two named-pipe arms too. Its test asserts that the first connection closes.Found on the way, not in this PR
socket.connecting. Aconnect(hostname)on a live socket then ends with no event when the socket drains before the lookup answers. Handed off.connect({ socket })on aTLSSocketwith a live session replaces its handle with no event: a second'secureConnect', and two connections. Node throwsERR_MISSING_ARGS. Unchanged here.socket.connect({})throwsERR_MISSING_ARGSand leaves a live socket inconnecting. Handed off.no test proof · iteration 4 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/net/socket-reconnect-live.test.ts, test/js/node/net/node-net.test.ts