Skip to content

Commit

Permalink
http: fix request with option timeout and agent
Browse files Browse the repository at this point in the history
When request with both timeout and agent, timeout not
work. This patch will fix it, socket timeout will set
to request timeout before socket is connected, and
socket timeout will reset to agent timeout after
response end.

Fixes: #21185

PR-URL: #21204
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
  • Loading branch information
killagu authored and targos committed Jul 14, 2018
1 parent 3d93273 commit bba500d
Show file tree
Hide file tree
Showing 6 changed files with 169 additions and 42 deletions.
2 changes: 2 additions & 0 deletions doc/api/http.md
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,8 @@ added: v0.3.4
* `maxFreeSockets` {number} Maximum number of sockets to leave open
in a free state. Only relevant if `keepAlive` is set to `true`.
**Default:** `256`.
* `timeout` {number} Socket timeout in milliseconds.
This will set the timeout after the socket is connected.

The default [`http.globalAgent`][] that is used by [`http.request()`][] has all
of these values set to their respective defaults.
Expand Down
33 changes: 26 additions & 7 deletions lib/_http_agent.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,8 @@ function Agent(options) {

if (socket.writable &&
this.requests[name] && this.requests[name].length) {
this.requests[name].shift().onSocket(socket);
const req = this.requests[name].shift();
setRequestSocket(this, req, socket);
if (this.requests[name].length === 0) {
// don't leak
delete this.requests[name];
Expand Down Expand Up @@ -176,12 +177,12 @@ Agent.prototype.addRequest = function addRequest(req, options, port/* legacy */,
delete this.freeSockets[name];

this.reuseSocket(socket, req);
req.onSocket(socket);
setRequestSocket(this, req, socket);
this.sockets[name].push(socket);
} else if (sockLen < this.maxSockets) {
debug('call onSocket', sockLen, freeLen);
// If we are under maxSockets create a new one.
this.createSocket(req, options, handleSocketCreation(req, true));
this.createSocket(req, options, handleSocketCreation(this, req, true));
} else {
debug('wait for socket');
// We are over limit so we'll add it to the queue.
Expand Down Expand Up @@ -305,9 +306,10 @@ Agent.prototype.removeSocket = function removeSocket(s, options) {

if (this.requests[name] && this.requests[name].length) {
debug('removeSocket, have a request, make a socket');
var req = this.requests[name][0];
const req = this.requests[name][0];
// If we have pending requests and a socket gets closed make a new one
this.createSocket(req, options, handleSocketCreation(req, false));
const socketCreationHandler = handleSocketCreation(this, req, false);
this.createSocket(req, options, socketCreationHandler);
}
};

Expand Down Expand Up @@ -337,19 +339,36 @@ Agent.prototype.destroy = function destroy() {
}
};

function handleSocketCreation(request, informRequest) {
function handleSocketCreation(agent, request, informRequest) {
return function handleSocketCreation_Inner(err, socket) {
if (err) {
process.nextTick(emitErrorNT, request, err);
return;
}
if (informRequest)
request.onSocket(socket);
setRequestSocket(agent, request, socket);
else
socket.emit('free');
};
}

function setRequestSocket(agent, req, socket) {
req.onSocket(socket);
const agentTimeout = agent.options.timeout || 0;
if (req.timeout === undefined || req.timeout === agentTimeout) {
return;
}
socket.setTimeout(req.timeout);
// reset timeout after response end
req.once('response', (res) => {
res.once('end', () => {
if (socket.timeout !== agentTimeout) {
socket.setTimeout(agentTimeout);
}
});
});
}

function emitErrorNT(emitter, err) {
emitter.emit('error', err);
}
Expand Down
74 changes: 39 additions & 35 deletions lib/_http_client.js
Original file line number Diff line number Diff line change
Expand Up @@ -639,18 +639,35 @@ function tickOnSocket(req, socket) {
socket.on('end', socketOnEnd);
socket.on('close', socketCloseListener);

if (req.timeout) {
const emitRequestTimeout = () => req.emit('timeout');
socket.once('timeout', emitRequestTimeout);
req.once('response', (res) => {
res.once('end', () => {
socket.removeListener('timeout', emitRequestTimeout);
});
});
if (req.timeout !== undefined) {
listenSocketTimeout(req);
}
req.emit('socket', socket);
}

function listenSocketTimeout(req) {
if (req.timeoutCb) {
return;
}
const emitRequestTimeout = () => req.emit('timeout');
// Set timeoutCb so it will get cleaned up on request end.
req.timeoutCb = emitRequestTimeout;
// Delegate socket timeout event.
if (req.socket) {
req.socket.once('timeout', emitRequestTimeout);
} else {
req.on('socket', (socket) => {
socket.once('timeout', emitRequestTimeout);
});
}
// Remove socket timeout listener after response end.
req.once('response', (res) => {
res.once('end', () => {
req.socket.removeListener('timeout', emitRequestTimeout);
});
});
}

ClientRequest.prototype.onSocket = function onSocket(socket) {
process.nextTick(onSocketNT, this, socket);
};
Expand Down Expand Up @@ -700,42 +717,29 @@ function _deferToConnect(method, arguments_, cb) {
}

ClientRequest.prototype.setTimeout = function setTimeout(msecs, callback) {
listenSocketTimeout(this);
msecs = validateTimerDuration(msecs);
if (callback) this.once('timeout', callback);

const emitTimeout = () => this.emit('timeout');

if (this.socket && this.socket.writable) {
if (this.timeoutCb)
this.socket.setTimeout(0, this.timeoutCb);
this.timeoutCb = emitTimeout;
this.socket.setTimeout(msecs, emitTimeout);
return this;
}

// Set timeoutCb so that it'll get cleaned up on request end
this.timeoutCb = emitTimeout;
if (this.socket) {
var sock = this.socket;
this.socket.once('connect', function() {
sock.setTimeout(msecs, emitTimeout);
});
return this;
setSocketTimeout(this.socket, msecs);
} else {
this.once('socket', (sock) => setSocketTimeout(sock, msecs));
}

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

return this;
};

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

This comment has been minimized.

Copy link
@arcanis

arcanis Jul 19, 2018

Contributor

This code is breaking. I haven't pinpoint the exact conditions, but the Yarn tests are now throwing exception from this location (we use the request module, if that's of any help).

https://circleci.com/gh/yarnpkg/yarn/18312?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

The problem is consistently reproducible, and doesn't occur with Node 10.6.

}
}

ClientRequest.prototype.setNoDelay = function setNoDelay(noDelay) {
this._deferToConnect('setNoDelay', [noDelay]);
};
Expand Down
1 change: 1 addition & 0 deletions lib/net.js
Original file line number Diff line number Diff line change
Expand Up @@ -408,6 +408,7 @@ function writeAfterFIN(chunk, encoding, cb) {
}

Socket.prototype.setTimeout = function(msecs, callback) {
this.timeout = msecs;
// Type checking identical to timers.enroll()
msecs = validateTimerDuration(msecs);

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

// Test that `req.setTimeout` will fired exactly once.

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

const HTTP_CLIENT_TIMEOUT = 2000;

const options = {
method: 'GET',
port: undefined,
host: '127.0.0.1',
path: '/',
timeout: HTTP_CLIENT_TIMEOUT,
};

const server = http.createServer(() => {
// Never respond.
});

server.listen(0, options.host, function() {
doRequest();
});

function doRequest() {
options.port = server.address().port;
const req = http.request(options);
req.setTimeout(HTTP_CLIENT_TIMEOUT / 2);
req.on('error', () => {
// This space is intentionally left blank.
});
req.on('close', common.mustCall(() => server.close()));

let timeout_events = 0;
req.on('timeout', common.mustCall(() => {
timeout_events += 1;
}));
req.end();

setTimeout(function() {
req.destroy();
assert.strictEqual(timeout_events, 1);
}, common.platformTimeout(HTTP_CLIENT_TIMEOUT));
}
55 changes: 55 additions & 0 deletions test/parallel/test-http-client-timeout-option-with-agent.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
'use strict';
const common = require('../common');

// Test that when http request uses both timeout and agent,
// timeout will work as expected.

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

const HTTP_AGENT_TIMEOUT = 1000;
const HTTP_CLIENT_TIMEOUT = 3000;

const agent = new http.Agent({ timeout: HTTP_AGENT_TIMEOUT });
const options = {
method: 'GET',
port: undefined,
host: '127.0.0.1',
path: '/',
timeout: HTTP_CLIENT_TIMEOUT,
agent,
};

const server = http.createServer(() => {
// Never respond.
});

server.listen(0, options.host, function() {
doRequest();
});

function doRequest() {
options.port = server.address().port;
const req = http.request(options);
const start = Date.now();
req.on('error', () => {
// This space is intentionally left blank.
});
req.on('close', common.mustCall(() => server.close()));

let timeout_events = 0;
req.on('timeout', common.mustCall(() => {
timeout_events += 1;
const duration = Date.now() - start;
// The timeout event cannot be precisely timed. It will delay
// some number of milliseconds, so test it in second units.
assert.strictEqual(duration / 1000 | 0, HTTP_CLIENT_TIMEOUT / 1000);
}));
req.end();

setTimeout(function() {
req.destroy();
assert.strictEqual(timeout_events, 1);
// Ensure the `timeout` event fired only once.
}, common.platformTimeout(HTTP_CLIENT_TIMEOUT * 2));
}

0 comments on commit bba500d

Please sign in to comment.