tls: stop the event-loop spin when a handshake stalls on WANT_READ - #37088
Conversation
…shake ssl_update_handshake set last_write_failed for both WANT_READ and WANT_WRITE. For WANT_READ no write failed: progress comes from the read side, and the flag kept EPOLLOUT (level-triggered) or the kqueue EVFILT_WRITE one-shot armed on an always-writable socket. Every tick re-entered the handshake with zero progress: 100% CPU whenever writable interest existed while the handshake stalled. us_socket_pause() arms writable, so pause() before the handshake finished spun until resume(); a peer that stalls its flight behind a blocked write spun the server until the idle timeout. WANT_WRITE keeps the flag, though the blocked BIO write normally sets it itself inside us_socket_raw_write. Measured with the new fixture: 2798ms CPU over a 2s stall window before, quiet after.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe OpenSSL handshake path now distinguishes read and write retry conditions. A subprocess regression test verifies that a paused, stalled TLS handshake does not spin the event loop. ChangesTLS handshake polling
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: 2
🤖 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/bun/net/tls-handshake-pause-spin-fixture.ts`:
- Line 22: Update the TLS fixture’s socket callbacks and CPU-check flow so
handshake, error, end, and close signals are recorded and cause the fixture to
exit non-zero after cleanup if they occur before sampling. Preserve the intended
stall check, and ensure the test explicitly fails through each relevant
completion or failure event rather than discarding callbacks.
- Around line 25-26: Replace the fixed 100ms warm-up in the TLS handshake
fixture with readiness synchronization: resolve a promise from the relevant
open() callback after the first writable dispatch, await it, then yield one
macrotask with Bun.sleep(0). Keep the existing 2-second sleep unchanged as the
measurement interval.
🪄 Autofix
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: e7228522-17d3-4571-b8e1-53cbef11258f
📒 Files selected for processing (3)
packages/bun-usockets/src/crypto/openssl.ctest/js/bun/net/socket.test.tstest/js/bun/net/tls-handshake-pause-spin-fixture.ts
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this touches the TLS handshake state machine in usockets — every TLS connection goes through this path across three event-loop backends (epoll, kqueue, libuv) — so a maintainer familiar with the usockets writable/readable interest model should confirm.
What was reviewed:
- Traced the spin:
us_socket_pausearms only WRITABLE (socket.c:844) → writable fires →ssl_update_handshake→ WANT_READ setlast_write_failed→ loop.c:611 kept writable armed on an always-writable fd. - Checked the drop-writable-on-WANT_READ path doesn't stall an unpaused handshake:
ssl_write_wants_readis still set, so incoming data re-drives viassl_retry_parked_write→us_internal_ssl_on_writable. - Confirmed WANT_WRITE keeps writable armed independently via the BIO write →
us_socket_raw_write→us_internal_rearm_writablepath (socket.c:501-502), so the explicit flag here is belt-and-suspenders for BIO retry paths. - Fixture measures CPU over a 2s idle window with an 800ms threshold; server is plain TCP so ServerHello never arrives and
pause()inopen()guarantees the WANT_READ stall.
Extended reasoning...
Overview
One behavioral change in packages/bun-usockets/src/crypto/openssl.c (ssl_update_handshake): the unconditional s->flags.last_write_failed = 1 on both SSL_ERROR_WANT_READ and SSL_ERROR_WANT_WRITE becomes conditional on WANT_WRITE only. A new fixture (tls-handshake-pause-spin-fixture.ts) plus a test entry in socket.test.ts reproduce a 100% CPU spin when a TLS client calls pause() before the handshake completes.
Mechanism verified
us_socket_pause (socket.c:839-844) sets poll events to LIBUS_SOCKET_WRITABLE only. The fd is immediately writable, so the writable event fires; loop.c:590 clears last_write_failed, dispatches to us_internal_ssl_on_writable → ssl_update_handshake → SSL_do_handshake returns WANT_READ (no ServerHello yet). The old code set last_write_failed = 1, so loop.c:611 kept (epoll) or re-registered (kqueue one-shot) writable interest, and the always-writable fd fired again immediately. With the fix, WANT_READ leaves the flag cleared, loop.c:611-612 drops writable interest via us_poll_events(&s->p) & LIBUS_SOCKET_READABLE, and a paused socket ends up with zero poll interest — correct, since resume() re-arms readable.
I checked the non-paused case for a stall regression: ssl_write_wants_read is still set unconditionally at line 2034, so when the peer's handshake bytes arrive on the readable side, us_internal_ssl_on_data runs and ssl_retry_parked_write (openssl.c:2105-2110) tail-calls us_internal_ssl_on_writable to re-drive the handshake. Dropping writable interest on WANT_READ therefore does not strand a normal handshake.
Security risks
None introduced. This is an event-loop-interest bookkeeping fix; it does not alter certificate verification, cipher negotiation, or any TLS protocol handling. The only failure mode of getting this wrong is a stall or a spin, not a security bypass.
Level of scrutiny
High. ssl_update_handshake runs on every TLS socket's open, writable, SNI-resolve, and startTLS paths, across epoll (level-triggered), kqueue (one-shot EVFILT_WRITE), and libuv/Windows. A subtle mistake here would affect every HTTPS/TLS connection Bun makes or accepts. The reasoning is sound and the bug-hunting pass surfaced nothing, but the blast radius alone puts this above the bar for auto-approval.
Other factors
- The 10-line block comment is heavier than house style prefers ("one line"); a maintainer may want it trimmed to a sentence plus an issue link.
- The fixture uses a fixed
await Bun.sleep(2000)window and a CPU threshold rather than an event-driven signal; that's acceptable here because the property being tested is CPU consumption over idle time, and 800ms/2000ms leaves ample headroom for debug+ASAN. The PR notes it fails at ~2798ms on the unfixed build. - PR body reports
node-tls-connect.test.ts(53 tests) still passes, which covers normal handshake completion.
Review feedback: the fixture now records handshake/end/error/close firing before the CPU sample (precondition breakage fails loudly instead of passing vacuously), and the warm-up waits for open() plus one macrotask instead of a fixed 100ms. Also trims the openssl.c comment.
There was a problem hiding this comment.
I didn't find any bugs, but this changes TLS handshake state-machine polling in openssl.c — a critical path every TLS connection traverses — so it's worth a human look.
What was reviewed:
- Traced
last_write_failedthrough loop.c:589-618: after the fix, WANT_READ drops writable and keeps whatever readable interest exists viaus_poll_events(&s->p) & LIBUS_SOCKET_READABLE. - Confirmed
ssl_write_wants_read = 1is still set for both WANT_READ/WANT_WRITE, sossl_retry_parked_writere-drives the handshake fromon_datawhen the peer's flight arrives — no new hang path. - Checked the fixture: precondition guards wire handshake/error/end/close to failure, the 2s sleep is a measurement window (not a wait), and the 800ms threshold is well below the ~2800ms unfixed spin.
Extended reasoning...
Overview
The PR changes one line in packages/bun-usockets/src/crypto/openssl.c's ssl_update_handshake: it now sets s->flags.last_write_failed = 1 only when SSL_do_handshake returns SSL_ERROR_WANT_WRITE, not also for SSL_ERROR_WANT_READ. It adds a subprocess fixture (tls-handshake-pause-spin-fixture.ts) that pauses a TLS client mid-handshake against a plain-TCP server that never sends a ServerHello, then measures process CPU over a 2s idle window; the test in socket.test.ts runs it via bunRun.
Security risks
This is TLS handshake code. The change does not touch certificate verification, cipher selection, or any crypto operation — it only affects whether writable-poll interest is kept armed while the handshake is stalled on a read. The risk profile is availability, not confidentiality/integrity: a mistake here could cause handshakes to hang (never re-driven) rather than leak or corrupt anything. I traced the re-drive path: ssl_write_wants_read is still set unconditionally at line 2034, and us_internal_ssl_on_data → ssl_retry_parked_write → us_internal_ssl_on_writable → ssl_update_handshake re-drives the handshake when the peer's bytes arrive on the readable side. For WANT_WRITE, the flag is still set (and the BIO's us_socket_raw_write also sets it when the send actually blocks), so blocked-write handshakes still re-arm writable. I don't see a hang regression, but the epoll-vs-kqueue polling interaction (loop.c lines 611-618, kqueue one-shot re-arm) is subtle enough that a maintainer familiar with the usockets poll model should confirm.
Level of scrutiny
High. ssl_update_handshake runs for every TLS socket on every writable dispatch during the handshake, across epoll (Linux) and kqueue (macOS/BSD) which have different re-arm semantics. The fix is one conditional, but the surrounding state machine (last_write_failed, ssl_write_wants_read, ssl_read_wants_write, kqueue one-shot vs epoll level-trigger) has many interacting flags. The PR description's mechanism explanation is coherent and matches what I read in loop.c, and the author reports node-tls-connect.test.ts (53 tests) still passes.
Other factors
The test is well-constructed: it now guards the stall precondition (handshake/error/end/close all fail the fixture, addressed after CodeRabbit feedback in bf96c93), awaits the open event plus one macrotask before sampling, and uses CPU-time (not wall-clock) with a threshold ~3.5× below the observed unfixed spin. The 2-second Bun.sleep is the measurement interval for a does-not-happen check, which REVIEW.md permits. Both CodeRabbit inline comments are marked resolved. No prior review from me on this PR.
2 participants
What
A TLS socket whose handshake is stalled on
SSL_ERROR_WANT_READspins the event loop at 100% CPU whenever writable interest is armed.pause()before the handshake finishes is the unbounded case (writable is armed by pause and the ServerHello can never be consumed), measured at 2798ms CPU over a 2s idle window; a peer that stalls its next flight behind one blocked server write gets the bounded-but-renewable variant until the idle timeout.Why
ssl_update_handshakesets->flags.last_write_failed = 1for bothSSL_ERROR_WANT_READandSSL_ERROR_WANT_WRITE. The flag means "the last write would have blocked", and the writable dispatch (loop.c) uses it to keep EPOLLOUT / the kqueueEVFILT_WRITEone-shot armed. For WANT_READ no write failed, so the socket is immediately writable again: writable event,SSL_do_handshake, WANT_READ, re-arm, forever.WANT_WRITE keeps the flag (its blocked BIO write normally sets it itself inside
us_socket_raw_write).Fix
Set
last_write_failedonly forSSL_ERROR_WANT_WRITE(packages/bun-usockets/src/crypto/openssl.c,ssl_update_handshake).Verification
New fixture
test/js/bun/net/tls-handshake-pause-spin-fixture.ts(plain TCP server that never sends a ServerHello, TLS client pauses inopen(), CPU measured over a 2s window): fails on current bun withSPIN: 2798ms, passes with this change.test/js/node/tls/node-tls-connect.test.ts(53 tests) passes;socket.test.ts/tcp-server.test.tsshow only the failures already present on main in this environment.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/net/socket.test.ts