diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index 24aae1caca69d3..fa9e38ae7481e9 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -1139,12 +1139,12 @@ OutgoingMessage.prototype._flush = function _flush() { if (socket?.writable) { // There might be remaining data in this.output; write it out - const ret = this._flushOutput(socket); + this._flushOutput(socket); if (this.finished) { // This is a queue to the server or client to bring in the next this. this._finish(); - } else if (ret && this[kNeedDrain]) { + } else if (this[kNeedDrain] && this.writableLength === 0) { this[kNeedDrain] = false; this.emit('drain'); } diff --git a/lib/_http_server.js b/lib/_http_server.js index 4da10dd1edf133..24ca21606316ad 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -817,7 +817,13 @@ function socketOnDrain(socket, state) { } const msg = socket._httpMessage; - if (msg && !msg.finished && msg[kNeedDrain]) { + // Only emit 'drain' once the message has no data pending anywhere, so that + // msg.writableLength === 0 when the event fires. socketOnDrain is called + // synchronously from updateOutgoingData during _flushOutput, at which point + // the bytes we just handed to the socket (or the stale outputSize) mean + // the message is not actually drained yet - we wait for the socket's + // own 'drain' event instead. + if (msg && !msg.finished && msg[kNeedDrain] && msg.writableLength === 0) { msg[kNeedDrain] = false; msg.emit('drain'); } diff --git a/test/parallel/test-http-outgoing-drain-writable-length.js b/test/parallel/test-http-outgoing-drain-writable-length.js new file mode 100644 index 00000000000000..39bb0a9447676d --- /dev/null +++ b/test/parallel/test-http-outgoing-drain-writable-length.js @@ -0,0 +1,58 @@ +'use strict'; +// Regression test: when a pipelined ServerResponse (whose writes were +// buffered in outputData while the socket belonged to a previous response) +// is finally assigned its socket and flushed, 'drain' must not be emitted +// until the socket's own buffer has actually drained. Previously, +// socketOnDrain was called synchronously from _flushOutput via _onPendingData +// and emitted 'drain' even though the bytes we just wrote were still sitting +// in the socket's writable buffer, so res.writableLength was non-zero. + +const common = require('../common'); +const http = require('http'); +const net = require('net'); +const assert = require('assert'); + +let step = 0; + +const server = http.createServer(common.mustCall((req, res) => { + step++; + + if (step === 1) { + // Keep the first response open briefly so the second is queued with + // res.socket === null. + res.writeHead(200, { 'Content-Type': 'text/plain' }); + setTimeout(() => res.end('ok'), 50); + return; + } + + // Second (pipelined) response - queued in state.outgoing, no socket yet. + assert.strictEqual(res.socket, null); + + res.writeHead(200, { 'Content-Type': 'text/plain' }); + + // Write past the response's highWaterMark so res.write() returns false + // and kNeedDrain is set. Data is buffered in outputData. + const chunk = Buffer.alloc(16 * 1024, 'x'); + while (res.write(chunk)); + assert.strictEqual(res.writableNeedDrain, true); + + res.on('drain', common.mustCall(() => { + assert.strictEqual( + res.writableLength, 0, + `'drain' fired with writableLength=${res.writableLength}`, + ); + res.end(); + server.close(); + })); +}, 2)); + +server.listen(0, common.mustCall(function() { + const port = this.address().port; + const client = net.connect(port); + client.write( + `GET /1 HTTP/1.1\r\nHost: localhost:${port}\r\n\r\n` + + `GET /2 HTTP/1.1\r\nHost: localhost:${port}\r\n\r\n`, + ); + client.resume(); + client.on('error', () => {}); +}));