From a36e1843b3625424c43967278f5d9f59707714b5 Mon Sep 17 00:00:00 2001 From: uzlopak Date: Fri, 23 Aug 2024 23:22:06 +0200 Subject: [PATCH 1/6] fix: refactor fast timers, fix UND_ERR_CONNECT_TIMEOUT on event loop blocking --- benchmarks/timers/compare-timer-getters.mjs | 18 + lib/core/connect.js | 63 ++-- lib/dispatcher/client-h1.js | 26 +- lib/util/timers.js | 372 +++++++++++++++++--- test/issue-3410.js | 47 +++ test/socket-timeout.js | 7 - test/timers.js | 176 +++++++++ test/util.js | 78 ---- test/utils/event-loop-blocker.js | 10 + test/utils/hello-world-server.js | 30 ++ 10 files changed, 667 insertions(+), 160 deletions(-) create mode 100644 benchmarks/timers/compare-timer-getters.mjs create mode 100644 test/issue-3410.js create mode 100644 test/timers.js create mode 100644 test/utils/event-loop-blocker.js create mode 100644 test/utils/hello-world-server.js diff --git a/benchmarks/timers/compare-timer-getters.mjs b/benchmarks/timers/compare-timer-getters.mjs new file mode 100644 index 00000000000..aadff558f1f --- /dev/null +++ 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 index b388f022298..e40fb90d81c 100644 --- 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 index 367b3c8d97d..af3af17841e 100644 --- a/lib/dispatcher/client-h1.js +++ b/lib/dispatcher/client-h1.js @@ -156,18 +156,20 @@ class Parser { this.bytesRead = 0 + this.timeoutDeadline = 0 this.keepAlive = '' this.contentLength = '' this.connection = '' 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, this) + if (delay !== this.timeoutValue) { + this.timeout && timers.clearTimeout(this.timeout) + if (delay) { + this.timeout = timers.setTimeout(onParserTimeout, delay, this) + this.timeoutDeadline = this.timeoutValue ? timers.now() + this.timeoutValue : 0 // istanbul ignore else: only for jest if (this.timeout.unref) { this.timeout.unref() @@ -175,7 +177,7 @@ class Parser { } else { this.timeout = null } - this.timeoutValue = value + this.timeoutValue = delay } else if (this.timeout) { // istanbul ignore else: only for jest if (this.timeout.refresh) { @@ -184,6 +186,12 @@ class Parser { } } + updateTimeout () { + if (this.timeoutDeadline && this.timeoutDeadline < timers.now()) { + onParserTimeout(this) + } + } + resume () { if (this.socket.destroyed || !this.paused) { return @@ -286,7 +294,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 @@ -613,6 +621,8 @@ class Parser { function onParserTimeout (parser) { const { socket, timeoutType, client } = parser + parser.timeoutDeadline = 0 + /* istanbul ignore else */ if (timeoutType === TIMEOUT_HEADERS) { if (!socket[kWriting] || socket.writableNeedDrain || client[kRunning] > 1) { @@ -813,6 +823,8 @@ function resumeH1 (client) { socket[kParser].setTimeout(headersTimeout, TIMEOUT_HEADERS) } } + + socket[kParser].updateTimeout() } } diff --git a/lib/util/timers.js b/lib/util/timers.js index d0091cc15f7..18ec4496c2b 100644 --- a/lib/util/timers.js +++ b/lib/util/timers.js @@ -1,99 +1,383 @@ 'use strict' -const TICK_MS = 499 +/** + * This module provides a fast timer implementation, + * + * The timer is "faster" because it is a low resolution timer that only + * tries to be accurate to within 500ms. This is useful for timers that + * are expected to be set with a delay of 1 second or more. Node.js + * timers are not guaranteed to be precise as the event loop could be + * blocked by other tasks, and thus the timers could be refreshed later + * than expected. + */ -let fastNow = Date.now() +const nativeSetTimeout = global.setTimeout +const nativeClearTimeout = global.clearTimeout + +/** + * The fastNow variable is used to store the current time in milliseconds + * since the process started. + * + * @type {number} + */ +let fastNow = 0 + +/** + * The RESOLUTION_MS is the desired time in milliseconds. + * + * @type {number} + * @default 1000 + */ +const RESOLUTION_MS = 1e3 + +/** + * TICK_MS is the desired time in milliseconds between each tick. Target + * is the half of the resolution time minus 1 ms to account for a potential + * overhead of the event loop. + * + * @type {number} + * @default 499 + */ +const TICK_MS = (RESOLUTION_MS >> 1) - 1 + +/** + * The fastNowTimeout is one Node.js timer that will be used to process + * the FastTimers in the fastTimers array. + * + * @type {NodeJS.Timeout} + */ let fastNowTimeout +/** + * The kFastTimer symbol is used to identify the FastTimer instances. + * @type {Symbol} + */ +const kFastTimer = Symbol('kFastTimer') + +/** + * The fastTimers array contains all the active FastTimers. + * + * @type {FastTimer[]} + */ const fastTimers = [] -function onTimeout () { - fastNow = Date.now() +/** + * The following constants are used to represent the state of a FastTimer. + */ - let len = fastTimers.length +/** + * The NOT_IN_LIST constant is used to mark the FastTimer as not in the + * fastTimers array. A FastTimer with this status will not be processed in the + * next tick by the onTick function. + * + * A FastTimer can be added back to the fastTimers array by calling the refresh + * method on the FastTimer instance. + * + * @type {-2} + */ +const NOT_IN_LIST = -2 + +/** + * The TO_BE_CLEARED constant is used to mark the FastTimer as to be planned to + * be removed from the fastTimers array. A FastTimer with this status will be + * removed from the fastTimers array in the next tick by the onTick function, + * thus it will not be processed again by the onTick function. + * + * A FastTimer can also be set to the TO_BE_CLEARED state if the clear method is + * called on the FastTimer instance. + * + * @type {-1} + */ +const TO_BE_CLEARED = -1 + +/** + * The PENDING constant is used to mark the FastTimer as waiting to be processed + * in the next tick by the onTick function. A FastTimer with this status will + * have their _idleStart value set accordingly and the status set to ACTIVE in + * the next tick by the onTick function. + * + * @type {0} + */ +const PENDING = 0 + +/** + * The ACTIVE constant is used to mark the FastTimer as active and waiting for + * the timer to expire. A FastTimer with this status will be checked in the next + * tick by the onTick function and if the timer has expired it will execute the + * callback. + * + * @type {1} + */ +const ACTIVE = 1 + +/** + * The onTick function is called every TICK_MS milliseconds and is responsible + * for processing the fastTimers array. + * + * @returns {void} + */ +function onTick () { + /** + * The fastNow variable is used to store the current time in milliseconds + * since the process started. + * + * @type {number} + */ + fastNow = Math.trunc(performance.now()) + + /** + * The idx variable is used to iterate over the fastTimers array. + * Expired fastTimers will be removed from the array by being + * replaced with the last element in the array. Thus, the idx variable + * will only be incremented if the current element is not removed. + * + * @type {number} + */ let idx = 0 + + /** + * The len variable will contain the length of the fastTimers array + * and will be decremented when a FastTimer has to be removed the fastTimers + * array. + * + * @type {number} + */ + let len = fastTimers.length + while (idx < len) { + /** + * @type {FastTimer} + */ const timer = fastTimers[idx] - if (timer.state === 0) { - timer.state = fastNow + timer.delay - TICK_MS - } else if (timer.state > 0 && fastNow >= timer.state) { - timer.state = -1 - timer.callback(timer.opaque) + // If the timer is in the PENDING state, set the _idleStart accordingly and + // set the state to ACTIVE. + // If the timer is in the ACTIVE state and the timer has expired, it will + // be processed in the next tick. + if (timer._state === PENDING) { + // Set the _idleStart value to the fastNow value minus the TICK_MS value + // to account for the time the timer was in the PENDING state. + timer._idleStart = fastNow - TICK_MS + timer._state = ACTIVE + } else if ( + timer._state === ACTIVE && + fastNow >= timer._idleStart + timer._idleTimeout + ) { + timer._state = TO_BE_CLEARED + timer._idleStart = -1 + timer._onTimeout(timer._timerArg) } - if (timer.state === -1) { - timer.state = -2 - if (idx !== len - 1) { - fastTimers[idx] = fastTimers.pop() - } else { - fastTimers.pop() + if (timer._state === TO_BE_CLEARED) { + timer._state = NOT_IN_LIST + + // Move the last element to the current index and decrement len if it is + // not the only element in the array. + // After the while loop completed the excess elements will be removed. + if (--len !== 0) { + fastTimers[idx] = fastTimers[len] } - len -= 1 } else { - idx += 1 + ++idx } } - if (fastTimers.length > 0) { + // Set the length of the fastTimers array to the new length and thus + // removing the excess FastTimers from the array. + fastTimers.length = len + + // If there are still active FastTimers in the array, refresh the Timer. + // If there are no active FastTimers, the timer will be refreshed again + // when a new FastTimer is instantiated. + if (fastTimers.length !== 0) { refreshTimeout() } } function refreshTimeout () { - if (fastNowTimeout?.refresh) { + // If the fastNowTimeout is already set, refresh it. + if (fastNowTimeout) { fastNowTimeout.refresh() + // fastNowTimeout is not instantiated yet, create a new Timer. } else { clearTimeout(fastNowTimeout) - fastNowTimeout = setTimeout(onTimeout, TICK_MS) + fastNowTimeout = setTimeout(onTick, TICK_MS) + + // If the Timer has an unref method, call it to allow the process to exit if + // there are no other active handles. if (fastNowTimeout.unref) { fastNowTimeout.unref() } } } -class Timeout { - constructor (callback, delay, opaque) { - this.callback = callback - this.delay = delay - this.opaque = opaque +/** + * The FastTimer class is a low resolution timer. + */ +class FastTimer { + [kFastTimer] = true + + /** + * If the state of the timer is a non-negative number, it represents the + * time in milliseconds when the timer should expire. Values equal or less + * than zero represent the following states: + * - NOT_IN_LIST: The timer is not in the list of active timers. + * - TO_BE_CLEARED: The timer is in the list of active timers but is marked + * to be cleared and removed from the fastTimers array in the next tick. + * - PENDING: The timer is in the list of active timers and is waiting for + * the next tick to be activated. + * - ACTIVE: The timer is in the list of active timers and is waiting for + * the timer to expire. + * @type {number} + * @private + */ + _state = NOT_IN_LIST + + /** + * The time in milliseconds when the timer should expire. + * + * @type {number} + * @private + */ + _idleTimeout = -1 + + /** + * The time in milliseconds when the timer was started. This value is used to + * calculate when the timer should expire. If the timer is in the PENDING + * state, this value is set to the fastNow value in the next tick by the + * onTick function. + * + * @type {number} + * @private + */ + _idleStart = -1 + + /** + * The function to be executed when the timer expires. + * @type {Function} + * @private + */ + _onTimeout - // -2 not in timer list - // -1 in timer list but inactive - // 0 in timer list waiting for time - // > 0 in timer list waiting for time to expire - this.state = -2 + /** + * The argument to be passed to the function when the timer expires. + * @type {*} + * @private + */ + _timerArg + + /** + * @constructor + * @param {Function} callback A function to be executed after the timer + * expires. + * @param {number} delay The time, in milliseconds that the timer should wait + * before the specified function or code is executed. + * @param {*} arg + */ + constructor (callback, delay, arg) { + this._onTimeout = callback + this._idleTimeout = delay + this._timerArg = arg this.refresh() } + /** + * Sets the timer's start time to the current time, and reschedules the timer + * to call its callback at the previously specified duration adjusted to the + * current time. + * Using this on a timer that has already called its callback will reactivate + * the timer. + * + * @returns {void} + */ refresh () { - if (this.state === -2) { + // In the special case that the timer is not in the list of active timers, + // add it back to the array to be processed in the next tick by the onTick + // function. + if (this._state === NOT_IN_LIST) { fastTimers.push(this) - if (!fastNowTimeout || fastTimers.length === 1) { - refreshTimeout() - } } - this.state = 0 + // If the timer is the only active timer, refresh the fastNowTimeout for + // better resolution. + if (!fastNowTimeout || fastTimers.length === 1) { + refreshTimeout() + } + + // Setting the state to PENDING will cause the timer to be reset in the + // next tick by the onTick function. + this._state = PENDING } + /** + * The clear method marks the timer as to be cleared, by setting the _state + * properly. The timer will be removed from the list of active timers in the + * next tick by the onTick function + * + * @returns {void} + * @private + */ clear () { - this.state = -1 + this._state = TO_BE_CLEARED + this._idleStart = -1 } } +/** + * This module exports a setTimeout and clearTimeout function that can be + * used as a drop-in replacement for the native functions. + */ module.exports = { - setTimeout (callback, delay, opaque) { - return delay <= 1e3 - ? setTimeout(callback, delay, opaque) - : new Timeout(callback, delay, opaque) + /** + * The setTimeout() method sets a timer which executes a function once the + * timer expires. + * @param {Function} callback A function to be executed after the timer + * expires. + * @param {number} delay The time, in milliseconds that the timer should + * wait before the specified function or code is executed. + * @param {*} [arg] An optional argument to be passed to the callback function + * when the timer expires. + * @returns {NodeJS.Timeout|FastTimer} + */ + setTimeout (callback, delay, arg) { + // If the delay is less than or equal to the RESOLUTION_MS value return a + // native Node.js Timer instance. + return delay <= RESOLUTION_MS + ? nativeSetTimeout(callback, delay, arg) + : new FastTimer(callback, delay, arg) }, + /** + * The clearTimeout method cancels an instantiated Timer previously created + * by calling setTimeout. + * + * @param {FastTimer} timeout + */ clearTimeout (timeout) { - if (timeout instanceof Timeout) { + // If the timeout is a FastTimer, call its own clear method. + if (timeout[kFastTimer]) { + /** + * @type {FastTimer} + */ timeout.clear() + // Otherwise it an instance of a native NodeJS.Timeout, so call the + // Node.js native clearTimeout function. } else { - clearTimeout(timeout) + nativeClearTimeout(timeout) } - } + }, + /** + * The now method returns the value of the cached performance.now() value. + * + * @returns {number} + */ + now () { + return fastNow + }, + /** + * Exporting for testing purposes only. + * Marking as deprecated to discourage any use outside of testing. + * @deprecated + */ + kFastTimer } diff --git a/test/issue-3410.js b/test/issue-3410.js new file mode 100644 index 00000000000..646424d6a29 --- /dev/null +++ b/test/issue-3410.js @@ -0,0 +1,47 @@ +'use strict' + +const { tspl } = require('@matteo.collina/tspl') +const { fork } = require('node:child_process') +const { resolve: pathResolve } = require('node:path') +const { test } = require('node:test') +const { Agent, fetch, setGlobalDispatcher } = require('..') +const { eventLoopBlocker } = require('./utils/event-loop-blocker') + +test('https://github.com/nodejs/undici/issues/3356', async (t) => { + t = tspl(t, { plan: 1 }) + + // Spawn a server in a new process to avoid effects from the blocking event loop + const { + serverProcess, + address + } = await new Promise((resolve, reject) => { + const childProcess = fork( + pathResolve(__dirname, './utils/hello-world-server.js'), + [], + { windowsHide: true } + ) + + childProcess.on('message', (address) => { + resolve({ + serverProcess: childProcess, + address + }) + }) + childProcess.on('error', err => { + reject(err) + }) + }) + + const connectTimeout = 2000 + setGlobalDispatcher(new Agent({ connectTimeout })) + + const fetchPromise = fetch(address) + + eventLoopBlocker(3000) + + const response = await fetchPromise + + t.equal(await response.text(), 'Hello World') + + serverProcess.kill('SIGKILL') +}) diff --git a/test/socket-timeout.js b/test/socket-timeout.js index 5be62fe4119..43ee1576615 100644 --- a/test/socket-timeout.js +++ b/test/socket-timeout.js @@ -3,7 +3,6 @@ const { tspl } = require('@matteo.collina/tspl') const { test, after } = require('node:test') const { Client, errors } = require('..') -const timers = require('../lib/util/timers') const { createServer } = require('node:http') const FakeTimers = require('@sinonjs/fake-timers') @@ -68,12 +67,6 @@ test('Disable socket timeout', async (t) => { const clock = FakeTimers.install() after(clock.uninstall.bind(clock)) - const orgTimers = { ...timers } - Object.assign(timers, { setTimeout, clearTimeout }) - after(() => { - Object.assign(timers, orgTimers) - }) - server.once('request', (req, res) => { setTimeout(() => { res.end('hello') diff --git a/test/timers.js b/test/timers.js new file mode 100644 index 00000000000..acec3df8eef --- /dev/null +++ b/test/timers.js @@ -0,0 +1,176 @@ +'use strict' + +const { tspl } = require('@matteo.collina/tspl') +const { describe, test } = require('node:test') + +const timers = require('../lib/util/timers') + +describe('timers', () => { + test('timers exports a clearTimeout', (t) => { + t = tspl(t, { plan: 1 }) + + t.ok(typeof timers.clearTimeout === 'function') + }) + + test('timers exports a setTimeout', (t) => { + t = tspl(t, { plan: 1 }) + + t.ok(typeof timers.setTimeout === 'function') + }) + + test('setTimeout instantiates a native NodeJS.Timeout when delay is lower or equal 1e3 ms', (t) => { + t = tspl(t, { plan: 2 }) + + t.strictEqual(timers.setTimeout(() => { }, 999)[timers.kFastTimer], undefined) + t.strictEqual(timers.setTimeout(() => { }, 1e3)[timers.kFastTimer], undefined) + }) + + test('setTimeout instantiates a FastTimer when delay is smaller than 1e3 ms', (t) => { + t = tspl(t, { plan: 1 }) + + const timeout = timers.setTimeout(() => { }, 1001) + t.strictEqual(timeout[timers.kFastTimer], true) + }) + + test('clearTimeout can clear a node native Timeout', (t) => { + t = tspl(t, { plan: 3 }) + + const nativeTimeoutId = setTimeout(() => { }, 1e6) + t.equal(nativeTimeoutId._idleTimeout, 1e6) + t.ok(timers.clearTimeout(nativeTimeoutId) === undefined) + t.equal(nativeTimeoutId._idleTimeout, -1) + }) + + test('a FastTimer will get a _idleStart value after short time', async (t) => { + t = tspl(t, { plan: 3 }) + + const timer = timers.setTimeout(() => { + t.fail('timer should not have fired') + }, 1e4) + + t.strictEqual(timer[timers.kFastTimer], true) + t.strictEqual(timer._idleStart, -1) + await new Promise((resolve) => setTimeout(resolve, 750)) + t.notStrictEqual(timer._idleStart, -1) + + timers.clearTimeout(timer) + }) + + test('a cleared FastTimer will reset the _idleStart value to -1', async (t) => { + t = tspl(t, { plan: 4 }) + + const timer = timers.setTimeout(() => { + t.fail('timer should not have fired') + }, 1e4) + + t.strictEqual(timer[timers.kFastTimer], true) + t.strictEqual(timer._idleStart, -1) + await new Promise((resolve) => setTimeout(resolve, 750)) + t.notStrictEqual(timer._idleStart, -1) + timers.clearTimeout(timer) + t.strictEqual(timer._idleStart, -1) + }) + + test('a FastTimer can be cleared', async (t) => { + t = tspl(t, { plan: 3 }) + + const timer = timers.setTimeout(() => { + t.fail('timer should not have fired') + }, 1001) + + t.strictEqual(timer[timers.kFastTimer], true) + timers.clearTimeout(timer) + + t.strictEqual(timer._idleStart, -1) + await new Promise((resolve) => setTimeout(resolve, 750)) + t.strictEqual(timer._idleStart, -1) + }) + + test('a cleared FastTimer can be refreshed', async (t) => { + t = tspl(t, { plan: 2 }) + + const timer = timers.setTimeout(() => { + t.ok('pass') + }, 1001) + + t.strictEqual(timer[timers.kFastTimer], true) + timers.clearTimeout(timer) + timer.refresh() + await new Promise((resolve) => setTimeout(resolve, 2000)) + timers.clearTimeout(timer) + }) + + const getDelta = (start, target) => { + const end = process.hrtime.bigint() + const actual = (end - start) / 1_000_000n + return actual - BigInt(target) + } + + // timers.setTimeout implements a low resolution timer with a 500 ms granularity + // It is expected that in the worst case, a timer will fire about 500 ms after the + // intended amount of time, an extra 200 ms is added to account event loop overhead + // Timers should never fire excessively early, 1ms early is tolerated + const ACCEPTABLE_DELTA = 700n + + test('meet acceptable resolution time', async (t) => { + const testTimeouts = [0, 1, 499, 500, 501, 990, 999, 1000, 1001, 1100, 1400, 1499, 1500, 4000, 5000] + + t = tspl(t, { plan: 1 + testTimeouts.length * 2 }) + + const start = process.hrtime.bigint() + + for (const target of testTimeouts) { + timers.setTimeout(() => { + const delta = getDelta(start, target) + + t.ok(delta >= -1n, `${target}ms fired early`) + t.ok(delta < ACCEPTABLE_DELTA, `${target}ms fired late, got difference of ${delta}ms`) + }, target) + } + + setTimeout(() => t.ok(true), 6000) + await t.completed + }) + + test('refresh correctly with timeout < TICK_MS', async (t) => { + t = tspl(t, { plan: 3 }) + + const start = process.hrtime.bigint() + + const timeout = timers.setTimeout(() => { + // 400 ms timer was refreshed after 600ms; total target is 1000 + const delta = getDelta(start, 1000) + + t.ok(delta >= -1n, 'refreshed timer fired early') + t.ok(delta < ACCEPTABLE_DELTA, 'refreshed timer fired late') + }, 400) + + setTimeout(() => timeout.refresh(), 200) + setTimeout(() => timeout.refresh(), 400) + setTimeout(() => timeout.refresh(), 600) + + setTimeout(() => t.ok(true), 1500) + await t.completed + }) + + test('refresh correctly with timeout > TICK_MS', async (t) => { + t = tspl(t, { plan: 3 }) + + const start = process.hrtime.bigint() + + const timeout = timers.setTimeout(() => { + // 501ms timer was refreshed after 1250ms; total target is 1751 + const delta = getDelta(start, 1751) + + t.ok(delta >= -1n, 'refreshed timer fired early') + t.ok(delta < ACCEPTABLE_DELTA, 'refreshed timer fired late') + }, 501) + + setTimeout(() => timeout.refresh(), 250) + setTimeout(() => timeout.refresh(), 750) + setTimeout(() => timeout.refresh(), 1250) + + setTimeout(() => t.ok(true), 3000) + await t.completed + }) +}) diff --git a/test/util.js b/test/util.js index f646ec4ff25..b3e1193e506 100644 --- a/test/util.js +++ b/test/util.js @@ -1,12 +1,10 @@ 'use strict' -const { tspl } = require('@matteo.collina/tspl') const { strictEqual, throws, doesNotThrow } = require('node:assert') const { test, describe } = require('node:test') const { isBlobLike, parseURL, isHttpOrHttpsPrefixed, isValidPort } = require('../lib/core/util') const { Blob, File } = require('node:buffer') const { InvalidArgumentError } = require('../lib/core/errors') -const timers = require('../lib/util/timers') describe('isBlobLike', () => { test('buffer', () => { @@ -255,79 +253,3 @@ describe('parseURL', () => { }) }) }) - -describe('timers', () => { - const getDelta = (start, target) => { - const end = process.hrtime.bigint() - const actual = (end - start) / 1_000_000n - return actual - BigInt(target) - } - - // timers.setTimeout implements a low resolution timer with a 500 ms granularity - // It is expected that in the worst case, a timer will fire about 500 ms after the - // intended amount of time, an extra 200 ms is added to account event loop overhead - // Timers should never fire excessively early, 1ms early is tolerated - const ACCEPTABLE_DELTA = 700n - - test('meet acceptable resolution time', async (t) => { - const testTimeouts = [0, 1, 499, 500, 501, 990, 999, 1000, 1001, 1100, 1400, 1499, 1500, 4000, 5000] - - t = tspl(t, { plan: 1 + testTimeouts.length * 2 }) - - const start = process.hrtime.bigint() - - for (const target of testTimeouts) { - timers.setTimeout(() => { - const delta = getDelta(start, target) - - t.ok(delta >= -1n, `${target}ms fired early`) - t.ok(delta < ACCEPTABLE_DELTA, `${target}ms fired late`) - }, target) - } - - setTimeout(() => t.ok(true), 6000) - await t.completed - }) - - test('refresh correctly with timeout < TICK_MS', async (t) => { - t = tspl(t, { plan: 3 }) - - const start = process.hrtime.bigint() - - const timeout = timers.setTimeout(() => { - // 400 ms timer was refreshed after 600ms; total target is 1000 - const delta = getDelta(start, 1000) - - t.ok(delta >= -1n, 'refreshed timer fired early') - t.ok(delta < ACCEPTABLE_DELTA, 'refreshed timer fired late') - }, 400) - - setTimeout(() => timeout.refresh(), 200) - setTimeout(() => timeout.refresh(), 400) - setTimeout(() => timeout.refresh(), 600) - - setTimeout(() => t.ok(true), 1500) - await t.completed - }) - - test('refresh correctly with timeout > TICK_MS', async (t) => { - t = tspl(t, { plan: 3 }) - - const start = process.hrtime.bigint() - - const timeout = timers.setTimeout(() => { - // 501ms timer was refreshed after 1250ms; total target is 1751 - const delta = getDelta(start, 1751) - - t.ok(delta >= -1n, 'refreshed timer fired early') - t.ok(delta < ACCEPTABLE_DELTA, 'refreshed timer fired late') - }, 501) - - setTimeout(() => timeout.refresh(), 250) - setTimeout(() => timeout.refresh(), 750) - setTimeout(() => timeout.refresh(), 1250) - - setTimeout(() => t.ok(true), 3000) - await t.completed - }) -}) diff --git a/test/utils/event-loop-blocker.js b/test/utils/event-loop-blocker.js new file mode 100644 index 00000000000..9c2ec5075f0 --- /dev/null +++ b/test/utils/event-loop-blocker.js @@ -0,0 +1,10 @@ +'use strict' + +function eventLoopBlocker (ms) { + const nil = new Int32Array(new SharedArrayBuffer(4)) + Atomics.wait(nil, 0, 0, ms) +} + +module.exports = { + eventLoopBlocker +} diff --git a/test/utils/hello-world-server.js b/test/utils/hello-world-server.js new file mode 100644 index 00000000000..520ed3a734d --- /dev/null +++ b/test/utils/hello-world-server.js @@ -0,0 +1,30 @@ +'use strict' + +const { createServer } = require('node:http') +const hostname = '127.0.0.1' + +const server = createServer(async (req, res) => { + res.statusCode = 200 + res.setHeader('Content-Type', 'text/plain') + + await sendInDelayedChunks(res, 'Hello World', 125) + res.end() +}) + +async function sendInDelayedChunks (res, payload, delay) { + const chunks = payload.split('') + + for (const chunk of chunks) { + await new Promise(resolve => setTimeout(resolve, delay)) + + res.write(chunk) + } +} + +server.listen(0, hostname, () => { + if (process.send) { + process.send(`http://${hostname}:${server.address().port}/`) + } else { + console.log(`http://${hostname}:${server.address().port}/`) + } +}) From 50cc05d3d212325fae04d816273e9c2fc61e3fe5 Mon Sep 17 00:00:00 2001 From: uzlopak Date: Sat, 24 Aug 2024 00:16:19 +0200 Subject: [PATCH 2/6] also test native timers --- fetch-test.js | 16 ++++++++ test/issue-3410.js | 99 ++++++++++++++++++++++++++++++++-------------- 2 files changed, 86 insertions(+), 29 deletions(-) create mode 100644 fetch-test.js diff --git a/fetch-test.js b/fetch-test.js new file mode 100644 index 00000000000..d6084c371c3 --- /dev/null +++ b/fetch-test.js @@ -0,0 +1,16 @@ +// fetch-test.js +const { fetch } = require('.') +const { createServer } = require('node:http') + +const port = 8080 +const url = 'http://localhost:' + port + +const server = createServer((req, res) => res.end()).listen(port, async () => { + await fetch(url) + server.closeIdleConnections() + + setImmediate(async () => { + await fetch(url) // Throws TypeError with cause UND_ERR_SOCKET or ECONNRESET + server.close() + }) +}) diff --git a/test/issue-3410.js b/test/issue-3410.js index 646424d6a29..d5bb8093ef2 100644 --- a/test/issue-3410.js +++ b/test/issue-3410.js @@ -3,45 +3,86 @@ const { tspl } = require('@matteo.collina/tspl') const { fork } = require('node:child_process') const { resolve: pathResolve } = require('node:path') -const { test } = require('node:test') +const { describe, test } = require('node:test') const { Agent, fetch, setGlobalDispatcher } = require('..') const { eventLoopBlocker } = require('./utils/event-loop-blocker') -test('https://github.com/nodejs/undici/issues/3356', async (t) => { - t = tspl(t, { plan: 1 }) - - // Spawn a server in a new process to avoid effects from the blocking event loop - const { - serverProcess, - address - } = await new Promise((resolve, reject) => { - const childProcess = fork( - pathResolve(__dirname, './utils/hello-world-server.js'), - [], - { windowsHide: true } - ) - - childProcess.on('message', (address) => { - resolve({ - serverProcess: childProcess, - address +describe('https://github.com/nodejs/undici/issues/3410', () => { + test('FastTimers', async (t) => { + t = tspl(t, { plan: 1 }) + + // Spawn a server in a new process to avoid effects from the blocking event loop + const { + serverProcess, + address + } = await new Promise((resolve, reject) => { + const childProcess = fork( + pathResolve(__dirname, './utils/hello-world-server.js'), + [], + { windowsHide: true } + ) + + childProcess.on('message', (address) => { + resolve({ + serverProcess: childProcess, + address + }) + }) + childProcess.on('error', err => { + reject(err) }) }) - childProcess.on('error', err => { - reject(err) - }) + + const connectTimeout = 2000 + setGlobalDispatcher(new Agent({ connectTimeout })) + + const fetchPromise = fetch(address) + + eventLoopBlocker(3000) + + const response = await fetchPromise + + t.equal(await response.text(), 'Hello World') + + serverProcess.kill('SIGKILL') }) - const connectTimeout = 2000 - setGlobalDispatcher(new Agent({ connectTimeout })) + test('native Timers', async (t) => { + t = tspl(t, { plan: 1 }) + + // Spawn a server in a new process to avoid effects from the blocking event loop + const { + serverProcess, + address + } = await new Promise((resolve, reject) => { + const childProcess = fork( + pathResolve(__dirname, './utils/hello-world-server.js'), + [], + { windowsHide: true } + ) + + childProcess.on('message', (address) => { + resolve({ + serverProcess: childProcess, + address + }) + }) + childProcess.on('error', err => { + reject(err) + }) + }) + + const connectTimeout = 900 + setGlobalDispatcher(new Agent({ connectTimeout })) - const fetchPromise = fetch(address) + const fetchPromise = fetch(address) - eventLoopBlocker(3000) + eventLoopBlocker(1500) - const response = await fetchPromise + const response = await fetchPromise - t.equal(await response.text(), 'Hello World') + t.equal(await response.text(), 'Hello World') - serverProcess.kill('SIGKILL') + serverProcess.kill('SIGKILL') + }) }) From 5f275eb3051dd959b89ce0e23f2d78569cb504c9 Mon Sep 17 00:00:00 2001 From: uzlopak Date: Sat, 24 Aug 2024 13:50:34 +0200 Subject: [PATCH 3/6] improve the commentary of fasttimers --- fetch-test.js | 16 ------ lib/util/timers.js | 134 ++++++++++++++++++++++----------------------- 2 files changed, 65 insertions(+), 85 deletions(-) delete mode 100644 fetch-test.js diff --git a/fetch-test.js b/fetch-test.js deleted file mode 100644 index d6084c371c3..00000000000 --- a/fetch-test.js +++ /dev/null @@ -1,16 +0,0 @@ -// fetch-test.js -const { fetch } = require('.') -const { createServer } = require('node:http') - -const port = 8080 -const url = 'http://localhost:' + port - -const server = createServer((req, res) => res.end()).listen(port, async () => { - await fetch(url) - server.closeIdleConnections() - - setImmediate(async () => { - await fetch(url) // Throws TypeError with cause UND_ERR_SOCKET or ECONNRESET - server.close() - }) -}) diff --git a/lib/util/timers.js b/lib/util/timers.js index 18ec4496c2b..5dfa9420d71 100644 --- a/lib/util/timers.js +++ b/lib/util/timers.js @@ -1,21 +1,24 @@ 'use strict' /** - * This module provides a fast timer implementation, + * This module offers an optimized timer implementation designed for scenarios + * where high precision is not critical. * - * The timer is "faster" because it is a low resolution timer that only - * tries to be accurate to within 500ms. This is useful for timers that - * are expected to be set with a delay of 1 second or more. Node.js - * timers are not guaranteed to be precise as the event loop could be - * blocked by other tasks, and thus the timers could be refreshed later - * than expected. + * The timer achieves faster performance by using a low-resolution approach, + * with an accuracy target of within 500ms. This makes it particularly useful + * for timers with delays of 1 second or more, where exact timing is less + * crucial. + * + * It's important to note that Node.js timers are inherently imprecise, as + * delays can occur due to the event loop being blocked by other operations. + * Consequently, timers may trigger later than their scheduled time. */ const nativeSetTimeout = global.setTimeout const nativeClearTimeout = global.clearTimeout /** - * The fastNow variable is used to store the current time in milliseconds + * The fastNow variable is used to store a current time in milliseconds * since the process started. * * @type {number} @@ -23,7 +26,7 @@ const nativeClearTimeout = global.clearTimeout let fastNow = 0 /** - * The RESOLUTION_MS is the desired time in milliseconds. + * RESOLUTION_MS represents the target resolution time in milliseconds. * * @type {number} * @default 1000 @@ -31,9 +34,9 @@ let fastNow = 0 const RESOLUTION_MS = 1e3 /** - * TICK_MS is the desired time in milliseconds between each tick. Target - * is the half of the resolution time minus 1 ms to account for a potential - * overhead of the event loop. + * TICK_MS defines the desired interval in milliseconds between each tick. + * The target value is set to half the resolution time, minus 1 ms, to account + * for potential event loop overhead. * * @type {number} * @default 499 @@ -41,78 +44,75 @@ const RESOLUTION_MS = 1e3 const TICK_MS = (RESOLUTION_MS >> 1) - 1 /** - * The fastNowTimeout is one Node.js timer that will be used to process - * the FastTimers in the fastTimers array. + * fastNowTimeout is a Node.js timer used to manage and process + * the FastTimers stored in the `fastTimers` array. * * @type {NodeJS.Timeout} */ let fastNowTimeout /** - * The kFastTimer symbol is used to identify the FastTimer instances. + * The kFastTimer symbol is used to identify FastTimer instances. + * * @type {Symbol} */ const kFastTimer = Symbol('kFastTimer') /** - * The fastTimers array contains all the active FastTimers. + * The fastTimers array contains all active FastTimers. * * @type {FastTimer[]} */ const fastTimers = [] /** - * The following constants are used to represent the state of a FastTimer. + * These constants represent the various states of a FastTimer. */ /** - * The NOT_IN_LIST constant is used to mark the FastTimer as not in the - * fastTimers array. A FastTimer with this status will not be processed in the - * next tick by the onTick function. + * The `NOT_IN_LIST` constant indicates that the FastTimer is not included + * in the `fastTimers` array. Timers with this status will not be processed + * during the next tick by the `onTick` function. * - * A FastTimer can be added back to the fastTimers array by calling the refresh - * method on the FastTimer instance. + * A FastTimer can be re-added to the `fastTimers` array by invoking the + * `refresh` method on the FastTimer instance. * * @type {-2} */ const NOT_IN_LIST = -2 /** - * The TO_BE_CLEARED constant is used to mark the FastTimer as to be planned to - * be removed from the fastTimers array. A FastTimer with this status will be - * removed from the fastTimers array in the next tick by the onTick function, - * thus it will not be processed again by the onTick function. + * The `TO_BE_CLEARED` constant indicates that the FastTimer is scheduled + * for removal from the `fastTimers` array. A FastTimer in this state will + * be removed in the next tick by the `onTick` function and will no longer + * be processed. * - * A FastTimer can also be set to the TO_BE_CLEARED state if the clear method is - * called on the FastTimer instance. + * This status is also set when the `clear` method is called on the FastTimer instance. * * @type {-1} */ const TO_BE_CLEARED = -1 /** - * The PENDING constant is used to mark the FastTimer as waiting to be processed - * in the next tick by the onTick function. A FastTimer with this status will - * have their _idleStart value set accordingly and the status set to ACTIVE in - * the next tick by the onTick function. + * The `PENDING` constant signifies that the FastTimer is awaiting processing + * in the next tick by the `onTick` function. Timers with this status will have + * their `_idleStart` value set and their status updated to `ACTIVE` in the next tick. * * @type {0} */ const PENDING = 0 /** - * The ACTIVE constant is used to mark the FastTimer as active and waiting for - * the timer to expire. A FastTimer with this status will be checked in the next - * tick by the onTick function and if the timer has expired it will execute the - * callback. + * The `ACTIVE` constant indicates that the FastTimer is active and waiting + * for its timer to expire. During the next tick, the `onTick` function will + * check if the timer has expired, and if so, it will execute the associated callback. * * @type {1} */ const ACTIVE = 1 /** - * The onTick function is called every TICK_MS milliseconds and is responsible - * for processing the fastTimers array. + * The onTick function processes the fastTimers array. * * @returns {void} */ @@ -126,10 +126,9 @@ function onTick () { fastNow = Math.trunc(performance.now()) /** - * The idx variable is used to iterate over the fastTimers array. - * Expired fastTimers will be removed from the array by being - * replaced with the last element in the array. Thus, the idx variable - * will only be incremented if the current element is not removed. + * The `idx` variable is used to iterate over the `fastTimers` array. + * Expired timers are removed by replacing them with the last element in the array. + * Consequently, `idx` is only incremented when the current element is not removed. * * @type {number} */ @@ -137,8 +136,8 @@ function onTick () { /** * The len variable will contain the length of the fastTimers array - * and will be decremented when a FastTimer has to be removed the fastTimers - * array. + * and will be decremented when a FastTimer should be removed from the + * fastTimers array. * * @type {number} */ @@ -150,8 +149,6 @@ function onTick () { */ const timer = fastTimers[idx] - // If the timer is in the PENDING state, set the _idleStart accordingly and - // set the state to ACTIVE. // If the timer is in the ACTIVE state and the timer has expired, it will // be processed in the next tick. if (timer._state === PENDING) { @@ -173,7 +170,6 @@ function onTick () { // Move the last element to the current index and decrement len if it is // not the only element in the array. - // After the while loop completed the excess elements will be removed. if (--len !== 0) { fastTimers[idx] = fastTimers[len] } @@ -183,7 +179,7 @@ function onTick () { } // Set the length of the fastTimers array to the new length and thus - // removing the excess FastTimers from the array. + // removing the excess FastTimers elements from the array. fastTimers.length = len // If there are still active FastTimers in the array, refresh the Timer. @@ -212,29 +208,26 @@ function refreshTimeout () { } /** - * The FastTimer class is a low resolution timer. + * The `FastTimer` class is a data structure designed to store and manage + * timer information. */ class FastTimer { [kFastTimer] = true /** - * If the state of the timer is a non-negative number, it represents the - * time in milliseconds when the timer should expire. Values equal or less - * than zero represent the following states: - * - NOT_IN_LIST: The timer is not in the list of active timers. - * - TO_BE_CLEARED: The timer is in the list of active timers but is marked - * to be cleared and removed from the fastTimers array in the next tick. - * - PENDING: The timer is in the list of active timers and is waiting for - * the next tick to be activated. - * - ACTIVE: The timer is in the list of active timers and is waiting for - * the timer to expire. - * @type {number} + * The state of the timer, which can be one of the following: + * - NOT_IN_LIST (-2) + * - TO_BE_CLEARED (-1) + * - PENDING (0) + * - ACTIVE (1) + * + * @type {-2|-1|0|1} * @private */ _state = NOT_IN_LIST /** - * The time in milliseconds when the timer should expire. + * The number of milliseconds to wait before calling the callback. * * @type {number} * @private @@ -243,11 +236,10 @@ class FastTimer { /** * The time in milliseconds when the timer was started. This value is used to - * calculate when the timer should expire. If the timer is in the PENDING - * state, this value is set to the fastNow value in the next tick by the - * onTick function. + * calculate when the timer should expire. * * @type {number} + * @default -1 * @private */ _idleStart = -1 @@ -260,7 +252,8 @@ class FastTimer { _onTimeout /** - * The argument to be passed to the function when the timer expires. + * The argument to be passed to the callback when the timer expires. + * * @type {*} * @private */ @@ -311,15 +304,18 @@ class FastTimer { } /** - * The clear method marks the timer as to be cleared, by setting the _state - * properly. The timer will be removed from the list of active timers in the - * next tick by the onTick function + * The `clear` method cancels the timer, preventing it from executing. * * @returns {void} * @private */ clear () { + // Set the state to TO_BE_CLEARED to mark the timer for removal in the next + // tick by the onTick function. this._state = TO_BE_CLEARED + + // Reset the _idleStart value to -1 to indicate that the timer is no longer + // active. this._idleStart = -1 } } @@ -360,7 +356,7 @@ module.exports = { * @type {FastTimer} */ timeout.clear() - // Otherwise it an instance of a native NodeJS.Timeout, so call the + // Otherwise it is an instance of a native NodeJS.Timeout, so call the // Node.js native clearTimeout function. } else { nativeClearTimeout(timeout) From b802be6e8e0a14b23999caf9cf2cf21524138893 Mon Sep 17 00:00:00 2001 From: uzlopak Date: Sat, 24 Aug 2024 23:38:29 +0200 Subject: [PATCH 4/6] revert changes in client-h1.js --- lib/dispatcher/client-h1.js | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/lib/dispatcher/client-h1.js b/lib/dispatcher/client-h1.js index af3af17841e..6d024402a5f 100644 --- a/lib/dispatcher/client-h1.js +++ b/lib/dispatcher/client-h1.js @@ -156,7 +156,6 @@ class Parser { this.bytesRead = 0 - this.timeoutDeadline = 0 this.keepAlive = '' this.contentLength = '' this.connection = '' @@ -169,7 +168,6 @@ class Parser { this.timeout && timers.clearTimeout(this.timeout) if (delay) { this.timeout = timers.setTimeout(onParserTimeout, delay, this) - this.timeoutDeadline = this.timeoutValue ? timers.now() + this.timeoutValue : 0 // istanbul ignore else: only for jest if (this.timeout.unref) { this.timeout.unref() @@ -186,12 +184,6 @@ class Parser { } } - updateTimeout () { - if (this.timeoutDeadline && this.timeoutDeadline < timers.now()) { - onParserTimeout(this) - } - } - resume () { if (this.socket.destroyed || !this.paused) { return @@ -621,8 +613,6 @@ class Parser { function onParserTimeout (parser) { const { socket, timeoutType, client } = parser - parser.timeoutDeadline = 0 - /* istanbul ignore else */ if (timeoutType === TIMEOUT_HEADERS) { if (!socket[kWriting] || socket.writableNeedDrain || client[kRunning] > 1) { @@ -823,8 +813,6 @@ function resumeH1 (client) { socket[kParser].setTimeout(headersTimeout, TIMEOUT_HEADERS) } } - - socket[kParser].updateTimeout() } } From ad339a2feddf08267e287799332cd5ed48ba799a Mon Sep 17 00:00:00 2001 From: uzlopak Date: Sun, 25 Aug 2024 16:47:42 +0200 Subject: [PATCH 5/6] make fasttimers independent from performance.now --- lib/util/timers.js | 12 ++++++------ test/timers.js | 22 ++++++++++++++++++++++ 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/lib/util/timers.js b/lib/util/timers.js index 5dfa9420d71..8fa3ac56b7b 100644 --- a/lib/util/timers.js +++ b/lib/util/timers.js @@ -18,8 +18,7 @@ const nativeSetTimeout = global.setTimeout const nativeClearTimeout = global.clearTimeout /** - * The fastNow variable is used to store a current time in milliseconds - * since the process started. + * The fastNow variable contains the internal fast timer clock value. * * @type {number} */ @@ -118,12 +117,13 @@ const ACTIVE = 1 */ function onTick () { /** - * The fastNow variable is used to store the current time in milliseconds - * since the process started. + * Increment the fastNow value by the TICK_MS value, despite the actual time + * that has passed since the last tick. This approach ensures independence + * from the system clock and delays caused by a blocked event loop. * * @type {number} */ - fastNow = Math.trunc(performance.now()) + fastNow += TICK_MS /** * The `idx` variable is used to iterate over the `fastTimers` array. @@ -363,7 +363,7 @@ module.exports = { } }, /** - * The now method returns the value of the cached performance.now() value. + * The now method returns the value of the internal fast timer clock. * * @returns {number} */ diff --git a/test/timers.js b/test/timers.js index acec3df8eef..792f67063ba 100644 --- a/test/timers.js +++ b/test/timers.js @@ -4,6 +4,7 @@ const { tspl } = require('@matteo.collina/tspl') const { describe, test } = require('node:test') const timers = require('../lib/util/timers') +const { eventLoopBlocker } = require('./utils/event-loop-blocker') describe('timers', () => { test('timers exports a clearTimeout', (t) => { @@ -100,6 +101,27 @@ describe('timers', () => { timers.clearTimeout(timer) }) + test('a FastTimer will only increment by the defined TICK_MS value', async (t) => { + t = tspl(t, { plan: 2 }) + + const startInternalClock = timers.now() + + // The long running FastTimer will ensure that the internal clock is + // incremented by the TICK_MS value in the onTick function + const longRunningFastTimer = timers.setTimeout(() => {}, 1e10) + + eventLoopBlocker(1000) + + // wait to ensure the timer has fired in the next loop + await new Promise((resolve) => setTimeout(resolve, 1)) + + t.strictEqual(timers.now() - startInternalClock, 499) + await new Promise((resolve) => setTimeout(resolve, 1000)) + t.strictEqual(timers.now() - startInternalClock, 1497) + + timers.clearTimeout(longRunningFastTimer) + }) + const getDelta = (start, target) => { const end = process.hrtime.bigint() const actual = (end - start) / 1_000_000n From 33fe1014bba66fc4a99d025507100259536ae00a Mon Sep 17 00:00:00 2001 From: uzlopak Date: Sun, 25 Aug 2024 17:00:31 +0200 Subject: [PATCH 6/6] less flaky? --- test/timers.js | 42 +++++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/test/timers.js b/test/timers.js index 792f67063ba..621e1aa74bb 100644 --- a/test/timers.js +++ b/test/timers.js @@ -101,27 +101,6 @@ describe('timers', () => { timers.clearTimeout(timer) }) - test('a FastTimer will only increment by the defined TICK_MS value', async (t) => { - t = tspl(t, { plan: 2 }) - - const startInternalClock = timers.now() - - // The long running FastTimer will ensure that the internal clock is - // incremented by the TICK_MS value in the onTick function - const longRunningFastTimer = timers.setTimeout(() => {}, 1e10) - - eventLoopBlocker(1000) - - // wait to ensure the timer has fired in the next loop - await new Promise((resolve) => setTimeout(resolve, 1)) - - t.strictEqual(timers.now() - startInternalClock, 499) - await new Promise((resolve) => setTimeout(resolve, 1000)) - t.strictEqual(timers.now() - startInternalClock, 1497) - - timers.clearTimeout(longRunningFastTimer) - }) - const getDelta = (start, target) => { const end = process.hrtime.bigint() const actual = (end - start) / 1_000_000n @@ -195,4 +174,25 @@ describe('timers', () => { setTimeout(() => t.ok(true), 3000) await t.completed }) + + test('a FastTimer will only increment by the defined TICK_MS value', async (t) => { + t = tspl(t, { plan: 2 }) + + const startInternalClock = timers.now() + + // The long running FastTimer will ensure that the internal clock is + // incremented by the TICK_MS value in the onTick function + const longRunningFastTimer = timers.setTimeout(() => {}, 1e10) + + eventLoopBlocker(1000) + + // wait to ensure the timer has fired in the next loop + await new Promise((resolve) => setTimeout(resolve, 1)) + + t.strictEqual(timers.now() - startInternalClock, 499) + await new Promise((resolve) => setTimeout(resolve, 1000)) + t.ok(timers.now() - startInternalClock <= 1497) + + timers.clearTimeout(longRunningFastTimer) + }) })