Repository navigation
Conversation
new tls.TLSSocket(new net.Socket()).destroy() threw an uncaught 'handle.close is not a function' from closeSocketHandle. A client-side TLSSocket wrapping a Duplex that has not yet connected stores the stream itself as _handle (there is no native handle yet). Node wraps that stream in a JSStreamSocket whose close() destroys it; do the equivalent in closeSocketHandle by falling back to handle.destroy() when the handle has no close method.
|
Warning Review limit reached
Next review available in: 8 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: diff is green; CI blocked on unrelated build-queue infra. Reproduced with: bun -e 'import tls from "node:tls"; import net from "node:net"; new tls.TLSSocket(new net.Socket()).destroy(); setTimeout(() => console.log("SURVIVED"), 100);'Main throws uncaught CI: builds #81831 and #81933 both stalled because the |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found, but this touches the shared closeSocketHandle path in node:net and changes how wrapped-stream handles are torn down, so it's worth a maintainer glance.
What was reviewed:
- Verified native handles (which have
.close) hit the original branch unchanged; only the previously-throwing wrapped-Duplex case takes the newdestroy()fallback. - Checked
_destroycall sites:kAdoptedTLSRawis only set on native upgrade handles, so a wrapped Duplex always reachescloseSocketHandle. - Tests await both
'close'events (no sleeps), cover net.Socket + Duplex, and the added.destroy()calls on the two existing detached-wrap tests are now safe because of this fix.
Extended reasoning...
Overview
Five-line change to closeSocketHandle in src/js/node/net.ts: instead of unconditionally calling handle.close(cb), it checks typeof handle.close === 'function' and falls back to handle.destroy?.() (guarded by !handle.destroyed). This handles the case where tls.TLSSocket stores the wrapped Duplex directly as _handle before any connect/upgrade — previously .destroy() on such a socket threw an uncaught TypeError. Two new describe.each cases cover net.Socket and plain Duplex; two existing tests now call .destroy() on their detached wraps (which this fix makes possible).
Security risks
None. No auth/crypto/permission surface; this is teardown-path plumbing. The typeof check and optional-chained destroy?.() don't call user-overridable prototype methods any more than the previous handle.close() did.
Level of scrutiny
Medium-high. The change itself is small and the native-handle branch is byte-identical to before, but closeSocketHandle is on the destroy path for every net/tls socket, and the underlying design question — whether tls.ts should keep storing a bare Duplex as _handle vs. wrapping it in something with close() (as Node's JSStreamSocket does) — was raised by comment-cop and answered by the author, but a maintainer should confirm they're OK with the probe-and-fallback approach living in net.ts rather than a proper wrapper in tls.ts.
Other factors
Tests are well-structured (event-driven, no sleeps, both variants covered, assert both sockets' destroyed and both 'close' events). I traced _destroy: kAdoptedTLSRaw is only set on native upgrade results, so the Duplex case reliably reaches the modified helper. onSocketHandleClosed (skipped in the fallback branch) is a no-op, so nothing is lost by not passing it. The handle.onread = noop in the cleanup path is harmless on a Duplex.
There was a problem hiding this comment.
LGTM — targeted fix for an uncaught TypeError: handle.close is not a function when destroying a never-connected TLSSocket wrapping a Duplex.
What was reviewed:
closeSocketHandle: native handles still hithandle.close(cb)unchanged; the new branch only fires when_handleis the wrapped stream tls.ts assigns for JSStreamSocket compat, and mirrors Node'sJSStreamSocket.close()(destroy the wrapped stream).- Tests await the actual
'close'events on both the TLS wrapper and the raw stream fornet.SocketandDuplex; the added.destroy()calls on pre-existing detached-wrapper tests now exercise the same path. - Checked the other
closeSocketHandlecaller (_closeAfterHandlingErrormicrotask) and the follow-onhandle.onread = noop— both are harmless on a Duplex.
Extended reasoning...
Overview
The PR fixes new tls.TLSSocket(duplex).destroy() before any connect, which currently throws an uncaught TypeError: handle.close is not a function and kills the process. Bun's tls.ts intentionally stores the wrapped Duplex directly as this._handle (for http2-wrapper's JSStreamSocket-constructor compat), but Socket.prototype._destroy → closeSocketHandle assumed _handle was always a native handle with .close(). The fix makes closeSocketHandle fall back to handle.destroy() when close is not a function, guarded by !handle.destroyed to avoid double-destroy. Tests cover both net.Socket and generic Duplex wraps and assert that both the TLS wrapper and the raw stream emit 'close' and end up destroyed. Two existing tests that previously left never-connected wrappers un-destroyed now call .destroy(), which additionally exercises the fixed path.
Security risks
None. This is destroy-path cleanup for a client-side TLSSocket that was never connected. No trust decisions, credentials, or verification logic are touched; the change is strictly additive and only reachable when _handle lacks a close method (i.e., it is a JS stream, not a native handle).
Level of scrutiny
Moderate. closeSocketHandle is on the shared socket destroy path, but the change is defensive: the native-handle path (typeof handle.close === "function") is byte-identical to before, so all existing sockets are unaffected. The new branch matches Node semantics — Node's JSStreamSocket.close() also just destroys the wrapped stream — and the follow-on handle.onread = noop / self._handle = null cleanup is harmless on a Duplex. I also checked the second caller (the _closeAfterHandlingError microtask at net.ts:2225) — it flows through the same guarded branch.
Other factors
The comment-cop feedback was addressed (comment tightened to one line). The PR description shows the full test file passing on both debug/ASAN and release builds and failing on main, and lists the vendored Node TLS tests that still pass. The tests await real events (no sleeps), wire failure via once(), and cover the variant matrix (net.Socket and Duplex). One could argue a fuller JSStreamSocket wrapper in tls.ts would be the "deeper" fix, but tls.ts already deliberately uses the Duplex as _handle for ecosystem compat, and this fallback is exactly what Node's wrapper's close() does — so this is the right layer.
|
Related: #37664 removes the root cause this works around. The client-side |
|
Still reproduces on current main (732491c), and this diff also covers the connected-socket shape of the same bug (client STARTTLS teardown, which is how the const raw = net.connect(port, "127.0.0.1", () => {
const wrap = new tls.TLSSocket(raw, { rejectUnauthorized: false });
wrap.destroy();
});main: the wrap emits With the |
|
Closing: #42330 covers this. #42330 fixes The coverage from here is in #42330 too: |
2 participants
Reproduction
Node v26.3.0:
SURVIVED destroyed=true, exit 0.Bun on main (44f6469):
The error is uncaught and kills the process. Same for
new tls.TLSSocket(new Duplex()).Cause
A client-side
new tls.TLSSocket(duplex)stores the wrapped stream directly asthis._handle(tls.tsthis._handle = socket); until a connect/upgrade runs there is no native handle.Socket.prototype._destroysees_handleis truthy and routes it tocloseSocketHandle, which callshandle.close(cb)unconditionally. Anet.Socket/Duplexhas noclosemethod.Node wraps such a stream in a
JSStreamSocket, whoseclose()destroys the underlying stream, sohandle.close()always exists there.Fix
closeSocketHandlefalls back tohandle.destroy()whenhandle.closeis not a function (i.e._handleis a wrapped stream, not a native handle). This both stops the uncaught TypeError and destroys the wrapped stream like Node does: with the fix, the wrapped socket's'close'fires andraw.destroyed === true, matching Node.Verification
Both tests throw
handle.close is not a functionwithout the fix. Vendoredtest-tls-on-empty-socket,test-tls-destroy-whilst-write,test-tls-js-stream,test-tls-connect-given-socketstill pass.[stamp-90s] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file