http: propagate highWaterMark to ClientRequest OutgoingMessage#64653
Open
trivenay wants to merge 1 commit into
Open
http: propagate highWaterMark to ClientRequest OutgoingMessage#64653trivenay wants to merge 1 commit into
trivenay wants to merge 1 commit into
Conversation
`http.request({ highWaterMark })` passes the value to the TCP socket
via createConnection() but does not set it on the OutgoingMessage
internal kHighWaterMark. OutgoingMessage._writeRaw() has two mutually
exclusive write paths:
Path A (socket connected): conn.write() — uses socket HWM ✓
Path B (no socket yet): outputSize < this[kHighWaterMark] — uses
OutgoingMessage own default (64 KB) ✗
Because the OutgoingMessage constructor already accepts
options.highWaterMark, the fix is to set kHighWaterMark from the
user options after they are parsed in the ClientRequest constructor.
This resolves two symptoms:
1. write() returning the wrong boolean for pre-socket writes (the
user highWaterMark was silently ignored on all Node versions).
2. A deadlock on Node >= 24.16.0 where the incorrect false return
sets kNeedDrain, but drain never fires because the socket was
never backpressured (introduced by the stricter drain gate in
nodejs#62936).
Signed-off-by: Trivikram Narayanan Kamasamudram <trivenay@amazon.com>
Fixes: nodejs#64645
Refs: nodejs#62936
Collaborator
|
Review requested:
|
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.
http.request({ highWaterMark })passes the value to the TCP socket viacreateConnection()but does not set it on theOutgoingMessage's internalkHighWaterMark.OutgoingMessage._writeRaw()has two mutually exclusive write paths:conn.write()— uses socket's HWM (correct)outputSize < this[kHighWaterMark]— uses OutgoingMessage's own default (64 KB) (incorrect)Because the
OutgoingMessageconstructor already acceptsoptions.highWaterMark, the fix is to setkHighWaterMarkfrom the user's options after they are parsed in theClientRequestconstructor.This resolves two symptoms:
write()returning the wrong boolean for pre-socket writes (the user'shighWaterMarkwas silently ignored on all Node versions).falsereturn setskNeedDrain, butdrainnever fires because the socket was never backpressured (introduced by the stricter drain gate in http: emit 'drain' on OutgoingMessage only after buffers drain #62936).Test results (compiled from source)
Minimal reproduction
node -e "require('http').createServer((req,res)=>{req.resume();req.on('end',()=>res.end('ok'))}).listen(9001)"write()returnsfalse(wrong)false(wrong)true(correct)Fixes: #64645
Refs: #62936