http: emit drain on socket takeover and avoid stale HWM reuse - #64991
Open
trivenay wants to merge 1 commit into
Open
http: emit drain on socket takeover and avoid stale HWM reuse#64991trivenay wants to merge 1 commit into
trivenay wants to merge 1 commit into
Conversation
Collaborator
|
Review requested:
|
When OutgoingMessage transitions from pre-socket buffering (Path B) to socket-connected writing (Path A), the backpressure domain changes — subsequent writes go directly to the socket, which enforces its own backpressure via socket.write() return values. The OM should emit drain at this transition point to signal that its buffer is clear and the caller can resume writing under the socket backpressure regime. Previously, _flush() gated drain emission on writableLength === 0 which included socket.writableLength. This conflated two independent backpressure domains: the OM pre-socket buffer and the socket kernel write queue. When the socket had a higher writableHighWaterMark than the OM (e.g. agent-reused socket from a prior request), the socket was never backpressured and never emitted drain, causing a permanent deadlock. Additionally, avoid reusing a pooled socket in http.Agent when its writableHighWaterMark differs from the request highWaterMark, so that the user backpressure threshold is respected for the common case of the built-in Agent. Signed-off-by: Naman Trivedi <trivenay@amazon.com> Fixes: nodejs#64680 Refs: nodejs#64653 Refs: nodejs#62936
trivenay
force-pushed
the
http-agent-hwm-no-reuse
branch
from
August 3, 2026 22:29
afb656c to
b307aa7
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #64991 +/- ##
==========================================
- Coverage 90.27% 90.25% -0.02%
==========================================
Files 762 762
Lines 247534 247548 +14
Branches 46694 46689 -5
==========================================
- Hits 223457 223424 -33
- Misses 15529 15541 +12
- Partials 8548 8583 +35
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When
OutgoingMessagetransitions from pre-socket buffering (Path B) to socket-connected writing (Path A), the backpressure domain changes. The OM should emitdrainat this transition to signal that its buffer is clear and the caller can resume writing under the socket's own backpressure.Previously,
_flush()gated drain emission onwritableLength === 0(which includessocket.writableLength). This conflated the OM's buffer state with the socket's kernel write queue. When the socket had a higherwritableHighWaterMarkthan the OM (e.g., agent reuses a socket from a prior request with a different HWM), the socket was never backpressured, never emitted drain — permanent deadlock.Approach
This PR makes two changes to address the problem:
1. Drain fix in
_flush()(the must-have): Once_flushOutput()completes and all buffered data has been handed to the socket, emit drain unconditionally. From this point, the socket enforces its own backpressure viasocket.write()return values. We don't wait forsocket.writableLengthto reach zero because that's the socket's backpressure domain — not the OM's. If the socket is full, the very nextwrite()through Path A will returnfalseand the user stops writing again naturally.2. Agent HWM mismatch check (defense in depth): Don't reuse a pooled socket in
http.Agentif itswritableHighWaterMarkdiffers from the request'shighWaterMark. This ensures the user's backpressure threshold is respected for users of the built-inhttp.Agent. We chose to include this becausehighWaterMarkon a connected TCP socket cannot be changed after creation (the underlying kernel buffer is not exposed via Node's TCP handle, and_writableState.highWaterMarkis cosmetic sincestate.lengthstays 0 for connected sockets). Since there's no way to make a reused socket respect a different HWM, the most resilient approach is to not reuse it. For requests to the same host:port it's rare that differenthighWaterMarkvalues are used, so socket reuse still happens for the vast majority of connections.The drain fix alone prevents the deadlock universally (including custom agents and
createConnection). The agent check additionally ensures correct backpressure behavior — not just absence of deadlock — for the common case.Deadlock reproduction (requires reduced TCP send buffer)
Fixes: #64680
Refs: #64653
Refs: #62936