Repository: denoland/deno
Pull request: fix(ext/node): New async setInterval function to improve the nodejs compatibility

#26499 

This PR adds a better implementation of `setInterval` using async functions, similar to the one in Node.js. It's mostly based on the Node.js version, with some updates to error handling and logic.

The PR is complete, but I need help with:
- Running a single test file
- Deciding where to place my test files

Currently, I have a test to validate my code, but I’m running it outside of `cargo test` because running all tests at once is frustrating and often causes my system to freeze due to high memory use. This issue occurred even before my changes.

Changed files:
filename: ext/node/polyfills/timers.ts
patch: @@ -15,10 +15,16 @@ import {
   setUnrefTimeout,
   Timeout,
 } from "ext:deno_node/internal/timers.mjs";
-import { validateFunction } from "ext:deno_node/internal/validators.mjs";
+import {
+  validateAbortSignal,
+  validateBoolean,
+  validateFunction,
+  validateObject,
+} 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";
 import * as timers from "ext:deno_web/02_timers.js";
+import { AbortError } from "ext:deno_node/internal/errors.ts";
 
 const clearTimeout_ = timers.clearTimeout;
 const clearInterval_ = timers.clearInterval;
@@ -89,10 +95,88 @@ export function clearImmediate(immediate: Immediate) {
   clearTimeout_(immediate._immediateId);
 }
 
+async function* setIntervalAsync(
+  after: number,
+  value: number,
+  options: { signal?: AbortSignal; ref?: boolean } = { __proto__: null },
+) {
+  validateObject(options, "options");
+
+  if (typeof options?.signal !== "undefined") {
+    validateAbortSignal(options.signal, "options.signal");
+  }
+
+  if (typeof options?.ref !== "undefined") {
+    validateBoolean(options.ref, "options.ref");
+  }
+
+  const { signal, ref = true } = options;
+
+  if (signal?.aborted) {
+    throw new AbortError(undefined, { cause: signal?.reason });
+  }
+
+  let onCancel: (() => void) | undefined = undefined;
+  let interval: Timeout | undefined = undefined;
+  try {
+    let notYielded = 0;
+    let callback: ((value?: object) => void) | undefined = undefined;
+    let rejectCallback: ((message?: string) => void) | undefined = undefined;
+    interval = new Timeout(
+      () => {
+        notYielded++;
+        if (callback) {
+          callback();
+          callback = undefined;
+          rejectCallback = undefined;
+        }
+      },
+      after,
+      [],
+      true,
+      ref,
+    );
+    if (signal) {
+      onCancel = () => {
+        clearInterval(interval);
+        if (rejectCallback) {
+          rejectCallback(signal.reason);
+          callback = undefined;
+          rejectCallback = undefined;
+        }
+      };
+      signal.addEventListener("abort", onCancel, { once: true });
+    }
+    while (!signal?.aborted) {
+      if (notYielded === 0) {
+        await new Promise((resolve: () => void, reject: () => void) => {
+          callback = resolve;
+          rejectCallback = reject;
+        });
+      }
+      for (; notYielded > 0; notYielded--) {
+        yield value;
+      }
+    }
+  } catch (error) {
+    if (signal?.aborted) {
+      throw new AbortError(undefined, { cause: signal?.reason });
+    }
+    throw error;
+  } finally {
+    if (interval) {
+      clearInterval(interval);
+    }
+    if (onCancel) {
+      signal?.removeEventListener("abort", onCancel);
+    }
+  }
+}
+
 export const promises = {
   setTimeout: promisify(setTimeout),
   setImmediate: promisify(setImmediate),
-  setInterval: promisify(setInterval),
+  setInterval: setIntervalAsync,
 };
 
 promises.scheduler = {

filename: tests/unit_node/timers_test.ts
patch: @@ -3,6 +3,7 @@
 import { assert, fail } from "@std/assert";
 import * as timers from "node:timers";
 import * as timersPromises from "node:timers/promises";
+import { assertEquals } from "@std/assert";
 
 Deno.test("[node/timers setTimeout]", () => {
   {
@@ -108,3 +109,153 @@ Deno.test("[node/timers setImmediate returns Immediate object]", () => {
   imm.hasRef();
   clearImmediate(imm);
 });
+
+Deno.test({
+  name: "setInterval yields correct values at expected intervals",
+  async fn() {
+    // Test configuration
+    const CONFIG = {
+      expectedValue: 42,
+      intervalMs: 100,
+      iterations: 3,
+      tolerancePercent: 10,
+    };
+
+    const { setInterval } = timersPromises;
+    const results: Array<{ value: number; timestamp: number }> = [];
+    const startTime = Date.now();
+
+    const iterator = setInterval(CONFIG.intervalMs, CONFIG.expectedValue);
+
+    for await (const value of iterator) {
+      results.push({
+        value,
+        timestamp: Date.now(),
+      });
+      if (results.length === CONFIG.iterations) {
+        break;
+      }
+    }
+
+    const values = results.map((r) => r.value);
+    assertEquals(
+      values,
+      Array(CONFIG.iterations).fill(CONFIG.expectedValue),
+      `Each iteration should yield ${CONFIG.expectedValue}`,
+    );
+
+    const intervals = results.slice(1).map((result, index) => ({
+      interval: result.timestamp - results[index].timestamp,
+      iterationNumber: index + 1,
+    }));
+
+    const toleranceMs = (CONFIG.tolerancePercent / 100) * CONFIG.intervalMs;
+    const expectedRange = {
+      min: CONFIG.intervalMs - toleranceMs,
+      max: CONFIG.intervalMs + toleranceMs,
+    };
+
+    intervals.forEach(({ interval, iterationNumber }) => {
+      const isWithinTolerance = interval >= expectedRange.min &&
+        interval <= expectedRange.max;
+
+      assertEquals(
+        isWithinTolerance,
+        true,
+        `Iteration ${iterationNumber}: Interval ${interval}ms should be within ` +
+          `${expectedRange.min}ms and ${expectedRange.max}ms ` +
+          `(${CONFIG.tolerancePercent}% tolerance of ${CONFIG.intervalMs}ms)`,
+      );
+    });
+
+    const totalDuration = results[results.length - 1].timestamp - startTime;
+    const expectedDuration = CONFIG.intervalMs * CONFIG.iterations;
+    const isDurationReasonable =
+      totalDuration >= (expectedDuration - toleranceMs) &&
+      totalDuration <= (expectedDuration + toleranceMs);
+
+    assertEquals(
+      isDurationReasonable,
+      true,
+      `Total duration ${totalDuration}ms should be close to ${expectedDuration}ms ` +
+        `(within ${toleranceMs}ms tolerance)`,
+    );
+
+    const timestamps = results.map((r) => r.timestamp);
+    const areTimestampsOrdered = timestamps.every((timestamp, i) =>
+      i === 0 || timestamp > timestamps[i - 1]
+    );
+
+    assertEquals(
+      areTimestampsOrdered,
+      true,
+      "Timestamps should be strictly increasing",
+    );
+  },
+});
+
+Deno.test({
+  name: "setInterval with AbortSignal stops after expected duration",
+  async fn() {
+    const INTERVAL_MS = 500;
+    const TOTAL_DURATION_MS = 3000;
+    const TOLERANCE_MS = 500;
+
+    const abortController = new AbortController();
+    const { setInterval } = timersPromises;
+
+    // Set up abort after specified duration
+    const abortTimeout = timers.setTimeout(() => {
+      abortController.abort();
+    }, TOTAL_DURATION_MS);
+
+    // Track iterations and timing
+    const startTime = Date.now();
+    const iterations: number[] = [];
+
+    try {
+      for await (
+        const _timestamp of setInterval(INTERVAL_MS, undefined, {
+          signal: abortController.signal,
+        })
+      ) {
+        iterations.push(Date.now() - startTime);
+      }
+    } catch (error) {
+      if (error instanceof Error && error.name !== "AbortError") {
+        throw error;
+      }
+    } finally {
+      timers.clearTimeout(abortTimeout);
+    }
+
+    // Validate timing
+    const totalDuration = iterations[iterations.length - 1];
+    const isWithinTolerance =
+      totalDuration >= (TOTAL_DURATION_MS - TOLERANCE_MS) &&
+      totalDuration <= (TOTAL_DURATION_MS + TOLERANCE_MS);
+
+    assertEquals(
+      isWithinTolerance,
+      true,
+      `Total duration ${totalDuration}ms should be within ±${TOLERANCE_MS}ms of ${TOTAL_DURATION_MS}ms`,
+    );
+
+    // Validate interval consistency
+    const intervalDeltas = iterations.slice(1).map((time, i) =>
+      time - iterations[i]
+    );
+
+    intervalDeltas.forEach((delta, i) => {
+      const isIntervalValid = delta >= (INTERVAL_MS - 50) &&
+        delta <= (INTERVAL_MS + 50);
+      assertEquals(
+        isIntervalValid,
+        true,
+        `Interval ${
+          i + 1
+        } duration (${delta}ms) should be within ±50ms of ${INTERVAL_MS}ms`,
+      );
+    });
+  },
+});