http: emit drain on socket takeover and avoid stale HWM reuse · nodejs/node@dad9548 · GitHub
Skip to content

Commit dad9548

Browse files
trivenayaduh95
authored andcommitted
http: emit drain on socket takeover and avoid stale HWM reuse
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: #64680 Refs: #64653 Refs: #62936 PR-URL: #64991 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day>
1 parent 4674a10 commit dad9548

4 files changed

Lines changed: 127 additions & 2 deletions

File tree

lib/_http_agent.js

Lines changed: 10 additions & 0 deletions

lib/_http_outgoing.js

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1230,12 +1230,14 @@ OutgoingMessage.prototype._flush = function _flush() {
12301230

12311231
if (socket?.writable) {
12321232
// There might be remaining data in this.output; write it out
1233-
this._flushOutput(socket);
1233+
const ret = this._flushOutput(socket);
12341234

12351235
if (this.finished) {
12361236
// This is a queue to the server or client to bring in the next this.
12371237
this._finish();
1238-
} else if (this[kNeedDrain] && this.writableLength === 0) {
1238+
} else if (this[kNeedDrain] && ret !== false) {
1239+
// Socket accepted all data without backpressure - it won't emit
1240+
// drain, so we emit it since the OM buffer is now clear.
12391241
this[kNeedDrain] = false;
12401242
this.emit('drain');
12411243
}
Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
'use strict';
2+
3+
// Regression test: when a pooled socket's writableHighWaterMark differs from
4+
// the new request's highWaterMark, the agent must sync the socket's HWM so
5+
// that backpressure semantics match what the caller requested.
6+
//
7+
// See: https://github.com/nodejs/node/issues/64680
8+
9+
const common = require('../common');
10+
const assert = require('assert');
11+
const http = require('http');
12+
13+
const server = http.createServer(common.mustCall((req, res) => {
14+
req.resume();
15+
req.on('end', () => res.end('ok'));
16+
}, 2));
17+
18+
server.listen(0, common.mustCall(() => {
19+
const port = server.address().port;
20+
const agent = new http.Agent({ keepAlive: true });
21+
22+
// Request A: creates socket with HWM=1MB.
23+
http.request({
24+
host: 'localhost', port, method: 'POST', agent,
25+
highWaterMark: 1024 * 1024,
26+
}, common.mustCall((res) => {
27+
res.resume();
28+
res.on('end', common.mustCall(() => {
29+
// Wait for socket to return to pool.
30+
setTimeout(common.mustCall(requestB), 100);
31+
}));
32+
})).end('x');
33+
34+
function requestB() {
35+
const freeCount = Object.values(agent.freeSockets).flat().length;
36+
assert.strictEqual(freeCount, 1);
37+
38+
// Request B: HWM=10KB — agent must sync the reused socket's HWM.
39+
const reqB = http.request({
40+
host: 'localhost', port, method: 'POST', agent,
41+
highWaterMark: 10 * 1024,
42+
}, common.mustCall((res) => {
43+
res.resume();
44+
res.on('end', common.mustCall(() => {
45+
server.close();
46+
}));
47+
}));
48+
49+
reqB.on('socket', common.mustCall((socket) => {
50+
// Socket HWM must be synced to the request's value.
51+
assert.strictEqual(socket.writableHighWaterMark, 10 * 1024);
52+
}));
53+
54+
reqB.end('y');
55+
}
56+
}));
Lines changed: 57 additions & 0 deletions

0 commit comments

Comments
 (0)