Skip to content

Commit 83cdf17

Browse files
Renegade334targos
authored andcommitted
test_runner: clean up promisified interval generation
* 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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>
1 parent 45ffdb3 commit 83cdf17

File tree

2 files changed

+79
-77
lines changed

2 files changed

+79
-77
lines changed

lib/internal/test_runner/mock/mock_timers.js

Lines changed: 43 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
'use strict';
22

33
const {
4-
ArrayPrototypeAt,
54
ArrayPrototypeForEach,
65
ArrayPrototypeIncludes,
76
DatePrototypeGetTime,
@@ -14,9 +13,8 @@ const {
1413
ObjectDefineProperty,
1514
ObjectGetOwnPropertyDescriptor,
1615
ObjectGetOwnPropertyDescriptors,
17-
Promise,
16+
PromiseWithResolvers,
1817
Symbol,
19-
SymbolAsyncIterator,
2018
SymbolDispose,
2119
globalThis,
2220
} = primordials;
@@ -35,14 +33,15 @@ const {
3533
},
3634
} = require('internal/errors');
3735

36+
const { addAbortListener } = require('internal/events/abort_listener');
37+
3838
const { TIMEOUT_MAX } = require('internal/timers');
3939

4040
const PriorityQueue = require('internal/priority_queue');
4141
const nodeTimers = require('timers');
4242
const nodeTimersPromises = require('timers/promises');
4343
const EventEmitter = require('events');
4444

45-
let kResistStopPropagation;
4645
// Internal reference to the MockTimers class inside MockDate
4746
let kMock;
4847
// Initial epoch to which #now should be set to
@@ -423,62 +422,36 @@ class MockTimers {
423422
}
424423

425424
async * #setIntervalPromisified(interval, result, options) {
426-
const context = this;
427425
const emitter = new EventEmitter();
426+
427+
let abortListener;
428428
if (options?.signal) {
429429
validateAbortSignal(options.signal, 'options.signal');
430430

431431
if (options.signal.aborted) {
432432
throw abortIt(options.signal);
433433
}
434434

435-
const onAbort = (reason) => {
436-
emitter.emit('data', { __proto__: null, aborted: true, reason });
437-
};
438-
439-
kResistStopPropagation ??= require('internal/event_target').kResistStopPropagation;
440-
options.signal.addEventListener('abort', onAbort, {
441-
__proto__: null,
442-
once: true,
443-
[kResistStopPropagation]: true,
435+
abortListener = addAbortListener(options.signal, () => {
436+
emitter.emit('error', abortIt(options.signal));
444437
});
445438
}
446439

447440
const eventIt = EventEmitter.on(emitter, 'data');
448-
const callback = () => {
449-
emitter.emit('data', result);
450-
};
441+
const timer = this.#createTimer(true,
442+
() => emitter.emit('data'),
443+
interval,
444+
options);
451445

452-
const timer = this.#createTimer(true, callback, interval, options);
453-
const clearListeners = () => {
454-
emitter.removeAllListeners();
455-
context.#clearTimer(timer);
456-
};
457-
const iterator = {
458-
__proto__: null,
459-
[SymbolAsyncIterator]() {
460-
return this;
461-
},
462-
async next() {
463-
const result = await eventIt.next();
464-
const value = ArrayPrototypeAt(result.value, 0);
465-
if (value?.aborted) {
466-
iterator.return();
467-
throw abortIt(options.signal);
468-
}
469-
470-
return {
471-
__proto__: null,
472-
done: result.done,
473-
value,
474-
};
475-
},
476-
async return() {
477-
clearListeners();
478-
return eventIt.return();
479-
},
480-
};
481-
yield* iterator;
446+
try {
447+
// eslint-disable-next-line no-unused-vars
448+
for await (const event of eventIt) {
449+
yield result;
450+
}
451+
} finally {
452+
abortListener?.[SymbolDispose]();
453+
this.#clearInterval(timer);
454+
}
482455
}
483456

484457
#setImmediate(callback, ...args) {
@@ -490,38 +463,31 @@ class MockTimers {
490463
);
491464
}
492465

493-
#promisifyTimer({ timerFn, clearFn, ms, result, options }) {
494-
return new Promise((resolve, reject) => {
495-
if (options?.signal) {
496-
try {
497-
validateAbortSignal(options.signal, 'options.signal');
498-
} catch (err) {
499-
return reject(err);
500-
}
501-
502-
if (options.signal.aborted) {
503-
return reject(abortIt(options.signal));
504-
}
505-
}
466+
async #promisifyTimer({ timerFn, clearFn, ms, result, options }) {
467+
const { promise, resolve, reject } = PromiseWithResolvers();
468+
469+
let abortListener;
470+
if (options?.signal) {
471+
validateAbortSignal(options.signal, 'options.signal');
506472

507-
const onabort = () => {
508-
clearFn(timer);
509-
return reject(abortIt(options.signal));
510-
};
511-
512-
const timer = timerFn(() => {
513-
return resolve(result);
514-
}, ms);
515-
516-
if (options?.signal) {
517-
kResistStopPropagation ??= require('internal/event_target').kResistStopPropagation;
518-
options.signal.addEventListener('abort', onabort, {
519-
__proto__: null,
520-
once: true,
521-
[kResistStopPropagation]: true,
522-
});
473+
if (options.signal.aborted) {
474+
throw abortIt(options.signal);
523475
}
524-
});
476+
477+
abortListener = addAbortListener(options.signal, () => {
478+
reject(abortIt(options.signal));
479+
});
480+
}
481+
482+
const timer = timerFn(resolve, ms);
483+
484+
try {
485+
await promise;
486+
return result;
487+
} finally {
488+
abortListener?.[SymbolDispose]();
489+
clearFn(timer);
490+
}
525491
}
526492

527493
#setImmediatePromisified(result, options) {

test/parallel/test-runner-mock-timers.js

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ process.env.NODE_TEST_KNOWN_GLOBALS = 0;
44
const common = require('../common');
55

66
const assert = require('node:assert');
7+
const { getEventListeners } = require('node:events');
78
const { it, mock, describe } = require('node:test');
89
const nodeTimers = require('node:timers');
910
const nodeTimersPromises = require('node:timers/promises');
@@ -422,6 +423,8 @@ describe('Mock Timers Test Suite', () => {
422423
});
423424

424425
describe('timers/promises', () => {
426+
const hasAbortListener = (signal) => !!getEventListeners(signal, 'abort').length;
427+
425428
describe('setTimeout Suite', () => {
426429
it('should advance in time and trigger timers when calling the .tick function multiple times', async (t) => {
427430
t.mock.timers.enable({ apis: ['setTimeout'] });
@@ -515,6 +518,22 @@ describe('Mock Timers Test Suite', () => {
515518
});
516519
});
517520

521+
it('should clear the abort listener when the timer resolves', async (t) => {
522+
t.mock.timers.enable({ apis: ['setTimeout'] });
523+
const expectedResult = 'result';
524+
const controller = new AbortController();
525+
const p = nodeTimersPromises.setTimeout(500, expectedResult, {
526+
ref: true,
527+
signal: controller.signal,
528+
});
529+
530+
assert(hasAbortListener(controller.signal));
531+
532+
t.mock.timers.tick(500);
533+
await p;
534+
assert(!hasAbortListener(controller.signal));
535+
});
536+
518537
it('should reject given an an invalid signal instance', async (t) => {
519538
t.mock.timers.enable({ apis: ['setTimeout'] });
520539
const expectedResult = 'result';
@@ -728,6 +747,23 @@ describe('Mock Timers Test Suite', () => {
728747
});
729748
});
730749

750+
it('should clear the abort listener when the interval returns', async (t) => {
751+
t.mock.timers.enable({ apis: ['setInterval'] });
752+
753+
const abortController = new AbortController();
754+
const intervalIterator = nodeTimersPromises.setInterval(1, Date.now(), {
755+
signal: abortController.signal,
756+
});
757+
758+
const first = intervalIterator.next();
759+
t.mock.timers.tick();
760+
761+
await first;
762+
assert(hasAbortListener(abortController.signal));
763+
await intervalIterator.return();
764+
assert(!hasAbortListener(abortController.signal));
765+
});
766+
731767
it('should abort operation given an abort controller signal on a real use case', async (t) => {
732768
t.mock.timers.enable({ apis: ['setInterval'] });
733769
const controller = new AbortController();

0 commit comments

Comments
 (0)