From 174fa33ec941b44325eb275710f15b639f7c7522 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sat, 26 Dec 2020 11:11:21 +0100 Subject: [PATCH 1/2] http: don't cork noop .end() Calling .end() a second time should be a noop and not leave the socket corked. Fixes: https://github.com/nodejs/node/issues/36620 --- lib/_http_outgoing.js | 13 +++++++++---- test/parallel/test-http-outgoing-end-multiple.js | 3 +++ 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index 8fb64ab82efceb..a9fee3155f761d 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -810,10 +810,6 @@ OutgoingMessage.prototype.end = function end(chunk, encoding, callback) { encoding = null; } - if (this.socket) { - this.socket.cork(); - } - if (chunk) { if (this.finished) { onError(this, @@ -821,6 +817,11 @@ OutgoingMessage.prototype.end = function end(chunk, encoding, callback) { typeof callback !== 'function' ? nop : callback); return this; } + + if (this.socket) { + this.socket.cork(); + } + write_(this, chunk, encoding, null, true); } else if (this.finished) { if (typeof callback === 'function') { @@ -832,6 +833,10 @@ OutgoingMessage.prototype.end = function end(chunk, encoding, callback) { } return this; } else if (!this._header) { + if (this.socket) { + this.socket.cork(); + } + this._contentLength = 0; this._implicitHeader(); } diff --git a/test/parallel/test-http-outgoing-end-multiple.js b/test/parallel/test-http-outgoing-end-multiple.js index ed42c913375e84..696443f9390cd0 100644 --- a/test/parallel/test-http-outgoing-end-multiple.js +++ b/test/parallel/test-http-outgoing-end-multiple.js @@ -9,10 +9,13 @@ const onWriteAfterEndError = common.mustCall((err) => { const server = http.createServer(common.mustCall(function(req, res) { res.end('testing ended state', common.mustCall()); + assert.strictEqual(res.writableCorked, 0); res.end(common.mustCall((err) => { assert.strictEqual(err.code, 'ERR_STREAM_ALREADY_FINISHED'); })); + assert.strictEqual(res.writableCorked, 0); res.end('end', onWriteAfterEndError); + assert.strictEqual(res.writableCorked, 0); res.on('error', onWriteAfterEndError); res.on('finish', common.mustCall(() => { res.end(common.mustCall((err) => { From 340a0169dc00d6e0a374e7ea82397253ddee11cd Mon Sep 17 00:00:00 2001 From: Dimitris Halatsis Date: Sun, 10 Jan 2021 18:35:12 +0200 Subject: [PATCH 2/2] test: http complete response after socket double end --- test/parallel/test-http-outgoing-end-cork.js | 95 ++++++++++++++++++++ 1 file changed, 95 insertions(+) create mode 100644 test/parallel/test-http-outgoing-end-cork.js diff --git a/test/parallel/test-http-outgoing-end-cork.js b/test/parallel/test-http-outgoing-end-cork.js new file mode 100644 index 00000000000000..6a217238c447c4 --- /dev/null +++ b/test/parallel/test-http-outgoing-end-cork.js @@ -0,0 +1,95 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const http = require('http'); + +const REQ_TIMEOUT = 500; // Set max ms of request time before abort + +// Set total allowed test timeout to avoid infinite loop +// that will hang test suite +const TOTAL_TEST_TIMEOUT = 1000; + +// Placeholder for sockets handled, to make sure that we +// will reach a socket re-use case. +const handledSockets = new Set(); + +let metReusedSocket = false; // Flag for request loop termination. + +const doubleEndResponse = (res) => { + // First end the request while sending some normal data + res.end('regular end of request', 'utf8', common.mustCall()); + // Make sure the response socket is uncorked after first call of end + assert.strictEqual(res.writableCorked, 0); + res.end(); // Double end the response to prep for next socket re-use. +}; + +const sendDrainNeedingData = (res) => { + // Send data to socket more than the high watermark so that + // it definitely needs drain + const highWaterMark = res.socket.writableHighWaterMark; + const bufferToSend = Buffer.alloc(highWaterMark + 100); + const ret = res.write(bufferToSend); // Write the request data. + // Make sure that we had back pressure on response stream. + assert.strictEqual(ret, false); + res.once('drain', () => res.end()); // End on drain. +}; + +const server = http.createServer((req, res) => { + const { socket: responseSocket } = res; + if (handledSockets.has(responseSocket)) { // re-used socket, send big data! + metReusedSocket = true; // stop request loop + console.debug('FOUND REUSED SOCKET!'); + sendDrainNeedingData(res); + } else { // not used again + // add to make sure we recognise it when we meet socket again + handledSockets.add(responseSocket); + doubleEndResponse(res); + } +}); + +server.listen(0); // Start the server on a random port. + +const sendRequest = (agent) => new Promise((resolve, reject) => { + const timeout = setTimeout(common.mustNotCall(() => { + reject(new Error('Request timed out')); + }), REQ_TIMEOUT); + http.get({ + port: server.address().port, + path: '/', + agent + }, common.mustCall((res) => { + const resData = []; + res.on('data', (data) => resData.push(data)); + res.on('end', common.mustCall(() => { + const totalData = resData.reduce((total, elem) => total + elem.length, 0); + clearTimeout(timeout); // Cancel rejection timeout. + resolve(totalData); // fulfill promise + })); + })); +}); + +server.once('listening', async () => { + const testTimeout = setTimeout(common.mustNotCall(() => { + console.error('Test running for a while but could not met re-used socket'); + process.exit(1); + }), TOTAL_TEST_TIMEOUT); + // Explicitly start agent to force socket reuse. + const agent = new http.Agent({ keepAlive: true }); + // Start the request loop + let reqNo = 0; + while (!metReusedSocket) { + try { + console.log(`Sending req no ${++reqNo}`); + const totalData = await sendRequest(agent); + console.log(`${totalData} bytes were received for request ${reqNo}`); + } catch (err) { + console.error(err); + process.exit(1); + } + } + // Successfully tested conditions and ended loop + clearTimeout(testTimeout); + console.log('Closing server'); + agent.destroy(); + server.close(); +});