From 001eae3d479a13350b06b8d3f8182b2060a2c60d Mon Sep 17 00:00:00 2001
From: Brian White <mscdex@mscdex.net>
Date: Wed, 26 Oct 2016 11:46:27 -0400
Subject: [PATCH] Fix socket 'connect' listener handling

This commit ensures that both a 'connect' timer only starts when a
new socket is being used for a request and that the 'connect' timer
is stopped in more places.

Fixes: https://github.com/request/request/issues/2438
---
 request.js            | 18 +++++++++++++++--
 tests/test-timeout.js | 47 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+), 2 deletions(-)

diff --git a/request.js b/request.js
index 8ed57b7f0..035385d45 100644
--- a/request.js
+++ b/request.js
@@ -766,8 +766,15 @@ Request.prototype.start = function () {
     self.emit('drain')
   })
   self.req.on('socket', function(socket) {
-    if (timeout !== undefined) {
-      socket.once('connect', function() {
+    // `._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.
@@ -785,6 +792,12 @@ Request.prototype.start = function () {
             self.emit('error', e)
           }
         })
+      }
+
+      socket.on('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
@@ -792,6 +805,7 @@ Request.prototype.start = function () {
       // 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'
diff --git a/tests/test-timeout.js b/tests/test-timeout.js
index 1f2944e43..ddcb9e1eb 100644
--- a/tests/test-timeout.js
+++ b/tests/test-timeout.js
@@ -6,6 +6,19 @@ function checkErrCode(t, err) {
     'Error ETIMEDOUT or ESOCKETTIMEDOUT')
 }
 
+function checkEventHandlers(t, socket) {
+  var connectListeners = socket.listeners('connect')
+  var found = false
+  for (var i = 0; i < connectListeners.length; ++i) {
+    var fn = connectListeners[i]
+    if (typeof fn === 'function' && fn.name === 'onReqSockConnect') {
+      found = true
+      break
+    }
+  }
+  t.ok(!found, 'Connect listener should not exist')
+}
+
 var server = require('./server')
   , request = require('../index')
   , tape = require('tape')
@@ -75,10 +88,14 @@ tape('should not timeout', function(t) {
     timeout: 1200
   }
 
+  var socket
   request(shouldntTimeout, function(err, res, body) {
     t.equal(err, null)
     t.equal(body, 'waited')
+    checkEventHandlers(t, socket)
     t.end()
+  }).on('socket', function(socket_) {
+    socket = socket_
   })
 })
 
@@ -131,10 +148,40 @@ tape('connect timeout', function(t) {
     url: tarpitHost + '/timeout',
     timeout: 100
   }
+  var socket
   request(shouldConnectTimeout, function(err) {
     checkErrCode(t, err)
     t.ok(err.connect === true, 'Connect Timeout Error should set \'connect\' property to true')
+    checkEventHandlers(t, socket)
     t.end()
+  }).on('socket', function(socket_) {
+    socket = socket_
+  })
+})
+
+tape('connect timeout with non-timeout error', function(t) {
+  // We need a destination that will not immediately return a TCP Reset
+  // packet. StackOverflow suggests this host:
+  // https://stackoverflow.com/a/904609/329700
+  var tarpitHost = 'http://10.255.255.1'
+  var shouldConnectTimeout = {
+    url: tarpitHost + '/timeout',
+    timeout: 1000
+  }
+  var socket
+  request(shouldConnectTimeout, function(err) {
+    t.notEqual(err, null)
+    // Delay the check since the 'connect' handler is removed in a separate
+    // 'error' handler which gets triggered after this callback
+    setImmediate(function() {
+      checkEventHandlers(t, socket)
+      t.end()
+    })
+  }).on('socket', function(socket_) {
+    socket = socket_
+    setImmediate(function() {
+      socket.emit('error', new Error('Fake Error'))
+    })
   })
 })