Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http: don't cork noop .end() #36633

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 9 additions & 4 deletions lib/_http_outgoing.js
Expand Up @@ -810,17 +810,18 @@ OutgoingMessage.prototype.end = function end(chunk, encoding, callback) {
encoding = null;
}

if (this.socket) {
this.socket.cork();
}
ronag marked this conversation as resolved.
Show resolved Hide resolved

if (chunk) {
if (this.finished) {
onError(this,
new ERR_STREAM_WRITE_AFTER_END(),
typeof callback !== 'function' ? nop : callback);
return this;
}

if (this.socket) {
this.socket.cork();
ronag marked this conversation as resolved.
Show resolved Hide resolved
}

write_(this, chunk, encoding, null, true);
} else if (this.finished) {
if (typeof callback === 'function') {
Expand All @@ -832,6 +833,10 @@ OutgoingMessage.prototype.end = function end(chunk, encoding, callback) {
}
return this;
} else if (!this._header) {
if (this.socket) {
ronag marked this conversation as resolved.
Show resolved Hide resolved
this.socket.cork();
}

this._contentLength = 0;
this._implicitHeader();
}
Expand Down
95 changes: 95 additions & 0 deletions 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();
});
3 changes: 3 additions & 0 deletions test/parallel/test-http-outgoing-end-multiple.js
Expand Up @@ -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) => {
Expand Down