From 0f4051a37ad23377091043206e64126003caa480 Mon Sep 17 00:00:00 2001 From: await-ovo <13152410380@163.com> Date: Mon, 3 Jul 2023 03:11:34 +0800 Subject: [PATCH] fix(ext/node): ignore cancelled timer when node timer refresh (#19637) For timers that have already executed clearTimeout, there is no need to recreate a new timer when refresh is executed again. --- cli/tests/unit_node/timers_test.ts | 12 +++++++- .../polyfills/internal/stream_base_commons.ts | 2 +- ext/node/polyfills/internal/timers.mjs | 30 +++++++++++++++++-- ext/node/polyfills/timers.ts | 28 +++++++++++++++-- 4 files changed, 64 insertions(+), 8 deletions(-) diff --git a/cli/tests/unit_node/timers_test.ts b/cli/tests/unit_node/timers_test.ts index 8676edb766..d110b44e45 100644 --- a/cli/tests/unit_node/timers_test.ts +++ b/cli/tests/unit_node/timers_test.ts @@ -1,6 +1,6 @@ // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license. -import { assert } from "../../../test_util/std/testing/asserts.ts"; +import { assert, fail } from "../../../test_util/std/testing/asserts.ts"; import * as timers from "node:timers"; import * as timersPromises from "node:timers/promises"; @@ -50,3 +50,13 @@ Deno.test("[node/timers/promises setTimeout]", () => { assert(p instanceof Promise); return p; }); + +// Regression test for https://github.com/denoland/deno/issues/17981 +Deno.test("[node/timers refresh cancelled timer]", () => { + const { setTimeout, clearTimeout } = timers; + const p = setTimeout(() => { + fail(); + }, 1); + clearTimeout(p); + p.refresh(); +}); diff --git a/ext/node/polyfills/internal/stream_base_commons.ts b/ext/node/polyfills/internal/stream_base_commons.ts index 250f54c7db..d7acf729de 100644 --- a/ext/node/polyfills/internal/stream_base_commons.ts +++ b/ext/node/polyfills/internal/stream_base_commons.ts @@ -35,7 +35,7 @@ import { import { isUint8Array } from "ext:deno_node/internal/util/types.ts"; import { errnoException } from "ext:deno_node/internal/errors.ts"; import { getTimerDuration, kTimeout } from "ext:deno_node/internal/timers.mjs"; -import { setUnrefTimeout } from "node:timers"; +import { clearTimeout, setUnrefTimeout } from "node:timers"; import { validateFunction } from "ext:deno_node/internal/validators.mjs"; import { codeMap } from "ext:deno_node/internal_binding/uv.ts"; import { Buffer } from "node:buffer"; diff --git a/ext/node/polyfills/internal/timers.mjs b/ext/node/polyfills/internal/timers.mjs index d7c986917b..c2e8c00e91 100644 --- a/ext/node/polyfills/internal/timers.mjs +++ b/ext/node/polyfills/internal/timers.mjs @@ -4,6 +4,13 @@ // TODO(petamoriken): enable prefer-primordials for node polyfills // deno-lint-ignore-file prefer-primordials +const primordials = globalThis.__bootstrap.primordials; +const { + MapPrototypeDelete, + MapPrototypeSet, + SafeMap, +} = primordials; + import { inspect } from "ext:deno_node/internal/util/inspect.mjs"; import { validateFunction, validateNumber } from "ext:deno_node/internal/validators.mjs"; import { ERR_OUT_OF_RANGE } from "ext:deno_node/internal/errors.ts"; @@ -22,6 +29,14 @@ export const kTimeout = Symbol("timeout"); const kRefed = Symbol("refed"); const createTimer = Symbol("createTimer"); +/** + * The keys in this map correspond to the key ID's in the spec's map of active + * timers. The values are the timeout's status. + * + * @type {Map} + */ +export const activeTimers = new SafeMap(); + // Timer constructor function. export function Timeout(callback, after, args, isRepeat, isRefed) { if (typeof after === "number" && after > TIMEOUT_MAX) { @@ -31,19 +46,26 @@ export function Timeout(callback, after, args, isRepeat, isRefed) { this._onTimeout = callback; this._timerArgs = args; this._isRepeat = isRepeat; + this._destroyed = false; this[kRefed] = isRefed; this[kTimerId] = this[createTimer](); } Timeout.prototype[createTimer] = function () { const callback = this._onTimeout; - const cb = (...args) => callback.bind(this)(...args); + const cb = (...args) => { + if (!this._isRepeat) { + MapPrototypeDelete(activeTimers, this[kTimerId]) + } + return callback.bind(this)(...args); + } const id = this._isRepeat ? setInterval_(cb, this._idleTimeout, ...this._timerArgs) : setTimeout_(cb, this._idleTimeout, ...this._timerArgs); if (!this[kRefed]) { Deno.unrefTimer(id); } + MapPrototypeSet(activeTimers, id, this); return id; }; @@ -59,8 +81,10 @@ Timeout.prototype[inspect.custom] = function (_, options) { }; Timeout.prototype.refresh = function () { - clearTimeout_(this[kTimerId]); - this[kTimerId] = this[createTimer](); + if (!this._destroyed) { + clearTimeout_(this[kTimerId]); + this[kTimerId] = this[createTimer](); + } return this; }; diff --git a/ext/node/polyfills/timers.ts b/ext/node/polyfills/timers.ts index e33f44f391..2bedbe65bf 100644 --- a/ext/node/polyfills/timers.ts +++ b/ext/node/polyfills/timers.ts @@ -3,7 +3,17 @@ // TODO(petamoriken): enable prefer-primordials for node polyfills // deno-lint-ignore-file prefer-primordials -import { setUnrefTimeout, Timeout } from "ext:deno_node/internal/timers.mjs"; +const primordials = globalThis.__bootstrap.primordials; +const { + MapPrototypeGet, + MapPrototypeDelete, +} = primordials; + +import { + activeTimers, + setUnrefTimeout, + Timeout, +} from "ext:deno_node/internal/timers.mjs"; import { validateFunction } from "ext:deno_node/internal/validators.mjs"; import { promisify } from "ext:deno_node/internal/util.mjs"; export { setUnrefTimeout } from "ext:deno_node/internal/timers.mjs"; @@ -32,7 +42,13 @@ export function clearTimeout(timeout?: Timeout | number) { if (timeout == null) { return; } - clearTimeout_(+timeout); + const id = +timeout; + const timer = MapPrototypeGet(activeTimers, id); + if (timer) { + timeout._destroyed = true; + MapPrototypeDelete(activeTimers, id); + } + clearTimeout_(id); } export function setInterval( callback: (...args: unknown[]) => void, @@ -46,7 +62,13 @@ export function clearInterval(timeout?: Timeout | number | string) { if (timeout == null) { return; } - clearInterval_(+timeout); + const id = +timeout; + const timer = MapPrototypeGet(activeTimers, id); + if (timer) { + timeout._destroyed = true; + MapPrototypeDelete(activeTimers, id); + } + clearInterval_(id); } // TODO(bartlomieju): implement the 'NodeJS.Immediate' versions of the timers. // https://github.com/DefinitelyTyped/DefinitelyTyped/blob/1163ead296d84e7a3c80d71e7c81ecbd1a130e9a/types/node/v12/globals.d.ts#L1120-L1131