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: fix request with option timeout and agent #21204

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
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
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} A number specifying the 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 @@ -647,18 +647,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 that it'll 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 @@ -708,42 +725,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);
}
}

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 @@ -405,6 +405,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
43 changes: 43 additions & 0 deletions test/parallel/test-http-client-set-timeout.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
'use strict';
const common = require('../common');
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 response
});

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 + 50));
}
48 changes: 48 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,48 @@
'use strict';
Copy link
Member

Choose a reason for hiding this comment

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

Would you be willing to add a comment here explaining what this is testing?

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

const HTTP_AGENT_TIMEOUT = 1000;
const HTTP_CLIENT_TIMEOUT = 2000;

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 response
});

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;
assert((Math.abs(duration - HTTP_CLIENT_TIMEOUT) < 20));
Copy link
Member

Choose a reason for hiding this comment

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

Regarding the request for a comment above, it would be useful in particular to know if what this is all about. 20 seems like a magic number and this sort of fuzzy duration check is a bit of a red flag in tests too. Curious if there's another way to do it that still manages to check whatever the test is testing for.

}));
req.end();

setTimeout(function() {
req.destroy();
assert.strictEqual(timeout_events, 1);
}, common.platformTimeout(HTTP_CLIENT_TIMEOUT + 50));
}