From 82da8b857050a7078a40b7e0f2781077c13abea7 Mon Sep 17 00:00:00 2001 From: Brian White Date: Thu, 3 Nov 2016 08:05:38 -0400 Subject: [PATCH] Always set request timeout on keep-alive connections 001eae3d479 would erroneously only set http request timeout if a new socket was being used for the request. This commit ensures the http request timeout is always set, even on keep-alive connections. --- request.js | 89 +++++++++++++++++++++++-------------------- tests/test-timeout.js | 29 ++++++++++++++ 2 files changed, 77 insertions(+), 41 deletions(-) diff --git a/request.js b/request.js index 035385d45..9528b5662 100644 --- a/request.js +++ b/request.js @@ -766,52 +766,59 @@ Request.prototype.start = function () { self.emit('drain') }) self.req.on('socket', function(socket) { + var setReqTimeout = function() { + // This timeout sets the amount of time to wait *between* bytes sent + // from the server once connected. + // + // In particular, it's useful for erroring if the server fails to send + // data halfway through streaming a response. + self.req.setTimeout(timeout, function () { + if (self.req) { + self.abort() + var e = new Error('ESOCKETTIMEDOUT') + e.code = 'ESOCKETTIMEDOUT' + e.connect = false + self.emit('error', e) + } + }) + } // `._connecting` was the old property which was made public in node v6.1.0 var isConnecting = socket._connecting || socket.connecting - // Only start the connection timer if we're actually connecting a new - // socket, otherwise if we're already connected (because this is a - // keep-alive connection) do not bother. This is important since we won't - // get a 'connect' event for an already connected socket. - if (timeout !== undefined && isConnecting) { - var onReqSockConnect = function() { - socket.removeListener('connect', onReqSockConnect) - clearTimeout(self.timeoutTimer) - self.timeoutTimer = null - // Set an additional timeout on the socket, via the `setsockopt` syscall. - // This timeout sets the amount of time to wait *between* bytes sent - // from the server once connected. - // - // In particular, it's useful for erroring if the server fails to send - // data halfway through streaming a response. - self.req.setTimeout(timeout, function () { - if (self.req) { - self.abort() - var e = new Error('ESOCKETTIMEDOUT') - e.code = 'ESOCKETTIMEDOUT' - e.connect = false - self.emit('error', e) - } - }) - } + if (timeout !== undefined) { + // Only start the connection timer if we're actually connecting a new + // socket, otherwise if we're already connected (because this is a + // keep-alive connection) do not bother. This is important since we won't + // get a 'connect' event for an already connected socket. + if (isConnecting) { + var onReqSockConnect = function() { + socket.removeListener('connect', onReqSockConnect) + clearTimeout(self.timeoutTimer) + self.timeoutTimer = null + setReqTimeout() + } - socket.on('connect', onReqSockConnect) + socket.on('connect', onReqSockConnect) - self.req.on('error', function(err) { - socket.removeListener('connect', onReqSockConnect) - }) + self.req.on('error', function(err) { + socket.removeListener('connect', onReqSockConnect) + }) - // Set a timeout in memory - this block will throw if the server takes more - // than `timeout` to write the HTTP status and headers (corresponding to - // the on('response') event on the client). NB: this measures wall-clock - // time, not the time between bytes sent by the server. - self.timeoutTimer = setTimeout(function () { - socket.removeListener('connect', onReqSockConnect) - self.abort() - var e = new Error('ETIMEDOUT') - e.code = 'ETIMEDOUT' - e.connect = true - self.emit('error', e) - }, timeout) + // Set a timeout in memory - this block will throw if the server takes more + // than `timeout` to write the HTTP status and headers (corresponding to + // the on('response') event on the client). NB: this measures wall-clock + // time, not the time between bytes sent by the server. + self.timeoutTimer = setTimeout(function () { + socket.removeListener('connect', onReqSockConnect) + self.abort() + var e = new Error('ETIMEDOUT') + e.code = 'ETIMEDOUT' + e.connect = true + self.emit('error', e) + }, timeout) + } else { + // We're already connected + setReqTimeout() + } } self.emit('socket', socket) }) diff --git a/tests/test-timeout.js b/tests/test-timeout.js index f2016b0b4..4eb6bc0cd 100644 --- a/tests/test-timeout.js +++ b/tests/test-timeout.js @@ -199,6 +199,35 @@ tape('connect timeout with non-timeout error', function(t) { }) }) +tape('request timeout with keep-alive connection', function(t) { + var agent = new require('http').Agent({ keepAlive: true }) + var firstReq = { + url: s.url + '/timeout', + agent: agent + } + request(firstReq, function(err) { + // We should now still have a socket open. For the second request we should + // see a request timeout on the active socket ... + t.equal(err, null) + var shouldReqTimeout = { + url: s.url + '/timeout', + timeout: 100, + agent: agent + } + request(shouldReqTimeout, function(err) { + checkErrCode(t, err) + t.ok(err.connect === false, 'Error should have been a request timeout error') + t.end() + }).on('socket', function(socket) { + var isConnecting = socket._connecting || socket.connecting + t.ok(isConnecting !== true, 'Socket should already be connected') + }) + }).on('socket', function(socket) { + var isConnecting = socket._connecting || socket.connecting + t.ok(isConnecting === true, 'Socket should be new') + }) +}) + tape('cleanup', function(t) { s.close(function() { t.end()