From 33e62a13940bc1976fc2e0c77ef0a0a506058bbb Mon Sep 17 00:00:00 2001 From: killagu Date: Fri, 8 Jun 2018 12:42:27 +0800 Subject: [PATCH 1/7] 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. Update agent doc, add timeout option. Fixes: https://github.com/nodejs/node/issues/21185 --- 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 | 43 +++++++++++ ...t-http-client-timeout-option-with-agent.js | 48 ++++++++++++ 6 files changed, 159 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 8f2c531713b4f5..de35c763caeb38 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} 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. 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..bb4d66db786956 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 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); }; @@ -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..b8e762717477af --- /dev/null +++ b/test/parallel/test-http-client-set-timeout.js @@ -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)); +} 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..e132581dbdb81e --- /dev/null +++ b/test/parallel/test-http-client-timeout-option-with-agent.js @@ -0,0 +1,48 @@ +'use strict'; +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)); + })); + req.end(); + + setTimeout(function() { + req.destroy(); + assert.strictEqual(timeout_events, 1); + }, common.platformTimeout(HTTP_CLIENT_TIMEOUT + 50)); +} From 4add8f66ad01b47edd2fe3a053c11147853bc7cf Mon Sep 17 00:00:00 2001 From: killagu Date: Mon, 11 Jun 2018 10:12:47 +0800 Subject: [PATCH 2/7] update tests --- test/parallel/test-http-client-set-timeout.js | 6 +++++- .../test-http-client-timeout-option-with-agent.js | 13 ++++++++++--- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/test/parallel/test-http-client-set-timeout.js b/test/parallel/test-http-client-set-timeout.js index b8e762717477af..7346fef477fdbe 100644 --- a/test/parallel/test-http-client-set-timeout.js +++ b/test/parallel/test-http-client-set-timeout.js @@ -1,5 +1,9 @@ 'use strict'; const common = require('../common'); + +// The test ensure call `req.setTimeout` will not let +// timeout fired twice. + const assert = require('assert'); const http = require('http'); @@ -39,5 +43,5 @@ function doRequest() { setTimeout(function() { req.destroy(); assert.strictEqual(timeout_events, 1); - }, common.platformTimeout(HTTP_CLIENT_TIMEOUT + 50)); + }, 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 index e132581dbdb81e..184503df55e982 100644 --- a/test/parallel/test-http-client-timeout-option-with-agent.js +++ b/test/parallel/test-http-client-timeout-option-with-agent.js @@ -1,10 +1,14 @@ 'use strict'; const common = require('../common'); + +// This test ensure http request with 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 = 2000; +const HTTP_CLIENT_TIMEOUT = 3000; const agent = new http.Agent({ timeout: HTTP_AGENT_TIMEOUT }); const options = { @@ -37,12 +41,15 @@ function doRequest() { req.on('timeout', common.mustCall(() => { timeout_events += 1; const duration = Date.now() - start; - assert((Math.abs(duration - HTTP_CLIENT_TIMEOUT) < 20)); + // The timeout event can not be accurately fired, It will delay + // in some 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); - }, common.platformTimeout(HTTP_CLIENT_TIMEOUT + 50)); + // Ensure the `timeout` be event fired only once. + }, common.platformTimeout(HTTP_CLIENT_TIMEOUT * 2)); } From 7ab62fb68627f8eb177efe2dd168f4160d0c62cc Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sun, 10 Jun 2018 20:23:39 -0700 Subject: [PATCH 3/7] minor edit to doc text --- doc/api/http.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/api/http.md b/doc/api/http.md index de35c763caeb38..dc4a75e8d438cf 100644 --- a/doc/api/http.md +++ b/doc/api/http.md @@ -126,7 +126,7 @@ 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. + * `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 From 977353131d5df85c87ee9b380db8950654d25257 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sun, 10 Jun 2018 20:24:58 -0700 Subject: [PATCH 4/7] fixup! minor edit to comment text --- lib/_http_client.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/_http_client.js b/lib/_http_client.js index bb4d66db786956..faedaac88aa21c 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -658,9 +658,9 @@ function listenSocketTimeout(req) { return; } const emitRequestTimeout = () => req.emit('timeout'); - // Set timeoutCb so that it'll get cleaned up on request end + // Set timeoutCb so it will get cleaned up on request end. req.timeoutCb = emitRequestTimeout; - // delegate socket timeout event + // Delegate socket timeout event. if (req.socket) { req.socket.once('timeout', emitRequestTimeout); } else { @@ -668,7 +668,7 @@ function listenSocketTimeout(req) { socket.once('timeout', emitRequestTimeout); }); } - // remove socket timeout listener after response end + // Remove socket timeout listener after response end. req.once('response', (res) => { res.once('end', () => { req.socket.removeListener('timeout', emitRequestTimeout); From 3b30906dc043f7cb4ece8344630ce2528cf6c5bc Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sun, 10 Jun 2018 20:25:54 -0700 Subject: [PATCH 5/7] squash! minor edit to comment --- test/parallel/test-http-client-set-timeout.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/parallel/test-http-client-set-timeout.js b/test/parallel/test-http-client-set-timeout.js index 7346fef477fdbe..b2bf0feed1a6fa 100644 --- a/test/parallel/test-http-client-set-timeout.js +++ b/test/parallel/test-http-client-set-timeout.js @@ -1,8 +1,7 @@ 'use strict'; const common = require('../common'); -// The test ensure call `req.setTimeout` will not let -// timeout fired twice. +// Test that `req.setTimeout` will fired exactly once. const assert = require('assert'); const http = require('http'); From 1cf617ef595a0aa31b5714c0e5fa28302f1b6f01 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sun, 10 Jun 2018 20:26:27 -0700 Subject: [PATCH 6/7] squash! minor edits to comments --- test/parallel/test-http-client-set-timeout.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-http-client-set-timeout.js b/test/parallel/test-http-client-set-timeout.js index b2bf0feed1a6fa..51284b42765493 100644 --- a/test/parallel/test-http-client-set-timeout.js +++ b/test/parallel/test-http-client-set-timeout.js @@ -17,7 +17,7 @@ const options = { }; const server = http.createServer(() => { - // never response + // Never respond. }); server.listen(0, options.host, function() { @@ -29,7 +29,7 @@ function doRequest() { const req = http.request(options); req.setTimeout(HTTP_CLIENT_TIMEOUT / 2); req.on('error', () => { - // this space is intentionally left blank + // This space is intentionally left blank. }); req.on('close', common.mustCall(() => server.close())); From 2c5f0c05bbedbf6647c436a90aa25ee4b3c60997 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sun, 10 Jun 2018 20:28:10 -0700 Subject: [PATCH 7/7] squash! minor edits to comments --- .../test-http-client-timeout-option-with-agent.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test/parallel/test-http-client-timeout-option-with-agent.js b/test/parallel/test-http-client-timeout-option-with-agent.js index 184503df55e982..a7f750a42e557b 100644 --- a/test/parallel/test-http-client-timeout-option-with-agent.js +++ b/test/parallel/test-http-client-timeout-option-with-agent.js @@ -1,7 +1,7 @@ 'use strict'; const common = require('../common'); -// This test ensure http request with timeout and agent, +// Test that when http request uses both timeout and agent, // timeout will work as expected. const assert = require('assert'); @@ -21,7 +21,7 @@ const options = { }; const server = http.createServer(() => { - // never response + // Never respond. }); server.listen(0, options.host, function() { @@ -33,7 +33,7 @@ function doRequest() { const req = http.request(options); const start = Date.now(); req.on('error', () => { - // this space is intentionally left blank + // This space is intentionally left blank. }); req.on('close', common.mustCall(() => server.close())); @@ -41,8 +41,8 @@ function doRequest() { req.on('timeout', common.mustCall(() => { timeout_events += 1; const duration = Date.now() - start; - // The timeout event can not be accurately fired, It will delay - // in some milliseconds, so test it in second units. + // 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(); @@ -50,6 +50,6 @@ function doRequest() { setTimeout(function() { req.destroy(); assert.strictEqual(timeout_events, 1); - // Ensure the `timeout` be event fired only once. + // Ensure the `timeout` event fired only once. }, common.platformTimeout(HTTP_CLIENT_TIMEOUT * 2)); }