test_runner: clean up promisified interval generation

Renegade334 · targos · commit 83cdf1701b9e · 2025-07-17T16:57:12.000+02:00
* yield from loop instead of setting up custom iterator * cancel abort listener on exit * do not call <Array>.at(0) PR-URL: #58824 Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
diff --git a/lib/internal/test_runner/mock/mock_timers.js b/lib/internal/test_runner/mock/mock_timers.js
@@ -1,7 +1,6 @@
 'use strict';
 
 const {
-  ArrayPrototypeAt,
   ArrayPrototypeForEach,
   ArrayPrototypeIncludes,
   DatePrototypeGetTime,
@@ -14,9 +13,8 @@ const {
   ObjectDefineProperty,
   ObjectGetOwnPropertyDescriptor,
   ObjectGetOwnPropertyDescriptors,
-  Promise,
+  PromiseWithResolvers,
   Symbol,
-  SymbolAsyncIterator,
   SymbolDispose,
   globalThis,
 } = primordials;
@@ -35,14 +33,15 @@ const {
   },
 } = require('internal/errors');
 
+const { addAbortListener } = require('internal/events/abort_listener');
+
 const { TIMEOUT_MAX } = require('internal/timers');
 
 const PriorityQueue = require('internal/priority_queue');
 const nodeTimers = require('timers');
 const nodeTimersPromises = require('timers/promises');
 const EventEmitter = require('events');
 
-let kResistStopPropagation;
 // Internal reference to the MockTimers class inside MockDate
 let kMock;
 // Initial epoch to which #now should be set to
@@ -423,62 +422,36 @@ class MockTimers {
   }
 
   async * #setIntervalPromisified(interval, result, options) {
-    const context = this;
     const emitter = new EventEmitter();
+
+    let abortListener;
     if (options?.signal) {
       validateAbortSignal(options.signal, 'options.signal');
 
       if (options.signal.aborted) {
         throw abortIt(options.signal);
       }
 
-      const onAbort = (reason) => {
-        emitter.emit('data', { __proto__: null, aborted: true, reason });
-      };
-
-      kResistStopPropagation ??= require('internal/event_target').kResistStopPropagation;
-      options.signal.addEventListener('abort', onAbort, {
-        __proto__: null,
-        once: true,
-        [kResistStopPropagation]: true,
+      abortListener = addAbortListener(options.signal, () => {
+        emitter.emit('error', abortIt(options.signal));
       });
     }
 
     const eventIt = EventEmitter.on(emitter, 'data');
-    const callback = () => {
-      emitter.emit('data', result);
-    };
+    const timer = this.#createTimer(true,
+                                    () => emitter.emit('data'),
+                                    interval,
+                                    options);
 
-    const timer = this.#createTimer(true, callback, interval, options);
-    const clearListeners = () => {
-      emitter.removeAllListeners();
-      context.#clearTimer(timer);
-    };
-    const iterator = {
-      __proto__: null,
-      [SymbolAsyncIterator]() {
-        return this;
-      },
-      async next() {
-        const result = await eventIt.next();
-        const value = ArrayPrototypeAt(result.value, 0);
-        if (value?.aborted) {
-          iterator.return();
-          throw abortIt(options.signal);
-        }
-
-        return {
-          __proto__: null,
-          done: result.done,
-          value,
-        };
-      },
-      async return() {
-        clearListeners();
-        return eventIt.return();
-      },
-    };
-    yield* iterator;
+    try {
+      // eslint-disable-next-line no-unused-vars
+      for await (const event of eventIt) {
+        yield result;
+      }
+    } finally {
+      abortListener?.[SymbolDispose]();
+      this.#clearInterval(timer);
+    }
   }
 
   #setImmediate(callback, ...args) {
@@ -490,38 +463,31 @@ class MockTimers {
     );
   }
 
-  #promisifyTimer({ timerFn, clearFn, ms, result, options }) {
-    return new Promise((resolve, reject) => {
-      if (options?.signal) {
-        try {
-          validateAbortSignal(options.signal, 'options.signal');
-        } catch (err) {
-          return reject(err);
-        }
-
-        if (options.signal.aborted) {
-          return reject(abortIt(options.signal));
-        }
-      }
+  async #promisifyTimer({ timerFn, clearFn, ms, result, options }) {
+    const { promise, resolve, reject } = PromiseWithResolvers();
+
+    let abortListener;
+    if (options?.signal) {
+      validateAbortSignal(options.signal, 'options.signal');
 
-      const onabort = () => {
-        clearFn(timer);
-        return reject(abortIt(options.signal));
-      };
-
-      const timer = timerFn(() => {
-        return resolve(result);
-      }, ms);
-
-      if (options?.signal) {
-        kResistStopPropagation ??= require('internal/event_target').kResistStopPropagation;
-        options.signal.addEventListener('abort', onabort, {
-          __proto__: null,
-          once: true,
-          [kResistStopPropagation]: true,
-        });
+      if (options.signal.aborted) {
+        throw abortIt(options.signal);
       }
-    });
+
+      abortListener = addAbortListener(options.signal, () => {
+        reject(abortIt(options.signal));
+      });
+    }
+
+    const timer = timerFn(resolve, ms);
+
+    try {
+      await promise;
+      return result;
+    } finally {
+      abortListener?.[SymbolDispose]();
+      clearFn(timer);
+    }
   }
 
   #setImmediatePromisified(result, options) {
diff --git a/test/parallel/test-runner-mock-timers.js b/test/parallel/test-runner-mock-timers.js
@@ -4,6 +4,7 @@ process.env.NODE_TEST_KNOWN_GLOBALS = 0;
 const common = require('../common');
 
 const assert = require('node:assert');
+const { getEventListeners } = require('node:events');
 const { it, mock, describe } = require('node:test');
 const nodeTimers = require('node:timers');
 const nodeTimersPromises = require('node:timers/promises');
@@ -422,6 +423,8 @@ describe('Mock Timers Test Suite', () => {
     });
 
     describe('timers/promises', () => {
+      const hasAbortListener = (signal) => !!getEventListeners(signal, 'abort').length;
+
       describe('setTimeout Suite', () => {
         it('should advance in time and trigger timers when calling the .tick function multiple times', async (t) => {
           t.mock.timers.enable({ apis: ['setTimeout'] });
@@ -515,6 +518,22 @@ describe('Mock Timers Test Suite', () => {
           });
         });
 
+        it('should clear the abort listener when the timer resolves', async (t) => {
+          t.mock.timers.enable({ apis: ['setTimeout'] });
+          const expectedResult = 'result';
+          const controller = new AbortController();
+          const p = nodeTimersPromises.setTimeout(500, expectedResult, {
+            ref: true,
+            signal: controller.signal,
+          });
+
+          assert(hasAbortListener(controller.signal));
+
+          t.mock.timers.tick(500);
+          await p;
+          assert(!hasAbortListener(controller.signal));
+        });
+
         it('should reject given an an invalid signal instance', async (t) => {
           t.mock.timers.enable({ apis: ['setTimeout'] });
           const expectedResult = 'result';
@@ -728,6 +747,23 @@ describe('Mock Timers Test Suite', () => {
           });
         });
 
+        it('should clear the abort listener when the interval returns', async (t) => {
+          t.mock.timers.enable({ apis: ['setInterval'] });
+
+          const abortController = new AbortController();
+          const intervalIterator = nodeTimersPromises.setInterval(1, Date.now(), {
+            signal: abortController.signal,
+          });
+
+          const first = intervalIterator.next();
+          t.mock.timers.tick();
+
+          await first;
+          assert(hasAbortListener(abortController.signal));
+          await intervalIterator.return();
+          assert(!hasAbortListener(abortController.signal));
+        });
+
         it('should abort operation given an abort controller signal on a real use case', async (t) => {
           t.mock.timers.enable({ apis: ['setInterval'] });
           const controller = new AbortController();
