Skip to content

Commit

Permalink
http: don't destroy completed request
Browse files Browse the repository at this point in the history
Calling destroy() on a completed ClientRequest, i.e.
once 'close' will be emitted should be a noop. Also
before emitting 'close' destroyed === true.

Fixes: #32851

PR-URL: #33120
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
  • Loading branch information
ronag authored and codebytere committed May 11, 2020
1 parent e1e57a4 commit cc5c8e0
Show file tree
Hide file tree
Showing 10 changed files with 60 additions and 5 deletions.
5 changes: 4 additions & 1 deletion lib/_http_client.js
Original file line number Diff line number Diff line change
Expand Up @@ -413,6 +413,8 @@ function socketCloseListener() {
// the `socketOnData`.
const parser = socket.parser;
const res = req.res;

req.destroyed = true;
if (res) {
// Socket closed before we emitted 'end' below.
if (!res.complete) {
Expand Down Expand Up @@ -667,7 +669,9 @@ function responseKeepAlive(req) {
// handlers have a chance to run.
defaultTriggerAsyncIdScope(asyncId, process.nextTick, emitFreeNT, req);

req.destroyed = true;
if (req.res) {
req.res.destroyed = true;
// Detach socket from IncomingMessage to avoid destroying the freed
// socket in IncomingMessage.destroy().
req.res.socket = null;
Expand Down Expand Up @@ -713,7 +717,6 @@ 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 test/parallel/test-http-client-agent-abort-close-event.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
'use strict';
const common = require('../common');
const assert = require('assert');
const http = require('http');

const server = http.createServer(common.mustNotCall());
Expand All @@ -16,6 +17,7 @@ server.listen(0, common.mustCall(() => {
.on('socket', common.mustNotCall())
.on('response', common.mustNotCall())
.on('close', common.mustCall(() => {
assert.strictEqual(req.destroyed, true);
server.close();
keepAliveAgent.destroy();
}))
Expand Down
2 changes: 2 additions & 0 deletions test/parallel/test-http-client-agent-end-close-event.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
'use strict';
const common = require('../common');
const assert = require('assert');
const http = require('http');

const server = http.createServer(common.mustCall((req, res) => {
Expand All @@ -18,6 +19,7 @@ server.listen(0, common.mustCall(() => {
.on('response', common.mustCall((res) => {
res
.on('close', common.mustCall(() => {
assert.strictEqual(req.destroyed, true);
server.close();
keepAliveAgent.destroy();
}))
Expand Down
1 change: 1 addition & 0 deletions test/parallel/test-http-client-close-event.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ server.listen(0, common.mustCall(() => {
}));

req.on('close', common.mustCall(() => {
assert.strictEqual(req.destroyed, true);
assert.strictEqual(errorEmitted, true);
server.close();
}));
Expand Down
6 changes: 4 additions & 2 deletions test/parallel/test-http-client-override-global-agent.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ function makeRequest() {
const req = http.get({
port: server.address().port
});
req.on('close', () =>
server.close());
req.on('close', () => {
assert.strictEqual(req.destroyed, true);
server.close();
});
}
1 change: 1 addition & 0 deletions test/parallel/test-http-client-set-timeout.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ server.listen(0, mustCall(() => {
}));

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

Expand Down
6 changes: 5 additions & 1 deletion test/parallel/test-http-client-timeout-event.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

'use strict';
const common = require('../common');
const assert = require('assert');
const http = require('http');

const options = {
Expand All @@ -38,7 +39,10 @@ server.listen(0, options.host, function() {
req.on('error', function() {
// This space is intentionally left blank
});
req.on('close', common.mustCall(() => server.close()));
req.on('close', common.mustCall(() => {
assert.strictEqual(req.destroyed, true);
server.close();
}));

req.setTimeout(1);
req.on('timeout', common.mustCall(() => {
Expand Down
5 changes: 4 additions & 1 deletion test/parallel/test-http-client-timeout-option.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,10 @@ server.listen(0, options.host, function() {
req.on('error', function() {
// This space is intentionally left blank
});
req.on('close', common.mustCall(() => server.close()));
req.on('close', common.mustCall(() => {
assert.strictEqual(req.destroyed, true);
server.close();
}));

let timeout_events = 0;
req.on('timeout', common.mustCall(() => timeout_events += 1));
Expand Down
1 change: 1 addition & 0 deletions test/parallel/test-http-client-timeout.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ server.listen(0, options.host, function() {
// This space intentionally left blank
});
req.on('close', function() {
assert.strictEqual(req.destroyed, true);
server.close();
});
function destroy() {
Expand Down
36 changes: 36 additions & 0 deletions test/parallel/test-http-keepalive-free.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
'use strict';

const common = require('../common');
const assert = require('assert');

const http = require('http');

for (const method of ['abort', 'destroy']) {
const server = http.createServer(common.mustCall((req, res) => {
res.end(req.url);
}));
server.listen(0, common.mustCall(() => {
const agent = http.Agent({ keepAlive: true });

const req = http
.request({
port: server.address().port,
agent
})
.on('socket', common.mustCall((socket) => {
socket.on('free', common.mustCall());
}))
.on('response', common.mustCall((res) => {
assert.strictEqual(req.destroyed, false);
res.on('end', () => {
assert.strictEqual(req.destroyed, true);
req[method]();
assert.strictEqual(req.socket.destroyed, false);
agent.destroy();
server.close();
}).resume();
}))
.end();
assert.strictEqual(req.destroyed, false);
}));
}

0 comments on commit cc5c8e0

Please sign in to comment.