From 949e8851484c016c07f6cc9e5889f0f2e56baf2a Mon Sep 17 00:00:00 2001 From: killagu Date: Fri, 8 Jun 2018 12:42:27 +0800 Subject: [PATCH] http: fix request with option timeout and agent 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: https://github.com/nodejs/node/issues/21185 PR-URL: https://github.com/nodejs/node/pull/21204 Reviewed-By: Khaidi Chu Reviewed-By: Matteo Collina Reviewed-By: James M Snell Reviewed-By: Ujjwal Sharma --- doc/api/http.md | 2 + lib/_http_agent.js | 33 +++++++-- lib/_http_client.js | 74 ++++++++++--------- lib/net.js | 1 + test/parallel/test-http-client-set-timeout.js | 46 ++++++++++++ ...t-http-client-timeout-option-with-agent.js | 55 ++++++++++++++ 6 files changed, 169 insertions(+), 42 deletions(-) create mode 100644 test/parallel/test-http-client-set-timeout.js create mode 100644 test/parallel/test-http-client-timeout-option-with-agent.js diff --git a/doc/api/http.md b/doc/api/http.md index 78f8bc4b6b133f..fdbc99774e1b7c 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 c4f753412ca83b..7d28cda681d810 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -656,18 +656,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); }; @@ -717,42 +734,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 2393539737910a..889f28b0aa7042 100644 --- a/lib/net.js +++ b/lib/net.js @@ -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); 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)); +}