node:net: let the GC collect a handle whose socket is destroyed before it connects

Merged
RO
2mo ago
+164-19

Problem

  • A node:net/node:tls socket destroyed before its connect starts keeps its TCPSocket/TLSSocket wrapper (and its net.Socket) alive forever. 1000 net.connect() + destroy() per round leave 1001, 2001, 3001 TCPSocket objects after full GC. destroy() during the DNS lookup, an early AbortSignal, and a connect() that throws leak the same way.
  • So does every failed name lookup: 100 net.connect({ host: "does-not-exist.invalid" }) per round leave 101 then 201 TCPSocket on stock bun, 1 then 1 with this change. A reconnect loop during a DNS outage grows without bound.
  • Cause: new_detached_socket (src/runtime/node/node_net_binding.rs) creates the wrapper through get_this_value, which holds it strong. Only a connect attempt's teardown downgrades that reference. A handle that never connects reaches no teardown: _handle.close() on a detached socket is a no-op.

Fix

  • The fixing line: new_detached_socket now creates the idle handle with JsRef::init_weak. net.Socket._handle owns it until a connect starts.
  • connect_finish already upgraded the reference for each attempt. That moves into NewSocket::this_value_for_connect, and the Windows named-pipe connect arms now call it too (they did not upgrade before).
  • Correct because the reference is now strong exactly while native work is pending, like other JsRef users (Bun.spawn, Valkey) that start weak.
  • Verified: new test in test/js/bun/net/socket-retention.test.ts fails on stock bun 1.4.3-canary (101 TCPSocket / 26 TLSSocket retained, protectedObjectTypeCounts 100 / 25) and passes on the debug ASAN build (1 / 1, protected 0 / 0). It asserts both the object counts and that no TCPSocket/TLSSocket is held by a strong handle. Other suites in the notes, Linux and Windows.

Related

  • Supersedes "Fix 2: never-connected sockets pinned forever" (a812bee39a) in draft #35538. Same root cause; that commit makes get_this_value take a weak reference whenever the socket is detached. This PR instead makes the weak reference explicit at the one creation site and leaves get_this_value unchanged, and this_value_for_connect also pins the two Windows named-pipe connect arms, which an is_detached() gate in get_this_value would leave weak during uv_pipe_connect. That hunk can be dropped from #35538 once this lands.
  • #34441 recorded the symptom: upstream test-gc-net-timeout.js not collecting all of its sockets.
  • Upstream test-gc-net-timeout.js (the test a812bee39a vendors) is not added here. It passes with this change on every lane of build 114199 except alpine 3.23 x64, where it timed out in 4 of 4 attempts. It passes here on glibc x64 with a release build of this branch: 5 of 5 runs, 16 of 16 pinned to 1, 2, 4 and 8 CPUs, and with localhost forced to two addresses where the first one fails. It observes collection through timing (a socket leaks only when its 10 ms timeout wins the race with the connect), like the tests #35182 deleted. The guard that fails on stock bun every time is the socket-retention.test.ts test above.

Not covered

  • destroy() from inside a 'connectionAttempt' listener (#42307). net.ts nulls _handle before kConnectTcp runs, so doConnect(null, ...) allocates a fresh native socket that nothing owns, and it stays strong-rooted and connected. Separate root cause in src/js/node/net.ts, reproduces the same on stock bun, not changed here.
  • Windows named pipes: a destroy() before the pipe open completes (traced statically only). The second named-pipe path found here, the attempt ref that a synchronous WindowsNamedPipeContext::connect/open failure never released (#42309), is fixed on main by #42317. This branch has main merged in, and both changes are in the two named-pipe arms of Listener::connect_inner.
  • No test fails if the two named-pipe arms in Listener::connect_inner go back to get_this_value. The upgrade there is what keeps a first node:net pipe connect alive across uv_pipe_connect when JS drops the socket; the Windows leak tests listed below check the opposite direction (nothing stays pinned).
  • Nothing asserts that this_value is strong where a native attempt starts (do_connect, WindowsNamedPipeContext::connect/open). A debug_assert! there would make every debug net test check it; left for a follow-up.

Background

  • NewSocket<SSL> (socket_body.rs) is the native side of a TCPSocket/TLSSocket. Its this_value: JsRef is Strong (a StrongRootBlock root) or Weak. The wrapper's finalizer derefs the native socket.
  • Socket.prototype.connect calls newDetachedSocket() and stores it in _handle before the lookup and the process.nextTick that starts the connect. doConnect(_handle) later reaches connect_finish with it as prev.
  • A detached handle has no us_socket_t, so closing it dispatches nothing native.
Notes
  • Heap snapshot (generateHeapSnapshotForDebugging) of the retained cells before the fix: each TCPSocket has exactly one incoming edge, Internal from StrongRootBlock, which is a GC root. No JS retainer. heapStats().protectedObjectTypeCounts counts the same strong handles, which is what the test asserts on.
  • Scenarios measured on stock bun 1.4.3-canary (200 sockets each, count after GC): immediate destroy 201, tls.connect + destroy 201 TLSSocket, destroy during lookup of localhost 201, destroy in the 'lookup' event 201, AbortSignal abort 201, connect() throwing ERR_SOCKET_BAD_PORT 201. Sockets that connect and then close (destroy after 'connect', server-side destroy, end(), ECONNREFUSED) do not leak: 1. The constant 1 is the class's prototype object, which heapStats() counts under the same name.
  • With the fix all of the above report 1 on the debug ASAN build, and the reporter's 1000-per-round loop stays at 1.
  • The terminal paths that downgrade a connect attempt's reference: mark_inactive after on_close, handle_connect_error, and the semi-socket branch of close()/terminate().
  • What keeps an unreferenced net.Socket alive before its connect starts: the process.nextTick closure (literal IP) or the lookup callback (hostname) captures it. Probes: 200 net.connect() calls with no JS reference kept, Bun.gc(true) forced between creation and the deferred connect and on a 1 ms interval. All 200 connect, receive data, and close (also with BUN_GARBAGE_COLLECTOR_LEVEL=2). The same probe with host: "localhost" (with and without autoSelectFamily) and with a custom async lookup: 100 of 100 each.
  • The test lives in socket-retention.test.ts because that file is the guard for the JsRef retention model and already drives it through node:net in a child process.
  • Suites run on the Linux debug ASAN build: test/js/bun/net/socket-retention.test.ts, test/js/bun/net/socket.test.ts, test/js/node/net/node-net.test.ts (the 10 failures left are environment failures that stock bun shows too: localhost resolution and no outbound DNS), handle-leak.test.ts, net-mongodb-pattern-leak.test.ts, double-connect.test.ts, socket-reconnect-live.test.ts, connect-autoselectfamily-stale-timer.test.ts, node-tls-connect.test.ts, node-tls-upgrade.test.ts, 53 test-net-* and 38 test-tls-* Node parallel tests.
  • Windows x64 debug build: the new test (fails on stock bun there too, 101), should work with named pipes (700 pipe connections, TCPSocket count check), should not leak when connect({path}) fails asynchronously while polling for a pipe, the Bun.connect named-pipe lifecycle tests in socket.test.ts, and node-tls-namedpipes.test.ts (400 TLS pipe connections, TLSSocket count at most 3) pass. The Windows runs predate the protectedObjectTypeCounts assertion and the comment trims, which change no compiled code.
  • cargo check --target x86_64-pc-windows-msvc passes for the #[cfg(windows)] arms.

no test proof · iteration 2 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/net/socket-retention.test.ts