Migrate TCPSocket/TLSSocket from hasPendingActivity to jsc.JSRef

Merged
RO
7mo ago
+295-41

What

NewSocket (the template behind TCPSocket and TLSSocket) predates jsc.JSRef and still used the old retention model:

  • this_value: jsc.JSValue = .zero — a raw, unprotected cached wrapper handle that can dangle.
  • has_pending_activity: std.atomic.Value(bool) read by hasPendingActivity, which the codegen ActiveDOMObject machinery polls to keep the JSC wrapper alive.

This requires hand-flipping the atomic on every state transition (six call sites). Miss one and the wrapper is pinned forever.

jsc.JSRef collapses this to: strong while the native socket is active, weak after close, finalized after GC. No separate atomic, no raw JSValue cache. UDPSocket, StatWatcher, Terminal, and H2FrameParser already use it.

How

NewSocket (src/bun.js/api/bun/socket.zig)

  • Replace this_value: jsc.JSValue + has_pending_activity atomic with this_value: jsc.JSRef.
  • Delete hasPendingActivity().
  • getThisValue() sets the ref strong on first access (matching the old default-true atomic); returns .js_undefined if already finalized to avoid creating a second wrapper that would double-deref on GC.
  • markActive() upgrades the ref (guarded by isNotEmpty() since server-side sockets may not have a wrapper yet).
  • markInactive() downgrades the ref before reading handlers — in client mode handlers.markInactive() frees the Handlers allocation, and the old atomic store had no JSC interaction so ordering didn't matter.
  • handleConnectError() downgrades unconditionally (the duplex-TLS-upgrade pre-open error path has no connect promise).
  • upgradeTLS() hand-off / WrappedHandler.onClose raw-socket cleanup downgrade the ref (guarded against .finalized).
  • finalize() marks the ref finalized; deinit() releases it.

Listener (src/bun.js/api/bun/socket/Listener.zig)

  • Mechanical adjustments only: struct-literal defaults and the isNotEmpty() assertion on prev.this_value for the socket's new field type.
  • strong_self stays as Strong.Optional — migrating it to JSRef triggered a shutdown-only ASAN use-after-poison (Listener freed before a server socket's markInactive ran) that didn't reproduce in local debug-asan. That migration is deferred to a follow-up.

Codegen (src/bun.js/api/sockets.classes.ts)

  • Drop hasPendingActivity: true for TCPSocket / TLSSocket. Generated headers no longer reference *__hasPendingActivity for these two classes.

Tests (test/js/bun/net/socket-retention.test.ts)

  • New regression guard for the JSRef retention contract: wrapper survives GC while the socket is active; both upgradeTLS wrappers (raw + tls) collectable after close; node:net reconnect-after-error doesn't accumulate orphan wrappers.

Non-goals

  • ref()/deref() RefCount (native lifetime) — untouched.
  • poll_ref: Async.KeepAlive (event-loop liveness) — untouched.
  • Handlers.promise (Strong.Optional holding the connect promise, not the wrapper) — untouched.
  • Listener.strong_self — deferred (see above).

Verification

  • bun run zig:check-all passes on all platforms (Linux/macOS/Windows × x64/arm64).
  • bun bd test test/js/bun/net/socket.test.ts, socket-retention.test.ts, test/regression/issue/17793.test.ts pass locally.
  • bun bd test test/js/node/net/*.test.ts, test/js/node/tls/node-tls-{connect,upgrade,server}.test.ts — all identical to main.
  • No has_pending_activity remains in socket.zig.
  • Generated code: rg 'TCPSocket__hasPendingActivity|TLSSocket__hasPendingActivity' build/debug/codegen/ → empty.