From fda2f0709fd09d4f5bff05a10754b8b3ecd127ee Mon Sep 17 00:00:00 2001 From: David Evans Date: Sun, 21 Jun 2026 01:36:14 +0100 Subject: [PATCH 1/2] http: fix drain event with cork/uncork When using cork() and uncork() with ServerResponse, the drain event was not reliably emitted after uncorking. This occurred because the uncork() method did not check if a drain was pending (kNeedDrain flag) after flushing the chunked buffer. This fix ensures that when uncork() successfully flushes buffered data and a drain was needed, the drain event is emitted immediately. This commit is a copy of PR #60437 (abandoned) with minor linting fixes. Fixes: nodejs#60432 Signed-off-by: David Evans --- lib/_http_outgoing.js | 8 ++- .../parallel/test-http-response-drain-cork.js | 67 +++++++++++++++++++ 2 files changed, 74 insertions(+), 1 deletion(-) create mode 100644 test/parallel/test-http-response-drain-cork.js diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index 4498ee72fe48d8..ef905b7274f374 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -315,7 +315,7 @@ OutgoingMessage.prototype.uncork = function uncork() { callbacks.push(buf[n + 2]); } } - this._send(crlf_buf, null, callbacks.length ? (err) => { + const ret = this._send(crlf_buf, null, callbacks.length ? (err) => { for (const callback of callbacks) { callback(err); } @@ -323,6 +323,12 @@ OutgoingMessage.prototype.uncork = function uncork() { this[kChunkedBuffer].length = 0; this[kChunkedLength] = 0; + + // If we successfully flushed and had pending drain, emit it + if (ret && this[kNeedDrain]) { + this[kNeedDrain] = false; + this.emit('drain'); + } }; OutgoingMessage.prototype.setTimeout = function setTimeout(msecs, callback) { diff --git a/test/parallel/test-http-response-drain-cork.js b/test/parallel/test-http-response-drain-cork.js new file mode 100644 index 00000000000000..29e312f309bd70 --- /dev/null +++ b/test/parallel/test-http-response-drain-cork.js @@ -0,0 +1,67 @@ +'use strict'; +const common = require('../common'); +const http = require('http'); +const assert = require('assert'); + +// Test that drain event is emitted correctly when using cork/uncork +// with ServerResponse and the write buffer is full + +const server = http.createServer(common.mustCall(async (req, res) => { + res.cork(); + + // Write small amount - won't need drain + assert.strictEqual(res.write('1'.repeat(100)), true); + + // Write large amount that should require drain + assert.strictEqual(res.write('2'.repeat(1000000)), false); + + // Verify writableNeedDrain is set + assert.strictEqual(res.writableNeedDrain, true); + + // Wait for drain event after uncorking + const drainPromise = new Promise((resolve) => { + res.once('drain', common.mustCall(() => { + // After drain, writableNeedDrain should be false + assert.strictEqual(res.writableNeedDrain, false); + resolve(); + })); + }); + + // Uncork should trigger drain if needed + res.uncork(); + + await drainPromise; + + // Cork again for next write + res.cork(); + + // Write more data + res.write('3'.repeat(100)); + + // Final uncork and end + res.uncork(); + res.end(); +})); + +server.listen(0, common.mustCall(() => { + http.get({ + port: server.address().port, + }, common.mustCall((res) => { + let data = ''; + res.setEncoding('utf8'); + + res.on('data', (chunk) => { + data += chunk; + }); + + res.on('end', common.mustCall(() => { + // Verify we got all the data + assert.strictEqual(data.length, 100 + 1000000 + 100); + assert.strictEqual(data.substring(0, 100), '1'.repeat(100)); + assert.strictEqual(data.substring(100, 1000100), '2'.repeat(1000000)); + assert.strictEqual(data.substring(1000100), '3'.repeat(100)); + + server.close(); + })); + })); +})); From a8421efc1d0a3cb5fd83bf88197ce3c404132b1d Mon Sep 17 00:00:00 2001 From: David Evans Date: Sun, 21 Jun 2026 13:55:20 +0100 Subject: [PATCH 2/2] http: Simplify drain-on-uncork logic and test Check internal state when deciding whether to send a drain event on uncork() - instead of relying on the return value of _send - as suggested in the PR comments. The test is updated to set an explicit high water mark for better reliability, and since this means we can use a much lower value, the size of data in the test has been reduced significantly. --- lib/_http_outgoing.js | 6 ++-- .../parallel/test-http-response-drain-cork.js | 35 +++++++++---------- 2 files changed, 19 insertions(+), 22 deletions(-) diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index ef905b7274f374..7694fe4b5b3f3e 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -315,7 +315,7 @@ OutgoingMessage.prototype.uncork = function uncork() { callbacks.push(buf[n + 2]); } } - const ret = this._send(crlf_buf, null, callbacks.length ? (err) => { + this._send(crlf_buf, null, callbacks.length ? (err) => { for (const callback of callbacks) { callback(err); } @@ -324,8 +324,8 @@ OutgoingMessage.prototype.uncork = function uncork() { this[kChunkedBuffer].length = 0; this[kChunkedLength] = 0; - // If we successfully flushed and had pending drain, emit it - if (ret && this[kNeedDrain]) { + // If we had a pending drain and flushed all data, emit the drain event. + if (this[kNeedDrain] && this.writableLength === 0) { this[kNeedDrain] = false; this.emit('drain'); } diff --git a/test/parallel/test-http-response-drain-cork.js b/test/parallel/test-http-response-drain-cork.js index 29e312f309bd70..18678748b9c2a4 100644 --- a/test/parallel/test-http-response-drain-cork.js +++ b/test/parallel/test-http-response-drain-cork.js @@ -10,10 +10,10 @@ const server = http.createServer(common.mustCall(async (req, res) => { res.cork(); // Write small amount - won't need drain - assert.strictEqual(res.write('1'.repeat(100)), true); + assert.strictEqual(res.write('1'.repeat(10)), true); - // Write large amount that should require drain - assert.strictEqual(res.write('2'.repeat(1000000)), false); + // Write enough to exceed highWaterMark (set in 'connection' listener) + assert.strictEqual(res.write('2'.repeat(1000)), false); // Verify writableNeedDrain is set assert.strictEqual(res.writableNeedDrain, true); @@ -27,24 +27,22 @@ const server = http.createServer(common.mustCall(async (req, res) => { })); }); - // Uncork should trigger drain if needed + // Uncork should trigger drain res.uncork(); - await drainPromise; - // Cork again for next write - res.cork(); - - // Write more data - res.write('3'.repeat(100)); - - // Final uncork and end - res.uncork(); res.end(); })); -server.listen(0, common.mustCall(() => { +server.on('connection', common.mustCall((socket) => { + // Set high water mark large enough to cover HTTP overhead + first + // small content batch, but not enough to cover second batch. + socket._writableState.highWaterMark = 1000; +})); + +server.listen(0, common.localhostIPv4, common.mustCall(() => { http.get({ + host: common.localhostIPv4, port: server.address().port, }, common.mustCall((res) => { let data = ''; @@ -56,12 +54,11 @@ server.listen(0, common.mustCall(() => { res.on('end', common.mustCall(() => { // Verify we got all the data - assert.strictEqual(data.length, 100 + 1000000 + 100); - assert.strictEqual(data.substring(0, 100), '1'.repeat(100)); - assert.strictEqual(data.substring(100, 1000100), '2'.repeat(1000000)); - assert.strictEqual(data.substring(1000100), '3'.repeat(100)); + assert.strictEqual(data.length, 10 + 1000); + assert.strictEqual(data.substring(0, 10), '1'.repeat(10)); + assert.strictEqual(data.substring(10), '2'.repeat(1000)); - server.close(); + server.close(common.mustCall()); })); })); }));