From c1957ecd7c08d76dfb3c33ee6233a782cf18230f Mon Sep 17 00:00:00 2001 From: Michael Leaney Date: Wed, 11 Oct 2017 20:29:27 +0800 Subject: [PATCH 1/3] Add test --- spec/core/ClockSpec.js | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/spec/core/ClockSpec.js b/spec/core/ClockSpec.js index 6b3ed1d0..350dc85e 100644 --- a/spec/core/ClockSpec.js +++ b/spec/core/ClockSpec.js @@ -679,4 +679,25 @@ describe("Clock (acceptance)", function() { expect(actualTimes).toEqual([baseTime.getTime(), baseTime.getTime() + 1, baseTime.getTime() + 3]); }) + + it('should be able to clear a timeout', function () { + var delayedFunctionScheduler = new jasmineUnderTest.DelayedFunctionScheduler(), + global = {Date: Date, setTimeout: undefined}, + mockDate = new jasmineUnderTest.MockDate(global), + clock = new jasmineUnderTest.Clock(global, function () { return delayedFunctionScheduler; }, mockDate); + + clock.install(); + + var timerId2; + + global.setTimeout(function () { + global.clearTimeout(timerId2); + }, 100); + + timerId2 = global.setTimeout(() => { + fail(); + }, 100); + + clock.tick(100); + }); }); From 516e00d7ba9cd5f3397d6e506241e726acefef7c Mon Sep 17 00:00:00 2001 From: Michael Leaney Date: Wed, 11 Oct 2017 20:54:56 +0800 Subject: [PATCH 2/3] Fix #1426 clearTimeout not correctly clearing a timeout clearTimeout was not correctly handling the case of clearing a timeout that is also scheduled to run at the same tick. This fix adds a deletedKeys array that is checked whilst we are running the scheduled functions for the current clock tick. If a function exists in deletedKeys it will not be ran. deletedKeys is then reset to an empty array. --- spec/core/ClockSpec.js | 24 ++++++++++++++++++++---- src/core/DelayedFunctionScheduler.js | 13 ++++++++++++- 2 files changed, 32 insertions(+), 5 deletions(-) diff --git a/spec/core/ClockSpec.js b/spec/core/ClockSpec.js index 350dc85e..37c1bc33 100644 --- a/spec/core/ClockSpec.js +++ b/spec/core/ClockSpec.js @@ -680,7 +680,7 @@ describe("Clock (acceptance)", function() { expect(actualTimes).toEqual([baseTime.getTime(), baseTime.getTime() + 1, baseTime.getTime() + 3]); }) - it('should be able to clear a timeout', function () { + it('correctly clears a scheduled timeout while the Clock is advancing', function () { var delayedFunctionScheduler = new jasmineUnderTest.DelayedFunctionScheduler(), global = {Date: Date, setTimeout: undefined}, mockDate = new jasmineUnderTest.MockDate(global), @@ -694,10 +694,26 @@ describe("Clock (acceptance)", function() { global.clearTimeout(timerId2); }, 100); - timerId2 = global.setTimeout(() => { - fail(); - }, 100); + timerId2 = global.setTimeout(fail, 100); clock.tick(100); }); + + it('correctly clears a scheduled interval while the Clock is advancing', function () { + var delayedFunctionScheduler = new jasmineUnderTest.DelayedFunctionScheduler(), + global = {Date: Date, setTimeout: undefined}, + mockDate = new jasmineUnderTest.MockDate(global), + clock = new jasmineUnderTest.Clock(global, function () { return delayedFunctionScheduler; }, mockDate); + + clock.install(); + + var timerId2; + var timerId1 = global.setInterval(function () { + global.clearInterval(timerId2); + }, 100); + + timerId2 = global.setInterval(fail, 100); + + clock.tick(400); + }); }); diff --git a/src/core/DelayedFunctionScheduler.js b/src/core/DelayedFunctionScheduler.js index 4756b323..91557a37 100644 --- a/src/core/DelayedFunctionScheduler.js +++ b/src/core/DelayedFunctionScheduler.js @@ -5,6 +5,7 @@ getJasmineRequireObj().DelayedFunctionScheduler = function() { var scheduledFunctions = {}; var currentTime = 0; var delayedFnCount = 0; + var deletedKeys = []; self.tick = function(millis, tickDate) { millis = millis || 0; @@ -51,6 +52,8 @@ getJasmineRequireObj().DelayedFunctionScheduler = function() { }; self.removeFunctionWithId = function(timeoutKey) { + deletedKeys.push(timeoutKey); + for (var runAtMillis in scheduledFunctions) { var funcs = scheduledFunctions[runAtMillis]; var i = indexOfFirstToPass(funcs, function (func) { @@ -126,7 +129,10 @@ getJasmineRequireObj().DelayedFunctionScheduler = function() { currentTime = newCurrentTime; - var funcsToRun = scheduledFunctions[currentTime]; + var funcsToRun = scheduledFunctions[currentTime].sort(function (a, b) { + return a.millis > b.millis; + }); + delete scheduledFunctions[currentTime]; forEachFunction(funcsToRun, function(funcToRun) { @@ -136,8 +142,13 @@ getJasmineRequireObj().DelayedFunctionScheduler = function() { }); forEachFunction(funcsToRun, function(funcToRun) { + if (deletedKeys.indexOf(funcToRun.timeoutKey) !== -1) { + // skip a timeoutKey deleted whilst we were running + return; + } funcToRun.funcToCall.apply(null, funcToRun.params || []); }); + deletedKeys = []; } while (scheduledLookup.length > 0 && // checking first if we're out of time prevents setTimeout(0) // scheduled in a funcToRun from forcing an extra iteration From 1136fddcde1f327c8c895b65275f45704bbc0107 Mon Sep 17 00:00:00 2001 From: Michael Leaney Date: Tue, 2 Jan 2018 14:51:51 +0800 Subject: [PATCH 3/3] Remove .sort() and fix logic in test --- spec/core/DelayedFunctionSchedulerSpec.js | 18 ++++++++++-------- src/core/DelayedFunctionScheduler.js | 4 +--- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/spec/core/DelayedFunctionSchedulerSpec.js b/spec/core/DelayedFunctionSchedulerSpec.js index c26252ef..fa846d6f 100644 --- a/spec/core/DelayedFunctionSchedulerSpec.js +++ b/spec/core/DelayedFunctionSchedulerSpec.js @@ -216,21 +216,23 @@ describe("DelayedFunctionScheduler", function() { it("removes functions during a tick that runs the function", function() { var scheduler = new jasmineUnderTest.DelayedFunctionScheduler(), - fn = jasmine.createSpy('fn'), + spy = jasmine.createSpy('fn'), + spyAndRemove = jasmine.createSpy('fn'), fnDelay = 10, timeoutKey; - timeoutKey = scheduler.scheduleFunction(fn, fnDelay, [], true); - scheduler.scheduleFunction(function () { + spyAndRemove.and.callFake(function() { scheduler.removeFunctionWithId(timeoutKey); - }, 2 * fnDelay); + }); - expect(fn).not.toHaveBeenCalled(); + scheduler.scheduleFunction(spyAndRemove, fnDelay); - scheduler.tick(3 * fnDelay); + timeoutKey = scheduler.scheduleFunction(spy, fnDelay, [], true); - expect(fn).toHaveBeenCalled(); - expect(fn.calls.count()).toBe(2); + scheduler.tick(2 * fnDelay); + + expect(spy).not.toHaveBeenCalled(); + expect(spyAndRemove).toHaveBeenCalled(); }); it("removes functions during the first tick that runs the function", function() { diff --git a/src/core/DelayedFunctionScheduler.js b/src/core/DelayedFunctionScheduler.js index 91557a37..73001cc3 100644 --- a/src/core/DelayedFunctionScheduler.js +++ b/src/core/DelayedFunctionScheduler.js @@ -129,9 +129,7 @@ getJasmineRequireObj().DelayedFunctionScheduler = function() { currentTime = newCurrentTime; - var funcsToRun = scheduledFunctions[currentTime].sort(function (a, b) { - return a.millis > b.millis; - }); + var funcsToRun = scheduledFunctions[currentTime]; delete scheduledFunctions[currentTime];