fix: refactor fast timers, fix UND_ERR_CONNECT_TIMEOUT on event loop …

…blocking (#3495) * fix: refactor fast timers, fix UND_ERR_CONNECT_TIMEOUT on event loop blocking * also test native timers * improve the commentary of fasttimers * revert changes in client-h1.js * make fasttimers independent from performance.now * less flaky?
nodejs · Aug 27, 2024 · 8a09d77 · 8a09d77
1 parent b725457
commit 8a09d77
Show file tree

Hide file tree

Showing 10 changed files with 714 additions and 160 deletions.
diff --git a/benchmarks/timers/compare-timer-getters.mjs b/benchmarks/timers/compare-timer-getters.mjs
@@ -0,0 +1,18 @@
+import { bench, group, run } from 'mitata'
+
+group('timers', () => {
+  bench('Date.now()', () => {
+    Date.now()
+  })
+  bench('performance.now()', () => {
+    performance.now()
+  })
+  bench('Math.trunc(performance.now())', () => {
+    Math.trunc(performance.now())
+  })
+  bench('process.uptime()', () => {
+    process.uptime()
+  })
+})
+
+await run()
diff --git a/lib/core/connect.js b/lib/core/connect.js
@@ -4,6 +4,7 @@ const net = require('node:net')
 const assert = require('node:assert')
 const util = require('./util')
 const { InvalidArgumentError, ConnectTimeoutError } = require('./errors')
+const timers = require('../util/timers')
 
 let tls // include tls conditionally since it is not always available
 
@@ -130,12 +131,12 @@ function buildConnector ({ allowH2, maxCachedSessions, socketPath, timeout, sess
       socket.setKeepAlive(true, keepAliveInitialDelay)
     }
 
-    const cancelTimeout = setupTimeout(() => onConnectTimeout(socket), timeout)
+    const cancelConnectTimeout = setupConnectTimeout(new WeakRef(socket), timeout)
 
     socket
       .setNoDelay(true)
       .once(protocol === 'https:' ? 'secureConnect' : 'connect', function () {
-        cancelTimeout()
+        cancelConnectTimeout()
 
         if (callback) {
           const cb = callback
@@ -144,7 +145,7 @@ function buildConnector ({ allowH2, maxCachedSessions, socketPath, timeout, sess
         }
       })
       .on('error', function (err) {
-        cancelTimeout()
+        cancelConnectTimeout()
 
         if (callback) {
           const cb = callback
@@ -157,30 +158,44 @@ function buildConnector ({ allowH2, maxCachedSessions, socketPath, timeout, sess
   }
 }
 
-function setupTimeout (onConnectTimeout, timeout) {
-  if (!timeout) {
-    return () => {}
-  }
+const setupConnectTimeout = process.platform === 'win32'
+  ? (socket, timeout) => {
+      if (!timeout) {
+        return () => { }
+      }
 
-  let s1 = null
-  let s2 = null
-  const timeoutId = setTimeout(() => {
-    // setImmediate is added to make sure that we prioritize socket error events over timeouts
-    s1 = setImmediate(() => {
-      if (process.platform === 'win32') {
+      let s1 = null
+      let s2 = null
+      const timer = timers.setTimeout(() => {
+      // setImmediate is added to make sure that we prioritize socket error events over timeouts
+        s1 = setImmediate(() => {
         // Windows needs an extra setImmediate probably due to implementation differences in the socket logic
-        s2 = setImmediate(() => onConnectTimeout())
-      } else {
-        onConnectTimeout()
+          s2 = setImmediate(() => onConnectTimeout(socket.deref()))
+        })
+      }, timeout)
+      return () => {
+        timers.clearTimeout(timer)
+        clearImmediate(s1)
+        clearImmediate(s2)
       }
-    })
-  }, timeout)
-  return () => {
-    clearTimeout(timeoutId)
-    clearImmediate(s1)
-    clearImmediate(s2)
-  }
-}
+    }
+  : (socket, timeout) => {
+      if (!timeout) {
+        return () => { }
+      }
+
+      let s1 = null
+      const timer = timers.setTimeout(() => {
+      // setImmediate is added to make sure that we prioritize socket error events over timeouts
+        s1 = setImmediate(() => {
+          onConnectTimeout(socket.deref())
+        })
+      }, timeout)
+      return () => {
+        timers.clearTimeout(timer)
+        clearImmediate(s1)
+      }
+    }
 
 function onConnectTimeout (socket) {
   let message = 'Connect Timeout Error'

diff --git a/lib/dispatcher/client-h1.js b/lib/dispatcher/client-h1.js
@@ -162,20 +162,20 @@ class Parser {
     this.maxResponseSize = client[kMaxResponseSize]
   }
 
-  setTimeout (value, type) {
+  setTimeout (delay, type) {
     this.timeoutType = type
-    if (value !== this.timeoutValue) {
-      timers.clearTimeout(this.timeout)
-      if (value) {
-        this.timeout = timers.setTimeout(onParserTimeout, value, new WeakRef(this))
+    if (delay !== this.timeoutValue) {
+      this.timeout && timers.clearTimeout(this.timeout)
+      if (delay) {
+        this.timeout = timers.setTimeout(onParserTimeout, delay, new WeakRef(this))
         // istanbul ignore else: only for jest
         if (this.timeout.unref) {
           this.timeout.unref()
         }
       } else {
         this.timeout = null
       }
-      this.timeoutValue = value
+      this.timeoutValue = delay
     } else if (this.timeout) {
       // istanbul ignore else: only for jest
       if (this.timeout.refresh) {
@@ -286,7 +286,7 @@ class Parser {
     this.llhttp.llhttp_free(this.ptr)
     this.ptr = null
 
-    timers.clearTimeout(this.timeout)
+    this.timeout && timers.clearTimeout(this.timeout)
     this.timeout = null
     this.timeoutValue = null
     this.timeoutType = null