fix(ext/node): fix TLS socket lifecycle and readStop backpressure#33524
Conversation
readStop() clears inner.onread to stop delivering decrypted data, but do_emit_read() had a fallback that looked up "onread" on the JS handle object. This defeated backpressure entirely -- data and EOF kept flowing to the stream even after readStop was called. Fix: - Remove the JS property fallback in do_emit_read. When onread is None, return immediately instead of bypassing readStop. - Buffer decrypted cleartext in pending_clear_out when readStop is active, so data is not lost. - Defer EOF delivery via pending_eof flag when readStop is active. - Flush buffered data and deferred EOF in read_start before the normal cycle, preserving data ordering. - Update emit_eof to also respect readStop state.
09ca30f to
946b324
Compare
The _onReadableStreamEnd change to call socket.end() via nextTick broke the child-process-stdio-merge-stdouts-into-cat test on Windows. When child processes share a pipe handle (p3.stdin) as stdout, the auto-end closes the pipe prematurely when one child exits, preventing subsequent children from writing through it. The TLS backpressure fix does not depend on this change.
… test" This reverts commit 8fa1169.
lunadogbot
left a comment
lunadogbot
left a comment
There was a problem hiding this comment.
LGTM. Walked through the lifecycle change with the assumption something was wrong; the design holds up:
- Removing the
recv.get(scope, key.into())fallback indo_emit_readis correct --inner.onreadis the single source of truth (set inread_start, cleared inread_stop), and the JS-side property lookup was racing the readStop bookkeeping. - The buffering invariants are preserved:
pending_clear_outis only ever populated whenonread.is_none()(matched in bothcyclesites and the newdispatch_clear_out_callbackssite), and read_start drains it BEFOREcycleruns again, so consumers see decrypted bytes in arrival order. Thepending_eof-after-pending_dataordering in the flush block matches that. _start()callingthis._handle.readStart()is the right way to flush a close_notify that landed during handshake (before JS installedonread); the call is idempotent becauseread_startre-grabsonreadfrom the same JS handle each time andpending_clear_outis only non-empty on the first call after a stop._onReadableStreamEnd'snextTick-deferredsocket.end()(gated onwritable && !destroyed) safely handles the case where user code already calledend(). Matches Node's behavior of letting the user'sendlistener observesocket.writable === true.- The
#[fast]->#[nofast]flip onread_startis forced by the new flush block triggering v8 callbacks (do_emit_read) which can't run inside a fast-call frame; consistent with the droppedOpStatearg.
New spec test exercises HWM=1024 over 64KB to force backpressure, asserts both byte count and end -- without the fix the safety timer fires and the test exits 1 loud, which is the right shape for a hang regression.
| // This must happen before the cycle below so the consumer sees the | ||
| // data in the order it was decrypted. | ||
| let inner_ptr = inner as *mut TLSWrapInner; | ||
| let pending_data = std::mem::take(&mut inner.pending_clear_out); |
There was a problem hiding this comment.
Minor: this mem::take clears pending_clear_out before the extract_emit_ctx(...) check. If extract_emit_ctx returns None (isolate unset / js_handle dropped), the buffered bytes are silently dropped on the floor.
In practice this can't happen here -- read_start is reachable only from a v8 callback, so isolate and js_handle are both alive. But defensively you could either restore inner.pending_clear_out = pending_data; inner.pending_eof = pending_eof; in the None branch, or debug_assert!(extract_emit_ctx is Some). Not a blocker.
fibibot
left a comment
fibibot
left a comment
There was a problem hiding this comment.
LGTM. Walked all five fixes:
-
do_emit_readJS-property fallback removal — therecv.get(scope, "onread")lookup as a fallback for missinginner.onreadis the actual bug: it bypasses readStop entirely (since readStop setsinner.onread = None, but the JS handle'sonreadproperty is still there from before). With it removed, missingonreadcorrectly returns and lets the new buffering paths kick in. -
pending_clear_out/pending_eofbuffering at the twocycle()callsites and atdo_shutdown_complete— when(*ptr).onread.is_none(), decrypted data and EOF accumulate instead of being dropped. Order is preserved (the post-readStart flush replays data first, then EOF). -
read_startflush block — does the right thing in the right order:mem::take(&mut inner.pending_clear_out)before the cycle, dispatch buffered data viado_emit_read, then EOF. Thelet inner_ptr = inner as *mut TLSWrapInneris shared with the cycle below, which is fine since the buffered-flush path completes before the cycle runs. -
_start()callingthis._handle.readStart()— close_notify can be processed during the handshake beforeinner.onreadis set, so the EOF lands inpending_eof. Without the kick-startreadStart()here, the'end'event would never fire (test-tls-econnreset / test-tls-close-notify hang). With it, readStart installs onread and flushes. -
net.Socket._onReadableStreamEndcallingthis.end()— pure Node-compat fix;allowHalfOpen=falseis the spec-default and Node sends close_notify viathis.end()in that case. ThenextTickwrapper correctly guards the late-registered-handler-sees-writable=true case. -
OpStateparam removal fromread_start— was unused, and the&mut OpStateborrow could conflict with the JS-callback re-entry viado_emit_readif any callee touched OpState (correct preventive cleanup, though I didn't find a concrete reentrant panic on the current shape).
The new tls_readstop_backpressure spec test exercises the path explicitly (64KB through a 1KB-HWM stream → buffering must be involved) and asserts both byte count and 'end' firing.
One non-blocking concern inline.
| if let Some(ctx) = extract_emit_ctx(ptr) { | ||
| if (*ptr).onread.is_none() { | ||
| // readStop is active -- buffer the data for later delivery | ||
| (*ptr).pending_clear_out.extend_from_slice(&result.data); |
There was a problem hiding this comment.
Non-blocking: pending_clear_out is unbounded — if a consumer holds readStop indefinitely while the peer keeps sending (and the underlying TCP receive buffer keeps draining into rustls), this extend_from_slice will keep growing. In practice the TCP-level backpressure should kick in once rustls stops draining, but worth a thought: a malicious peer + a forgotten readStop = unbounded memory. Node's node_tls_wrap.cc has a similar buffer, so this is more about awareness than a blocker.
3 participants
Summary
readStopbypassing backpressure in TLSWrap --do_emit_readhad a JS property fallback that delivered data even afterreadStop, breaking flow controlpending_clear_out/pending_eofbuffering so data received duringreadStopis not lostreadStartto handle close_notify received during handshake beforeonreadis installedthis._handle.readStart()in_start()to kick-start the TLS readable side after handshakenet.Socket._onReadableStreamEndto callthis.end()whenallowHalfOpenis false, matching Node.js -- without this, sockets never sent close_notify back to the peerOpStateparam fromTLSWrap::read_startto prevent reentrant RefCell panicsFixes 7+ previously-hanging node compat TLS tests:
test-tls-connect-simple,test-tls-close-event-after-write,test-tls-close-notify,test-tls-econnreset,test-tls-ca-concat,test-tls-connect-hwm-option,test-tls-destroy-whilst-write,test-tls-client-abort2Likely related to #33266 (Wrangler asset upload regression).
Test plan
tls_readstop_backpressure-- sends 64KB through TLS with 1KB highWaterMark, verifies all data arrives andendfirestls_keepalive_large_body,tls_jsstreamsocket_close)