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: make request.abort() destroy the socket #10818

Merged
merged 1 commit into from
Jan 28, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion lib/_http_client.js
Original file line number Diff line number Diff line change
Expand Up @@ -643,7 +643,11 @@ ClientRequest.prototype.onSocket = function onSocket(socket) {
function onSocketNT(req, socket) {
if (req.aborted) {
// If we were aborted while waiting for a socket, skip the whole thing.
socket.emit('free');
if (req.socketPath || !req.agent) {
socket.destroy();
} else {
socket.emit('free');
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not even sure if it's appropriate to emit 'free' and skip destroying the socket. ClientRequest#abort() always calls socket.destroy() when there is one.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bnoordhuis I think this is for keep-alive sockets?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bnoordhuis I guess you were referring to

node/lib/_http_client.js

Lines 296 to 299 in 77be180

if (this.socket) {
// in-progress
this.socket.destroy();
}
. I'm not sure either.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is when socket.emit('free'); was added: d0c7d93#diff-e3bc37430eb078ccbafe3aa3b570c91a, previously the socket was destroyed in all cases.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should always destroy(), I do not see a way for this socket to be reused even if there is an agent.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. There's really no safe way to reuse it. destroy() is the safest thing to do

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a test which should clarify why socket.emit('free') is used. It's for queued requests which are already aborted.

Copy link
Member Author

@lpinca lpinca Jan 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See f52c874b737ccb7e92d640b31e02c6efb3acd22c.

} else {
tickOnSocket(req, socket);
}
Expand Down
36 changes: 36 additions & 0 deletions test/parallel/test-http-abort-queued-2.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');

let socketsCreated = 0;

class Agent extends http.Agent {
createConnection(options, oncreate) {
const socket = super.createConnection(options, oncreate);
socketsCreated++;
return socket;
}
}

const server = http.createServer((req, res) => res.end());

server.listen(0, common.mustCall(() => {
const port = server.address().port;
const agent = new Agent({
keepAlive: true,
maxSockets: 1
});

http.get({agent, port}, (res) => res.resume());

const req = http.get({agent, port}, common.fail);
req.abort();

http.get({agent, port}, common.mustCall((res) => {
res.resume();
assert.strictEqual(socketsCreated, 1);
agent.destroy();
server.close();
}));
}));
19 changes: 19 additions & 0 deletions test/parallel/test-http-client-abort-no-agent.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
'use strict';
const common = require('../common');
const http = require('http');
const net = require('net');

const server = http.createServer(common.fail);

server.listen(0, common.mustCall(() => {
const req = http.get({
createConnection(options, oncreate) {
const socket = net.createConnection(options, oncreate);
socket.once('close', () => server.close());
return socket;
},
port: server.address().port
});

req.abort();
}));
24 changes: 24 additions & 0 deletions test/parallel/test-http-client-abort-unix-socket.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
'use strict';
const common = require('../common');
const http = require('http');

const server = http.createServer(common.fail);

class Agent extends http.Agent {
createConnection(options, oncreate) {
const socket = super.createConnection(options, oncreate);
socket.once('close', () => server.close());
return socket;
}
}

common.refreshTmpDir();

server.listen(common.PIPE, common.mustCall(() => {
const req = http.get({
agent: new Agent(),
socketPath: common.PIPE
});

req.abort();
}));