node:tls: keep the native handle reading through the handshake when the socket is paused - #35151
node:tls: keep the native handle reading through the handshake when the socket is paused#35151robobun wants to merge 7 commits into
Conversation
…en paused
Socket.prototype.pause() called while a TLS handshake is in flight
stopped native reads (us_socket_pause sets the poll write-only), so the
TLS engine never saw the ClientHello / server Finished and the handshake
wedged forever. In Node the TLSWrap reads the underlying fd independently
of the TLSSocket's stream state, so a pre-handshake pause() only affects
delivery of decrypted output; the handshake and FIN/close_notify proceed.
Gate the native pause()/unref() on !secureConnecting so only the Duplex
layer pauses. The data handler applies native backpressure at
highWaterMark once application data starts arriving. Also make
ServerHandlers.handshake honor a handler's stream state (readableFlowing
!== null) instead of unconditionally resume()ing, so a pause() made
inside the 'connection' or 'secureConnection' handler survives.
Covers:
- s.pause() inside a TLS server's 'connection' handler
- tls.createServer({ pauseOnConnect: true })
- tls.connect({ pauseOnConnect: true })
|
Updated 3:07 AM PT - Jul 23rd, 2026
✅ @robobun, your commit bf584bdc6b31ae5923750ef9bdd2753625cbc134 passed in 🧪 To try this PR locally: bunx bun-pr 35151That installs a local version of the PR into your bun-35151 --bun |
WalkthroughChangesTLS handshake pause and resume handling now preserves stream states selected by connection handlers and avoids pausing native reads during secure connection setup. New tests cover constructor options, server-side pauses, TLS pause and flow control
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js/node/net.ts`:
- Around line 884-888: Condense the inline comments to three lines or fewer
while preserving their durable invariants: shorten the flow-state preservation
explanation in src/js/node/net.ts lines 884-888, the native-read/handshake
invariant in src/js/node/net.ts lines 2120-2126, the regression rationale in
test/js/node/tls/node-tls-server.test.ts lines 1308-1311, and the Node/Bun
flow-state rationale in test/js/node/tls/node-tls-server.test.ts lines
1417-1425; no code behavior changes are required.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d5f97f91-86bf-4257-8086-0bd1579519fc
📒 Files selected for processing (3)
src/js/node/net.tstest/js/node/test/parallel/test-tls-server-parent-constructor-options.jstest/js/node/tls/node-tls-server.test.ts
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…ndler coverage, clarify Node-divergence comments
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/node/tls/node-tls-server.test.ts`:
- Around line 1314-1316: Introduce a descriptive named constant for the
2000-iteration polling limit used by waitFor, then replace the inline literal in
its loop condition with that constant. Keep waitFor’s existing polling behavior
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4d42c19d-cd2a-42a5-a679-3312232a5dd6
📒 Files selected for processing (2)
src/js/node/net.tstest/js/node/tls/node-tls-server.test.ts
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
test/js/node/tls/node-tls-server.test.ts:1346-1349— nit: this negative-contract guard throws inside a'data'event callback, which REVIEW.md's test rules explicitly prohibit ("never throw inside event callbacks"). If a regression ever fires'data'while paused, the throw propagates throughemit()back into the native data-dispatch path and surfaces as an uncaught exception rather than a cleanly-attributed test failure. Record the violation into a captured boolean instead — e.g.let firedWhilePaused = false; srv.on('data', d => { if (stopped) firedWhilePaused = true; got += d; })— andexpect(firedWhilePaused).toBe(false)after thewaitForbarrier.Extended reasoning...
What the issue is
The "server: s.pause() inside the 'connection' handler" test guards its negative contract (no
'data'while paused) by throwing from inside the listener:srv.on("data", d => { if (stopped) throw new Error("data event fired while paused"); got += d; });
REVIEW.md's test rules are explicit on this: "Wire EVERY failure event … to reject the awaited promise — never throw inside event callbacks." This is a repository-specific check, not a subjective style preference.
Why throwing here is the wrong failure channel
If the negative contract is ever violated (e.g. a future regression makes
'data'fire whilereadableFlowing === false), the throw propagates synchronously throughEventEmitter.emit()→Readable's flow/push path → the nativeon_datadispatch wrapper. It surfaces as an uncaught exception on the event loop rather than a rejection of the promise the test is awaiting. Thetry/finallyblock in the test only catches exceptions thrown on the awaited chain, so cleanup ordering is not guaranteed relative to the uncaught-exception report, and on persistent CI runners an uncaught exception in one test can poison later tests in the same file.There's a second, subtler problem: because the throw happens before
got += d, the subsequentexpect({ got: '' , … })assertion would still pass even if data fired while paused — so the throw is effectively the only guard for this contract, and it's wired to the wrong channel.Step-by-step: what happens on regression
- Suppose a future change causes
srvto emit'data'with"hello"whilestopped === trueandreadableFlowing === false. - The listener runs
throw new Error("data event fired while paused"). emit('data', …)does not catch listener throws; the exception unwinds through Readable's internal emit path back into the native socket data callback (Handlers.data→runCallback).- Bun reports it as an uncaught exception. The test's
await waitFor(() => srv!.readableLength >= 5)is still pending on a separate microtask chain; thetry/finallyaround it does not observe the throw. gotis still""(the throw skippedgot += d), so if the test does eventually reach theexpect({ got: '' })line it passes — the only failure signal is the out-of-band uncaught exception.
Fix
Record the violation and assert it on the awaited chain:
let stopped = true; let firedWhilePaused = false; let got = ""; srv.on("data", d => { if (stopped) firedWhilePaused = true; got += d; }); cli.write("hello"); await waitFor(() => srv!.readableLength >= 5); expect(firedWhilePaused).toBe(false); expect({ got, flowing: srv.readableFlowing, readableLength: srv.readableLength }).toEqual({ got: "", flowing: false, readableLength: 5, });
(Alternatively, reject a
Promise.withResolvers()promise that the testawaits — either routes the failure through the test's own promise chain sotry/finallycleanup runs in order and the failure is attributed to this test.)Severity
This is nit — test hygiene, not a runtime correctness bug. The branch is dead on the happy path (the fix under test makes
readableFlowing === falsebuffer without emitting), and thereadableLength: 5assertion also indirectly covers the contract. It only matters if the contract is later violated, at which point you want a clean, attributed failure rather than an uncaught exception. - Suppose a future change causes
…the event callback
|
CI on 6388921: the new test file and
Ready for review. |
|
@robobun fix the conflicts |
|
Rebased onto main (which now includes #32630). The conflict was in |
|
Status of this PR against main (72ec6e2) and #39830 (2c6c0bc), from a run of this PR's test file on both:
So #39830 covers the handshake stall. The read-ahead assertion is the one part of this PR that neither main nor #39830 has, and the two PRs pin it in opposite directions. Leaving this open for that decision. The branch also conflicts with main now. |
2 participants
What
Calling
socket.pause()on a TLS socket before the TLS handshake completes stalls the handshake forever. In practice this hits three paths:s.pause()inside atls.Server'connection'handlertls.createServer({ pauseOnConnect: true })tls.connect({ pauseOnConnect: true })All three complete in Node.
Repro
Cause
Socket.prototype.pausecallsthis._handle.pause()→ nativeus_socket_pause, which sets the poll to write-only. For a TLS socket mid-handshake the TLS engine then never sees the ClientHello / server Finished.In Node the
'connection'event delivers the rawnet.Socketand the separateTLSSocket'sTLSWrapkeeps reading the underlying fd regardless of either socket's stream state, so a pre-handshakepause()only affects delivery of decrypted output; the handshake and FIN/close_notify proceed. In Bun the sameTLSSocketobject is delivered to'connection', so its native pause stops the whole read path.Fix
Gate the native
pause()/unref()inSocket.prototype.pauseon!this.secureConnecting. While the handshake is in flight only the Duplex layer pauses (readableFlowing = false); the native handle keeps reading so the TLS engine can complete the handshake and observe close_notify. The data handler'sif (!self.push(buffer)) socket.pause()applies native backpressure athighWaterMarkonce application data starts arriving.ServerHandlers.handshakeis changed to honor the handler's stream state: only apply the server'spauseOnConnect/resume default whenreadableFlowing === null. Apause()(or'data'/'readable'attach) made inside the'connection'or'secureConnection'handler is no longer stomped by the post-emitresume().Plain TCP (
net.createServer,net.connect) is unchanged:secureConnectingis not set on plain sockets.What matches Node and what does not
The Node-matching observable is that the handshake completes on all three paths (verified against Node v26.3.0). Server
pauseOnConnect: truealso matches Node's post-handshake readable state (isPaused() === true,readableFlowing === false; Node's owntest-tls-server-parent-constructor-optionsasserts this).The post-handshake readable state for the other paths reflects a pre-existing architectural divergence this PR does not change: Bun hands the same
TLSSocketto both'connection'and'secureConnection', andtls.connectreturns the same object thatpauseOnConnectacts on, while Node uses separate objects. So apause()made on the'connection'socket, or clientpauseOnConnect, is visible on Bun'ssecureConnection/returned socket (isPaused() === true) but not on Node's. The custom tests that assert Bun's state say so inline.Verification
All five hang/time out (or assert the stomped state) on main and pass with this change.
Related
#35148 fixes only the server
pauseOnConnectoption path at theonconnectioncall site; this PR fixes it atSocket.prototype.pauseso every pre-handshakepause()call site is covered, and subsumes #35148. #35108 covers the plain-TCP sibling of theServerHandlers.handshakegate.[review] gate passed · iteration 4 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 1 rejected · iteration 4
evidence per changed file