Merge pull request #2431 from mscdex/timeouts-accuracy

Improve timeouts accuracy and node v6.8.0+ compatibility

request.js

@@ -736,6 +736,11 @@ Request.prototype.start = function () {
 
   debug('make request', self.uri.href)
 
+  // node v6.8.0 now supports a `timeout` value in `http.request()`, but we
+  // should delete it for now since we handle timeouts manually for better
+  // consistency with node versions before v6.8.0
+  delete reqOptions.timeout
+
   try {
     self.req = self.httpModule.request(reqOptions)
   } catch (err) {
@@ -747,38 +752,12 @@ Request.prototype.start = function () {
     self.startTime = new Date().getTime()
   }
 
+  var timeout
   if (self.timeout && !self.timeoutTimer) {
-    var timeout = self.timeout < 0 ? 0 : self.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 () {
-      var connectTimeout = self.req.socket && self.req.socket.readable === false
-      self.abort()
-      var e = new Error('ETIMEDOUT')
-      e.code = 'ETIMEDOUT'
-      e.connect = connectTimeout
-      self.emit('error', e)
-    }, timeout)
-
-    if (self.req.setTimeout) { // only works on node 0.6+
-      // 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, and may or may not correspond to the wall-clock time
-      // elapsed from the start of the request.
-      //
-      // 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.req.abort()
-          var e = new Error('ESOCKETTIMEDOUT')
-          e.code = 'ESOCKETTIMEDOUT'
-          e.connect = false
-          self.emit('error', e)
-        }
-      })
+    if (self.timeout < 0) {
+      timeout = 0
+    } else if (typeof self.timeout === 'number' && isFinite(self.timeout)) {
+      timeout = self.timeout
     }
   }
 
@@ -788,6 +767,39 @@ Request.prototype.start = function () {
     self.emit('drain')
   })
   self.req.on('socket', function(socket) {
+    if (timeout !== undefined) {
+      socket.once('connect', function() {
+        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)
+          }
+        })
+      })
+
+      // 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 () {
+        self.abort()
+        var e = new Error('ETIMEDOUT')
+        e.code = 'ETIMEDOUT'
+        e.connect = true
+        self.emit('error', e)
+      }, timeout)
+    }
     self.emit('socket', socket)
   })