Skip to content

http: propagate highWaterMark to ClientRequest OutgoingMessage#64653

Open
trivenay wants to merge 1 commit into
nodejs:mainfrom
trivenay:http-client-hwm-propagate
Open

http: propagate highWaterMark to ClientRequest OutgoingMessage#64653
trivenay wants to merge 1 commit into
nodejs:mainfrom
trivenay:http-client-hwm-propagate

Conversation

@trivenay

@trivenay trivenay commented Jul 21, 2026

Copy link
Copy Markdown

http.request({ highWaterMark }) passes the value to the TCP socket via createConnection() but does not set it on the OutgoingMessage's internal kHighWaterMark. OutgoingMessage._writeRaw() has two mutually exclusive write paths:

  • Path A (socket connected): conn.write() — uses socket's HWM (correct)
  • Path B (no socket yet): outputSize < this[kHighWaterMark] — uses OutgoingMessage's own default (64 KB) (incorrect)

Because the OutgoingMessage constructor already accepts options.highWaterMark, the fix is to set kHighWaterMark from the user's 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's 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 http: emit 'drain' on OutgoingMessage only after buffers drain #62936).

Test results (compiled from source)

=== UNPATCHED Node v26.5.0 ===
FAIL: write() returned false (expected true)

=== PATCHED Node v27.0.0-pre (this PR) ===
PASS: write() returned true
test-http-client-highwatermark.js passes (exit 0)
test-http-server-options-highwatermark.js passes
test-http-outgoing-drain-writable-length.js passes
test-http-outgoing-end-cork.js passes
test-http-highwatermark.js passes
test-http-response-drain-cork.js passes

Minimal reproduction

node -e "require('http').createServer((req,res)=>{req.resume();req.on('end',()=>res.end('ok'))}).listen(9001)"
const http = require('http');
const req = http.request({ hostname: '127.0.0.1', port: 9001, method: 'POST', highWaterMark: 100_000 });
const ok = req.write(Buffer.alloc(64 * 1024));
console.log(`write() returned ${ok}, expected true (64KB < 100KB highWaterMark)`);
if (!ok) {
  setTimeout(() => { console.error('DEADLOCK: drain never fired'); process.exit(1); }, 3000);
  req.on('drain', () => { console.log('drain fired'); req.end(); });
} else {
  req.end();
  req.on('response', (res) => { res.resume(); res.on('end', () => process.exit(0)); });
}
Node version write() returns Drain fires? Outcome
v22.x false (wrong) Yes (old eager drain) HWM ignored but no hang
v26.5.0 false (wrong) Never DEADLOCK
This PR true (correct) N/A (no backpressure) Works correctly

Fixes: #64645
Refs: #62936

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Jul 21, 2026
`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
@trivenay
trivenay force-pushed the http-client-hwm-propagate branch from b1249d0 to a008632 Compare July 21, 2026 18:13
@codecov

codecov Bot commented Jul 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.13%. Comparing base (8c1d128) to head (a008632).
⚠️ Report is 13 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #64653   +/-   ##
=======================================
  Coverage   90.12%   90.13%           
=======================================
  Files         741      741           
  Lines      242091   242142   +51     
  Branches    45563    45563           
=======================================
+ Hits       218180   218250   +70     
+ Misses      15430    15394   -36     
- Partials     8481     8498   +17     
Files with missing lines Coverage Δ
lib/_http_client.js 97.62% <100.00%> (+0.01%) ⬆️

... and 36 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@trivikr trivikr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@trivikr trivikr added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 21, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 21, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

http: ClientRequest highWaterMark option not propagated to OutgoingMessage

4 participants