From 1a23be5a49a84d3aed7e580dc79aaca5ad421954 Mon Sep 17 00:00:00 2001 From: Mika Fischer Date: Wed, 25 Oct 2023 10:50:11 +0200 Subject: [PATCH 1/5] test_runner: add tests for various mock timer issues --- test/parallel/test-runner-mock-timers.js | 85 ++++++++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/test/parallel/test-runner-mock-timers.js b/test/parallel/test-runner-mock-timers.js index f87ac8097c4e27..c031de50391359 100644 --- a/test/parallel/test-runner-mock-timers.js +++ b/test/parallel/test-runner-mock-timers.js @@ -479,6 +479,59 @@ describe('Mock Timers Test Suite', () => { code: 'ERR_INVALID_ARG_TYPE', }); }); + + // Test for https://github.com/nodejs/node/issues/50365 + it('should not affect other timers when aborting', async (t) => { + const f1 = t.mock.fn(); + const f2 = t.mock.fn(); + t.mock.timers.enable({ apis: ['setTimeout'] }); + const ac = new AbortController(); + + // id 1 & pos 1 in priority queue + nodeTimersPromises.setTimeout(100, undefined, { signal: ac.signal }).then(f1, f1); + // id 2 & pos 1 in priority queue (id 1 is moved to pos 2) + nodeTimersPromises.setTimeout(50).then(f2, f2); + + ac.abort(); // BUG: will remove timer at pos 1 not timer with id 1! + + t.mock.timers.runAll(); + await nodeTimersPromises.setImmediate(); // let promises settle + + // First setTimeout is aborted + assert.strictEqual(f1.mock.callCount(), 1); + assert.strictEqual(f1.mock.calls[0].arguments[0].code, 'ABORT_ERR'); + + // Second setTimeout should resolve, but never settles, because it was eronously removed by ac.abort() + assert.strictEqual(f2.mock.callCount(), 1); + }); + + // Test for https://github.com/nodejs/node/issues/50365 + it('should not affect other timers when aborted after triggering', async (t) => { + const f1 = t.mock.fn(); + const f2 = t.mock.fn(); + t.mock.timers.enable({ apis: ['setTimeout'] }); + const ac = new AbortController(); + + // id 1 & pos 1 in priority queue + nodeTimersPromises.setTimeout(50, true, { signal: ac.signal }).then(f1, f1); + // id 2 & pos 2 in priority queue + nodeTimersPromises.setTimeout(100).then(f2, f2); + + // First setTimeout resolves + t.mock.timers.tick(50); + await nodeTimersPromises.setImmediate(); // let promises settle + assert.strictEqual(f1.mock.callCount(), 1); + assert.strictEqual(f1.mock.calls[0].arguments.length, 1); + assert.strictEqual(f1.mock.calls[0].arguments[0], true); + + // Now timer with id 2 will be at pos 1 in priority queue + ac.abort(); // BUG: will remove timer at pos 1 not timer with id 1! + + // Second setTimeout should resolve, but never settles, because it was eronously removed by ac.abort() + t.mock.timers.runAll(); + await nodeTimersPromises.setImmediate(); // let promises settle + assert.strictEqual(f2.mock.callCount(), 1); + }); }); describe('setInterval Suite', () => { @@ -626,6 +679,38 @@ describe('Mock Timers Test Suite', () => { }); assert.strictEqual(numIterations, expectedIterations); }); + + // Test for https://github.com/nodejs/node/issues/50381 + it('should use the correct interval', (t) => { + t.mock.timers.enable({ apis: ['setInterval'] }); + const fn = t.mock.fn(); + setInterval(fn, 1000); + assert.strictEqual(fn.mock.callCount(), 0); + t.mock.timers.tick(1000); + assert.strictEqual(fn.mock.callCount(), 1); + t.mock.timers.tick(1); + t.mock.timers.tick(1); + t.mock.timers.tick(1); + assert.strictEqual(fn.mock.callCount(), 1); + }); + + // Test for https://github.com/nodejs/node/issues/50382 + it('should not prevent due timers to be processed', async (t) => { + t.mock.timers.enable({ apis: ['setInterval', 'setTimeout'] }); + const f1 = t.mock.fn(); + const f2 = t.mock.fn(); + + setInterval(f1, 1000); + setTimeout(f2, 1001); + + assert.strictEqual(f1.mock.callCount(), 0); + assert.strictEqual(f2.mock.callCount(), 0); + + t.mock.timers.tick(1001); + + assert.strictEqual(f1.mock.callCount(), 1); + assert.strictEqual(f2.mock.callCount(), 1); + }); }); }); }); From c087b39f76b91dd1e8760b50d38652079e665332 Mon Sep 17 00:00:00 2001 From: Mika Fischer Date: Tue, 24 Oct 2023 19:46:03 +0200 Subject: [PATCH 2/5] test_runner: fix tick processing terminating prematurely An incorrect early return caused processing of due timers to stop prematurely. Fixes: https://github.com/nodejs/node/issues/50382 --- lib/internal/test_runner/mock/mock_timers.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/internal/test_runner/mock/mock_timers.js b/lib/internal/test_runner/mock/mock_timers.js index 8fac645e9a3617..94f9f00dee7c11 100644 --- a/lib/internal/test_runner/mock/mock_timers.js +++ b/lib/internal/test_runner/mock/mock_timers.js @@ -624,7 +624,6 @@ class MockTimers { if (timer.interval) { timer.runAt += timer.interval; this.#executionQueue.insert(timer); - return; } timer = this.#executionQueue.peek(); From 73ec10436778b2f5cc58472f7cd59520dcf0b6d1 Mon Sep 17 00:00:00 2001 From: Mika Fischer Date: Tue, 24 Oct 2023 19:47:47 +0200 Subject: [PATCH 3/5] test_runner: fix mocked setInterval using the wrong interval mocked setInterval used the wrong increment when scheduling the new timer. Fixes: https://github.com/nodejs/node/issues/50382 --- lib/internal/test_runner/mock/mock_timers.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/internal/test_runner/mock/mock_timers.js b/lib/internal/test_runner/mock/mock_timers.js index 94f9f00dee7c11..407a671eda7a8c 100644 --- a/lib/internal/test_runner/mock/mock_timers.js +++ b/lib/internal/test_runner/mock/mock_timers.js @@ -265,7 +265,7 @@ class MockTimers { id: timerId, callback, runAt: this.#now + delay, - interval: isInterval, + interval: isInterval ? delay : undefined, args, }); @@ -621,7 +621,7 @@ class MockTimers { this.#executionQueue.shift(); - if (timer.interval) { + if (timer.interval !== undefined) { timer.runAt += timer.interval; this.#executionQueue.insert(timer); } From 22fbdd3f06222187511504001a984c6ada8ac6d5 Mon Sep 17 00:00:00 2001 From: Mika Fischer Date: Wed, 25 Oct 2023 09:52:39 +0200 Subject: [PATCH 4/5] test_runner: fix incorrect cleanup of timers Removal of mocket timers from the priority queue was broken. It used the timerId instead of the position in the queue as index. This lead to removal of incorrect timers from the queue causing timers not to be scheduled at all. Also, aborts caused removal from the queue even when the timer was already triggered, and thus no longer present in the queue. Fixes: https://github.com/nodejs/node/issues/50365 --- lib/internal/test_runner/mock/mock_timers.js | 24 ++++++++++++-------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/lib/internal/test_runner/mock/mock_timers.js b/lib/internal/test_runner/mock/mock_timers.js index 407a671eda7a8c..85512cc4b66a51 100644 --- a/lib/internal/test_runner/mock/mock_timers.js +++ b/lib/internal/test_runner/mock/mock_timers.js @@ -260,20 +260,23 @@ class MockTimers { #createTimer(isInterval, callback, delay, ...args) { const timerId = this.#currentTimer++; - this.#executionQueue.insert({ + const timer = { __proto__: null, id: timerId, callback, runAt: this.#now + delay, interval: isInterval ? delay : undefined, args, - }); - - return timerId; + }; + this.#executionQueue.insert(timer); + return timer; } - #clearTimer(position) { - this.#executionQueue.removeAt(position); + #clearTimer(timer) { + if (timer.priorityQueuePosition !== undefined) { + this.#executionQueue.removeAt(timer.priorityQueuePosition); + timer.priorityQueuePosition = undefined; + } } #createDate() { @@ -397,10 +400,10 @@ class MockTimers { emitter.emit('data', result); }; - const timerId = this.#createTimer(true, callback, interval, options); + const timer = this.#createTimer(true, callback, interval, options); const clearListeners = () => { emitter.removeAllListeners(); - context.#clearTimer(timerId); + context.#clearTimer(timer); }; const iterator = { __proto__: null, @@ -453,11 +456,11 @@ class MockTimers { } const onabort = () => { - clearFn(id); + clearFn(timer); return reject(abortIt(options.signal)); }; - const id = timerFn(() => { + const timer = timerFn(() => { return resolve(result); }, ms); @@ -620,6 +623,7 @@ class MockTimers { FunctionPrototypeApply(timer.callback, undefined, timer.args); this.#executionQueue.shift(); + timer.priorityQueuePosition = undefined; if (timer.interval !== undefined) { timer.runAt += timer.interval; From 1f91c1fed83b62f0bb30b4224d5ea9f2728d01fb Mon Sep 17 00:00:00 2001 From: Mika Fischer Date: Fri, 27 Oct 2023 10:15:21 +0200 Subject: [PATCH 5/5] Update test/parallel/test-runner-mock-timers.js Co-authored-by: Raz Luvaton <16746759+rluvaton@users.noreply.github.com> --- test/parallel/test-runner-mock-timers.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-runner-mock-timers.js b/test/parallel/test-runner-mock-timers.js index c031de50391359..72ddb3ad7421f0 100644 --- a/test/parallel/test-runner-mock-timers.js +++ b/test/parallel/test-runner-mock-timers.js @@ -681,7 +681,7 @@ describe('Mock Timers Test Suite', () => { }); // Test for https://github.com/nodejs/node/issues/50381 - it('should use the correct interval', (t) => { + it('should use the mocked interval', (t) => { t.mock.timers.enable({ apis: ['setInterval'] }); const fn = t.mock.fn(); setInterval(fn, 1000);