Skip to content

Commit

Permalink
fix: refactor fast timers, fix UND_ERR_CONNECT_TIMEOUT on event loop …
Browse files Browse the repository at this point in the history
…blocking (#3495) (#3673)

* 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?

(cherry picked from commit 8a09d77)

Co-authored-by: Aras Abbasi <[email protected]>
  • Loading branch information
github-actions[bot] and Uzlopak authored Oct 4, 2024
1 parent 7466672 commit 6764626
Show file tree
Hide file tree
Showing 10 changed files with 714 additions and 160 deletions.
18 changes: 18 additions & 0 deletions benchmarks/timers/compare-timer-getters.mjs
Original file line number Diff line number Diff line change
@@ -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()
63 changes: 39 additions & 24 deletions lib/core/connect.js
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand All @@ -144,7 +145,7 @@ function buildConnector ({ allowH2, maxCachedSessions, socketPath, timeout, sess
}
})
.on('error', function (err) {
cancelTimeout()
cancelConnectTimeout()

if (callback) {
const cb = callback
Expand All @@ -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'
Expand Down
14 changes: 7 additions & 7 deletions lib/dispatcher/client-h1.js
Original file line number Diff line number Diff line change
Expand Up @@ -164,20 +164,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) {
Expand Down Expand Up @@ -288,7 +288,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
Expand Down
Loading

0 comments on commit 6764626

Please sign in to comment.