fix: prioritise error events over timeouts (nodejs#1551)

lib/core/connect.js

@@ -75,14 +75,12 @@ function buildConnector ({ maxCachedSessions, socketPath, timeout, ...opts }) {
       })
     }
 
-    const timeoutId = timeout
-      ? setTimeout(onConnectTimeout, timeout, socket)
-      : null
+    const cancelTimeout = setupTimeout(() => onConnectTimeout(socket), timeout)
 
     socket
       .setNoDelay(true)
       .once(protocol === 'https:' ? 'secureConnect' : 'connect', function () {
-        clearTimeout(timeoutId)
+        cancelTimeout()
 
         if (callback) {
           const cb = callback
@@ -91,7 +89,7 @@ function buildConnector ({ maxCachedSessions, socketPath, timeout, ...opts }) {
         }
       })
       .on('error', function (err) {
-        clearTimeout(timeoutId)
+        cancelTimeout()
 
         if (callback) {
           const cb = callback
@@ -104,6 +102,31 @@ function buildConnector ({ maxCachedSessions, socketPath, timeout, ...opts }) {
   }
 }
 
+function setupTimeout (onConnectTimeout, timeout) {
+  if (!timeout) {
+    return () => {}
+  }
+
+  let s1 = null
+  let s2 = null
+  const timeoutId = setTimeout(() => {
+    // setImmediate is added to make sure that we priotorise socket error events over timeouts
+    s1 = setImmediate(() => {
+      if (process.platform === 'win32') {
+        // Windows needs an extra setImmediate probably due to implementation differences in the socket logic
+        s2 = setImmediate(() => onConnectTimeout())
+      } else {
+        onConnectTimeout()
+      }
+    })
+  }, timeout)
+  return () => {
+    clearTimeout(timeoutId)
+    clearImmediate(s1)
+    clearImmediate(s2)
+  }
+}
+
 function onConnectTimeout (socket) {
   util.destroy(socket, new ConnectTimeoutError())
 }

package.json

@@ -67,6 +67,7 @@
     "@sinonjs/fake-timers": "^9.1.2",
     "@types/node": "^17.0.29",
     "abort-controller": "^3.0.0",
+    "atomic-sleep": "^1.0.0",
     "busboy": "^1.6.0",
     "chai": "^4.3.4",
     "chai-as-promised": "^7.1.1",

test/connect-timeout.js

@@ -3,8 +3,24 @@
 const { test } = require('tap')
 const { Client, Pool, errors } = require('..')
 const net = require('net')
+const sleep = require('atomic-sleep')
 
-// Never connect
+test('priotorise socket errors over timeouts', (t) => {
+  t.plan(1)
+  const connectTimeout = 1000
+  const client = new Pool('http://foobar.bar:1234', { connectTimeout })
+
+  client.request({ method: 'GET', path: '/foobar' })
+    .then(() => t.fail())
+    .catch((err) => {
+      t.equal(err.code, 'ENOTFOUND')
+    })
+
+  // block for 1001ms which is enough for the dns lookup to complete and TO to fire
+  sleep(connectTimeout + 1)
+})
+
+// never connect
 net.connect = function (options) {
   return new net.Socket(options)
 }