Skip to content

Commit

Permalink
http: set IncomingMessage.destroyed
Browse files Browse the repository at this point in the history
IncomingMessage is a Readable stream and should properly
set the destroyed property.

PR-URL: nodejs#33131
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
  • Loading branch information
ronag authored and addaleax committed May 9, 2020
1 parent 96faea1 commit 28e6626
Show file tree
Hide file tree
Showing 10 changed files with 94 additions and 4 deletions.
4 changes: 4 additions & 0 deletions lib/_http_client.js
Original file line number Diff line number Diff line change
Expand Up @@ -422,10 +422,12 @@ function socketCloseListener() {
req.emit('close');
if (!res.aborted && res.readable) {
res.on('end', function() {
this.destroyed = true;
this.emit('close');
});
res.push(null);
} else {
res.destroyed = true;
res.emit('close');
}
} else {
Expand Down Expand Up @@ -539,6 +541,7 @@ function socketOnData(d) {
socket.readableFlowing = null;

req.emit(eventName, res, socket, bodyHead);
req.destroyed = true;
req.emit('close');
} else {
// Requested Upgrade or used CONNECT method, but have no handler.
Expand Down Expand Up @@ -710,6 +713,7 @@ function requestOnPrefinish() {
function emitFreeNT(req) {
req.emit('close');
if (req.res) {
req.res.destroyed = true;
req.res.emit('close');
}

Expand Down
2 changes: 2 additions & 0 deletions lib/_http_incoming.js
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,8 @@ IncomingMessage.prototype._read = function _read(n) {
// any messages, before ever calling this. In that case, just skip
// it, since something else is destroying this connection anyway.
IncomingMessage.prototype.destroy = function destroy(error) {
// TODO(ronag): Implement in terms of _destroy
this.destroyed = true;
if (this.socket)
this.socket.destroy(error);
};
Expand Down
9 changes: 8 additions & 1 deletion lib/_http_server.js
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,10 @@ function onServerResponseClose() {
// Ergo, we need to deal with stale 'close' events and handle the case
// where the ServerResponse object has already been deconstructed.
// Fortunately, that requires only a single if check. :-)
if (this._httpMessage) this._httpMessage.emit('close');
if (this._httpMessage) {
this._httpMessage.destroyed = true;
this._httpMessage.emit('close');
}
}

ServerResponse.prototype.assignSocket = function assignSocket(socket) {
Expand Down Expand Up @@ -534,6 +537,7 @@ function abortIncoming(incoming) {
while (incoming.length) {
const req = incoming.shift();
req.aborted = true;
req.destroyed = true;
req.emit('aborted');
req.emit('close');
}
Expand Down Expand Up @@ -660,11 +664,13 @@ function clearIncoming(req) {
if (parser && parser.incoming === req) {
if (req.readableEnded) {
parser.incoming = null;
req.destroyed = true;
req.emit('close');
} else {
req.on('end', clearIncoming);
}
} else {
req.destroyed = true;
req.emit('close');
}
}
Expand Down Expand Up @@ -708,6 +714,7 @@ function resOnFinish(req, res, socket, state, server) {
}

function emitCloseNT(self) {
self.destroyed = true;
self.emit('close');
}

Expand Down
41 changes: 41 additions & 0 deletions test/parallel/test-http-client-res-destroyed.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
'use strict';
const common = require('../common');
const assert = require('assert');
const http = require('http');

{
const server = http.createServer(common.mustCall((req, res) => {
res.end('asd');
}));

server.listen(0, common.mustCall(() => {
http.get({
port: server.address().port
}, common.mustCall((res) => {
assert.strictEqual(res.destroyed, false);
res.destroy();
assert.strictEqual(res.destroyed, true);
res.on('close', common.mustCall(() => {
server.close();
}));
}));
}));
}

{
const server = http.createServer(common.mustCall((req, res) => {
res.end('asd');
}));

server.listen(0, common.mustCall(() => {
http.get({
port: server.address().port
}, common.mustCall((res) => {
assert.strictEqual(res.destroyed, false);
res.on('close', common.mustCall(() => {
assert.strictEqual(res.destroyed, true);
server.close();
})).resume();
}));
}));
}
5 changes: 4 additions & 1 deletion test/parallel/test-http-connect-req-res.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,10 @@ server.listen(0, common.mustCall(function() {
path: 'example.com:443'
}, common.mustNotCall());

req.on('close', common.mustCall());
assert.strictEqual(req.destroyed, false);
req.on('close', common.mustCall(() => {
assert.strictEqual(req.destroyed, true);
}));

req.on('connect', common.mustCall(function(res, socket, firstBodyChunk) {
console.error('Client got CONNECT request');
Expand Down
5 changes: 4 additions & 1 deletion test/parallel/test-http-connect.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,10 @@ server.listen(0, common.mustCall(() => {
assert.strictEqual(socket._httpMessage, req);
}));

req.on('close', common.mustCall());
assert.strictEqual(req.destroyed, false);
req.on('close', common.mustCall(() => {
assert.strictEqual(req.destroyed, true);
}));

req.on('connect', common.mustCall((res, socket, firstBodyChunk) => {
// Make sure this request got removed from the pool.
Expand Down
9 changes: 8 additions & 1 deletion test/parallel/test-http-pause-resume-one-end.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
'use strict';
const common = require('../common');
const http = require('http');
const assert = require('assert');

const server = http.Server(function(req, res) {
res.writeHead(200, { 'Content-Type': 'text/plain' });
Expand All @@ -43,6 +44,12 @@ server.listen(0, common.mustCall(function() {
});
}));

res.on('end', common.mustCall());
res.on('end', common.mustCall(() => {
assert.strictEqual(res.destroyed, false);
}));
assert.strictEqual(res.destroyed, false);
res.on('close', common.mustCall(() => {
assert.strictEqual(res.destroyed, true);
}));
}));
}));
12 changes: 12 additions & 0 deletions test/parallel/test-http-req-res-close.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,25 @@ const server = http.Server(common.mustCall((req, res) => {
let resClosed = false;

res.end();
let resFinished = false;
res.on('finish', common.mustCall(() => {
resFinished = true;
assert.strictEqual(resClosed, false);
assert.strictEqual(res.destroyed, false);
assert.strictEqual(resClosed, false);
}));
assert.strictEqual(req.destroyed, false);
res.on('close', common.mustCall(() => {
resClosed = true;
assert.strictEqual(resFinished, true);
assert.strictEqual(res.destroyed, true);
}));
assert.strictEqual(req.destroyed, false);
req.on('end', common.mustCall(() => {
assert.strictEqual(req.destroyed, false);
}));
req.on('close', common.mustCall(() => {
assert.strictEqual(req.destroyed, true);
assert.strictEqual(req._readableState.ended, true);
}));
res.socket.on('close', () => server.close());
Expand Down
8 changes: 8 additions & 0 deletions test/parallel/test-http-response-close.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
'use strict';
const common = require('../common');
const http = require('http');
const assert = require('assert');

{
const server = http.createServer(
Expand All @@ -39,7 +40,9 @@ const http = require('http');
res.on('data', common.mustCall(() => {
res.destroy();
}));
assert.strictEqual(res.destroyed, false);
res.on('close', common.mustCall(() => {
assert.strictEqual(res.destroyed, true);
server.close();
}));
})
Expand All @@ -61,7 +64,12 @@ const http = require('http');
http.get(
{ port: server.address().port },
common.mustCall((res) => {
assert.strictEqual(res.destroyed, false);
res.on('end', common.mustCall(() => {
assert.strictEqual(res.destroyed, false);
}));
res.on('close', common.mustCall(() => {
assert.strictEqual(res.destroyed, true);
server.close();
}));
res.resume();
Expand Down
3 changes: 3 additions & 0 deletions test/parallel/test-http-server-stale-close.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
require('../common');
const http = require('http');
const fork = require('child_process').fork;
const assert = require('assert');

if (process.env.NODE_TEST_FORK_PORT) {
const req = http.request({
Expand All @@ -37,7 +38,9 @@ if (process.env.NODE_TEST_FORK_PORT) {
const server = http.createServer((req, res) => {
res.writeHead(200, { 'Content-Length': '42' });
req.pipe(res);
assert.strictEqual(req.destroyed, false);
req.on('close', () => {
assert.strictEqual(req.destroyed, true);
server.close();
res.end();
});
Expand Down

0 comments on commit 28e6626

Please sign in to comment.