diff --git a/doc/api/http.md b/doc/api/http.md index 8f2c531713b4f5..dc4a75e8d438cf 100644 --- a/doc/api/http.md +++ b/doc/api/http.md @@ -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. diff --git a/lib/_http_agent.js b/lib/_http_agent.js index 67f3d667b28169..1a2920cf098298 100644 --- a/lib/_http_agent.js +++ b/lib/_http_agent.js @@ -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]; @@ -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. @@ -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); } }; @@ -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); } diff --git a/lib/_http_client.js b/lib/_http_client.js index 4542a81cf0e9a1..faedaac88aa21c 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -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 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); }; @@ -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]); }; diff --git a/lib/net.js b/lib/net.js index 809a6f0f14582e..a2180272ed5310 100644 --- a/lib/net.js +++ b/lib/net.js @@ -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); diff --git a/test/parallel/test-http-client-set-timeout.js b/test/parallel/test-http-client-set-timeout.js new file mode 100644 index 00000000000000..51284b42765493 --- /dev/null +++ b/test/parallel/test-http-client-set-timeout.js @@ -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)); +} diff --git a/test/parallel/test-http-client-timeout-option-with-agent.js b/test/parallel/test-http-client-timeout-option-with-agent.js new file mode 100644 index 00000000000000..a7f750a42e557b --- /dev/null +++ b/test/parallel/test-http-client-timeout-option-with-agent.js @@ -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)); +}