From 125ba5367ed1ae400ec343faf89487d8c5528bb0 Mon Sep 17 00:00:00 2001
From: Subhi Al Hasan <jodevsa@gmail.com>
Date: Sat, 16 Jul 2022 00:23:34 +0300
Subject: [PATCH 1/2] fix: prioritise error events over timeouts

Error events in the implementation of 'net.connect' are defered to the next tick: https://github.com/nodejs/node/blob/main/lib/net.js#L1144
This will result in the TO callback always being executed before the error events callback in the callback queue.

This fix makes sure that error event callbacks are executed before the TO socket destroy logic.

fixes https://github.com/nodejs/undici/issues/1484
---
 lib/core/connect.js     | 10 +++++++++-
 package.json            |  1 +
 test/connect-timeout.js | 18 +++++++++++++++++-
 3 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/lib/core/connect.js b/lib/core/connect.js
index 57667a1314a..fbb35fec2d7 100644
--- a/lib/core/connect.js
+++ b/lib/core/connect.js
@@ -105,7 +105,15 @@ function buildConnector ({ maxCachedSessions, socketPath, timeout, ...opts }) {
 }
 
 function onConnectTimeout (socket) {
-  util.destroy(socket, new ConnectTimeoutError())
+  const destorySocket = () => util.destroy(socket, new ConnectTimeoutError())
+
+  // setImmediate is added to make sure that we priotorise socket error events over timeouts
+  // Windows needs an extra setImmediate probably due to implementation differences in the socket logic
+  if (process.platform === 'win32') {
+    setImmediate(() => setImmediate(() => destorySocket()))
+  } else {
+    setImmediate(() => destorySocket())
+  }
 }
 
 module.exports = buildConnector
diff --git a/package.json b/package.json
index 2bfb936dbe2..093656e4413 100644
--- a/package.json
+++ b/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",
diff --git a/test/connect-timeout.js b/test/connect-timeout.js
index eb6f02dbb1b..b2af6a3965d 100644
--- a/test/connect-timeout.js
+++ b/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)
 }

From 1ffe0e66430f1952174dff192e7f8a13eed6bf02 Mon Sep 17 00:00:00 2001
From: Subhi Al Hasan <jodevsa@gmail.com>
Date: Wed, 20 Jul 2022 00:11:55 +0300
Subject: [PATCH 2/2] fixes

---
 lib/core/connect.js | 43 +++++++++++++++++++++++++++++--------------
 1 file changed, 29 insertions(+), 14 deletions(-)

diff --git a/lib/core/connect.js b/lib/core/connect.js
index fbb35fec2d7..e9b456f8831 100644
--- a/lib/core/connect.js
+++ b/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,16 +102,33 @@ function buildConnector ({ maxCachedSessions, socketPath, timeout, ...opts }) {
   }
 }
 
-function onConnectTimeout (socket) {
-  const destorySocket = () => util.destroy(socket, new ConnectTimeoutError())
-
-  // setImmediate is added to make sure that we priotorise socket error events over timeouts
-  // Windows needs an extra setImmediate probably due to implementation differences in the socket logic
-  if (process.platform === 'win32') {
-    setImmediate(() => setImmediate(() => destorySocket()))
-  } else {
-    setImmediate(() => destorySocket())
+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())
+}
+
 module.exports = buildConnector