Skip to content

Commit 6879aa4

Browse files
trivenayaduh95
authored andcommitted
http: propagate highWaterMark to ClientRequest OutgoingMessage
`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 #62936). Signed-off-by: Naman Trivedi <trivenay@amazon.com> Fixes: #64645 Refs: #62936 PR-URL: #64653 Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Tim Perry <pimterry@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
1 parent aa3f168 commit 6879aa4

2 files changed

Lines changed: 90 additions & 0 deletions

File tree

lib/_http_client.js

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@ const {
5151
kSkipPendingData,
5252
} = require('_http_common');
5353
const {
54+
kHighWaterMark,
5455
kUniqueHeaders,
5556
parseUniqueHeadersOption,
5657
OutgoingMessage,
@@ -348,6 +349,14 @@ function ClientRequest(input, options, cb) {
348349
options = ObjectAssign({ __proto__: null }, input, options);
349350
}
350351

352+
// Propagate the user's highWaterMark to OutgoingMessage so that
353+
// _writeRaw() uses the correct threshold for writes buffered before
354+
// the socket connects (Path B). Without this, the OutgoingMessage
355+
// defaults to 64 KB regardless of what the caller requested.
356+
if (options.highWaterMark != null) {
357+
this[kHighWaterMark] = options.highWaterMark;
358+
}
359+
351360
let agent = options.agent;
352361
const defaultAgent = options._defaultAgent || Agent.globalAgent;
353362
if (agent === false) {
Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
// Flags: --expose-internals
2+
'use strict';
3+
4+
// Regression test: http.request({ highWaterMark }) must propagate the value
5+
// to OutgoingMessage's kHighWaterMark so that _writeRaw() Path B (buffering
6+
// before socket connects) uses the correct threshold.
7+
//
8+
// Without the fix:
9+
// - write() returns the wrong boolean (compares against default 64KB)
10+
// - On Node >= 24.16.0 (post #62936), this causes a deadlock when
11+
// the user awaits 'drain' after write() incorrectly returns false.
12+
//
13+
// Fixes: https://github.com/nodejs/node/issues/64645
14+
15+
const common = require('../common');
16+
const assert = require('assert');
17+
const http = require('http');
18+
const { kHighWaterMark } = require('_http_outgoing');
19+
const { getDefaultHighWaterMark } = require('internal/streams/state');
20+
21+
const server = http.createServer(common.mustCall((req, res) => {
22+
req.resume();
23+
req.on('end', () => res.end('ok'));
24+
}, 3));
25+
26+
server.listen(0, common.mustCall(() => {
27+
const port = server.address().port;
28+
let completed = 0;
29+
30+
function done() {
31+
if (++completed === 3) server.close();
32+
}
33+
34+
// Test 1: kHighWaterMark is set to user value on the ClientRequest.
35+
{
36+
const hwm = getDefaultHighWaterMark() * 2;
37+
const req = http.request({ port, method: 'POST', highWaterMark: hwm });
38+
assert.strictEqual(req[kHighWaterMark], hwm);
39+
req.end();
40+
req.on('response', (res) => { res.resume(); res.on('end', done); });
41+
}
42+
43+
// Test 2: Large HWM — write below threshold returns true before socket connects.
44+
{
45+
const req = http.request({
46+
port,
47+
method: 'POST',
48+
highWaterMark: 100_000,
49+
}, common.mustCall((res) => {
50+
res.resume();
51+
res.on('end', done);
52+
}));
53+
54+
// 64KB write in the same tick — socket not yet connected (Path B).
55+
// With HWM=100KB, write() must return true.
56+
const result = req.write(Buffer.alloc(64 * 1024));
57+
assert.strictEqual(result, true);
58+
req.end();
59+
}
60+
61+
// Test 3: Small HWM — write above threshold returns false, drain fires.
62+
{
63+
const req = http.request({
64+
port,
65+
method: 'POST',
66+
highWaterMark: 512,
67+
}, common.mustCall((res) => {
68+
res.resume();
69+
res.on('end', done);
70+
}));
71+
72+
// 2KB write in the same tick — exceeds HWM of 512 bytes.
73+
const result = req.write(Buffer.alloc(2 * 1024));
74+
assert.strictEqual(result, false);
75+
76+
// Drain must fire (no deadlock) so we can complete the request.
77+
req.on('drain', common.mustCall(() => {
78+
req.end();
79+
}));
80+
}
81+
}));

0 commit comments

Comments
 (0)