Skip to content

Commit

Permalink
http: use 'connect' event only if socket is connecting
Browse files Browse the repository at this point in the history
Fixes a bug that prevented `ClientRequest.prototype.setTimeout()` from
working properly when the socket was reused for multiple requests.

Fixes: nodejs#16716
Refs: nodejs#8895
  • Loading branch information
lpinca committed Nov 6, 2017
1 parent 92f8663 commit 407389d
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 2 deletions.
8 changes: 6 additions & 2 deletions lib/_http_client.js
Original file line number Diff line number Diff line change
Expand Up @@ -728,9 +728,13 @@ ClientRequest.prototype.setTimeout = function setTimeout(msecs, callback) {
}

this.once('socket', function(sock) {
sock.once('connect', function() {
if (sock.connecting) {
sock.once('connect', function() {
sock.setTimeout(msecs, emitTimeout);
});
} else {
sock.setTimeout(msecs, emitTimeout);
});
}
});

return this;
Expand Down
42 changes: 42 additions & 0 deletions test/parallel/test-http-client-timeout-connect-listener.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
'use strict';
const common = require('../common');

// This test ensures that `ClientRequest.prototype.setTimeout()` does
// not add a listener for the `'connect'` event to socket if the
// socket is already connected.

const assert = require('assert');
const http = require('http');

// Maximum allowed value for timeouts.
const timeout = 2 ** 31 - 1;

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

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

doRequest(options, common.mustCall(() => {
const req = doRequest(options, common.mustCall(() => {
agent.destroy();
server.close();
}));

req.on('socket', common.mustCall((socket) => {
assert.strictEqual(socket.listenerCount('connect'), 0);
}));
}));
}));

function doRequest(options, callback) {
const req = http.get(options, (res) => {
res.on('end', callback);
res.resume();
});

req.setTimeout(timeout);
return req;
}

0 comments on commit 407389d

Please sign in to comment.