Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Commit 717476a

Browse filesBrowse files
ronagaduh95
authored andcommitted
http: emit 'drain' on OutgoingMessage only after buffers drain
Previously, socketOnDrain could be invoked synchronously from _flushOutput (via _onPendingData -> updateOutgoingData) while the bytes just handed to the socket were still buffered and while outputSize had not yet been reset on the OutgoingMessage. The 'drain' event fired even though res.writableLength was non-zero, breaking the invariant a user would reasonably expect after `while (!res.write(...));`. Gate the emission in socketOnDrain on msg.writableLength === 0 (which also covers outputSize + chunked buffer + socket.writableLength), and apply the same check in OutgoingMessage._flush so that 'drain' is only emitted when the response is genuinely drained. The socket's own 'drain' event will otherwise propagate through socketOnDrain when the socket buffer actually empties. Signed-off-by: Robert Nagy <ronagy@icloud.com> Assisted-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> PR-URL: #62936 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ethan Arrowood <ethan@arrowood.dev> Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day>
1 parent 1397d8c commit 717476a
Copy full SHA for 717476a

3 files changed

+67-3Lines changed: 67 additions & 3 deletions

File tree

Expand file treeCollapse file tree
Open diff view settings
Filter options
Expand file treeCollapse file tree
Open diff view settings
Collapse file

‎lib/_http_outgoing.js‎

Copy file name to clipboardExpand all lines: lib/_http_outgoing.js
+2-2Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1144,12 +1144,12 @@ OutgoingMessage.prototype._flush = function _flush() {
11441144

11451145
if (socket?.writable) {
11461146
// There might be remaining data in this.output; write it out
1147-
const ret = this._flushOutput(socket);
1147+
this._flushOutput(socket);
11481148

11491149
if (this.finished) {
11501150
// This is a queue to the server or client to bring in the next this.
11511151
this._finish();
1152-
} else if (ret && this[kNeedDrain]) {
1152+
} else if (this[kNeedDrain] && this.writableLength === 0) {
11531153
this[kNeedDrain] = false;
11541154
this.emit('drain');
11551155
}
Collapse file

‎lib/_http_server.js‎

Copy file name to clipboardExpand all lines: lib/_http_server.js
+7-1Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -819,7 +819,13 @@ function socketOnDrain(socket, state) {
819819
}
820820

821821
const msg = socket._httpMessage;
822-
if (msg && !msg.finished && msg[kNeedDrain]) {
822+
// Only emit 'drain' once the message has no data pending anywhere, so that
823+
// msg.writableLength === 0 when the event fires. socketOnDrain is called
824+
// synchronously from updateOutgoingData during _flushOutput, at which point
825+
// the bytes we just handed to the socket (or the stale outputSize) mean
826+
// the message is not actually drained yet - we wait for the socket's
827+
// own 'drain' event instead.
828+
if (msg && !msg.finished && msg[kNeedDrain] && msg.writableLength === 0) {
823829
msg[kNeedDrain] = false;
824830
msg.emit('drain');
825831
}
Collapse file
+58Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
'use strict';
2+
// Regression test: when a pipelined ServerResponse (whose writes were
3+
// buffered in outputData while the socket belonged to a previous response)
4+
// is finally assigned its socket and flushed, 'drain' must not be emitted
5+
// until the socket's own buffer has actually drained. Previously,
6+
// socketOnDrain was called synchronously from _flushOutput via _onPendingData
7+
// and emitted 'drain' even though the bytes we just wrote were still sitting
8+
// in the socket's writable buffer, so res.writableLength was non-zero.
9+
10+
const common = require('../common');
11+
const http = require('http');
12+
const net = require('net');
13+
const assert = require('assert');
14+
15+
let step = 0;
16+
17+
const server = http.createServer(common.mustCall((req, res) => {
18+
step++;
19+
20+
if (step === 1) {
21+
// Keep the first response open briefly so the second is queued with
22+
// res.socket === null.
23+
res.writeHead(200, { 'Content-Type': 'text/plain' });
24+
setTimeout(() => res.end('ok'), 50);
25+
return;
26+
}
27+
28+
// Second (pipelined) response - queued in state.outgoing, no socket yet.
29+
assert.strictEqual(res.socket, null);
30+
31+
res.writeHead(200, { 'Content-Type': 'text/plain' });
32+
33+
// Write past the response's highWaterMark so res.write() returns false
34+
// and kNeedDrain is set. Data is buffered in outputData.
35+
const chunk = Buffer.alloc(16 * 1024, 'x');
36+
while (res.write(chunk));
37+
assert.strictEqual(res.writableNeedDrain, true);
38+
39+
res.on('drain', common.mustCall(() => {
40+
assert.strictEqual(
41+
res.writableLength, 0,
42+
`'drain' fired with writableLength=${res.writableLength}`,
43+
);
44+
res.end();
45+
server.close();
46+
}));
47+
}, 2));
48+
49+
server.listen(0, common.mustCall(function() {
50+
const port = this.address().port;
51+
const client = net.connect(port);
52+
client.write(
53+
`GET /1 HTTP/1.1\r\nHost: localhost:${port}\r\n\r\n` +
54+
`GET /2 HTTP/1.1\r\nHost: localhost:${port}\r\n\r\n`,
55+
);
56+
client.resume();
57+
client.on('error', () => {});
58+
}));

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.