Always set request timeout on keep-alive connections

001eae3 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 · Nov 3, 2016 · 1c27101 · 1c27101
1 parent 7228f13
commit 1c27101
Show file tree

Hide file tree

Showing 2 changed files with 73 additions and 41 deletions.
diff --git 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
@@ -199,6 +199,31 @@ tape('connect timeout with non-timeout error', function(t) {
   })
 })
 
+tape('request timeout with keep-alive connection', function(t) {
+  var firstReq = {
+    url: s.url + '/timeout',
+    forever: true
+  }
+  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,
+      forever: true
+    }
+    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 === false, 'Socket should already be connected')
+    })
+  })
+})
+
 tape('cleanup', function(t) {
   s.close(function() {
     t.end()