From 0dd8f02d9d79d6e2f4a9dabc9c712168f05a2d10 Mon Sep 17 00:00:00 2001 From: Nicholas Jamieson Date: Sun, 14 Nov 2021 07:25:06 +1000 Subject: [PATCH 1/7] chore: use sinon sandbox consistently --- spec/schedulers/AsapScheduler-spec.ts | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/spec/schedulers/AsapScheduler-spec.ts b/spec/schedulers/AsapScheduler-spec.ts index 2b1070e052..1c90daeaec 100644 --- a/spec/schedulers/AsapScheduler-spec.ts +++ b/spec/schedulers/AsapScheduler-spec.ts @@ -39,9 +39,10 @@ describe('Scheduler.asap', () => { it('should cancel asap actions when delay > 0', () => { testScheduler.run(({ cold, expectObservable, flush, time }) => { - const setImmediateSpy = sinon.spy(immediateProvider, 'setImmediate'); - const setSpy = sinon.spy(intervalProvider, 'setInterval'); - const clearSpy = sinon.spy(intervalProvider, 'clearInterval'); + const sandbox = sinon.createSandbox(); + const setImmediateSpy = sandbox.spy(immediateProvider, 'setImmediate'); + const setSpy = sandbox.spy(intervalProvider, 'setInterval'); + const clearSpy = sandbox.spy(intervalProvider, 'clearInterval'); const a = cold(' a '); const ta = time(' ----| '); @@ -57,9 +58,7 @@ describe('Scheduler.asap', () => { expect(setImmediateSpy).to.have.not.been.called; expect(setSpy).to.have.been.calledOnce; expect(clearSpy).to.have.been.calledOnce; - setImmediateSpy.restore(); - setSpy.restore(); - clearSpy.restore(); + sandbox.restore(); }); }); @@ -67,7 +66,7 @@ describe('Scheduler.asap', () => { const sandbox = sinon.createSandbox(); const fakeTimer = sandbox.useFakeTimers(); // callThrough is missing from the declarations installed by the typings tool in stable - const stubSetInterval = ( sinon.stub(global, 'setInterval')).callThrough(); + const stubSetInterval = ( sandbox.stub(global, 'setInterval')).callThrough(); const period = 50; const state = { index: 0, period }; type State = typeof state; @@ -86,7 +85,6 @@ describe('Scheduler.asap', () => { fakeTimer.tick(period); expect(state).to.have.property('index', 2); expect(stubSetInterval).to.have.property('callCount', 1); - stubSetInterval.restore(); sandbox.restore(); }); @@ -94,7 +92,7 @@ describe('Scheduler.asap', () => { const sandbox = sinon.createSandbox(); const fakeTimer = sandbox.useFakeTimers(); // callThrough is missing from the declarations installed by the typings tool in stable - const stubSetInterval = ( sinon.stub(global, 'setInterval')).callThrough(); + const stubSetInterval = ( sandbox.stub(global, 'setInterval')).callThrough(); const period = 50; const state = { index: 0, period }; type State = typeof state; @@ -114,7 +112,6 @@ describe('Scheduler.asap', () => { fakeTimer.tick(period); expect(state).to.have.property('index', 2); expect(stubSetInterval).to.have.property('callCount', 3); - stubSetInterval.restore(); sandbox.restore(); }); From 67e2c83b0453a4ebd1414186c1e621746d8f3ed4 Mon Sep 17 00:00:00 2001 From: Nicholas Jamieson Date: Sun, 14 Nov 2021 07:28:19 +1000 Subject: [PATCH 2/7] test: add failing flush tests --- .../AnimationFrameScheduler-spec.ts | 50 ++++++++++++++++++- spec/schedulers/AsapScheduler-spec.ts | 46 +++++++++++++++++ 2 files changed, 94 insertions(+), 2 deletions(-) diff --git a/spec/schedulers/AnimationFrameScheduler-spec.ts b/spec/schedulers/AnimationFrameScheduler-spec.ts index e3d6b4b56f..0a80e0c882 100644 --- a/spec/schedulers/AnimationFrameScheduler-spec.ts +++ b/spec/schedulers/AnimationFrameScheduler-spec.ts @@ -1,6 +1,6 @@ import { expect } from 'chai'; import * as sinon from 'sinon'; -import { animationFrameScheduler, Subscription, merge } from 'rxjs'; +import { animationFrameScheduler, Subscription, merge, animationFrames } from 'rxjs'; import { delay } from 'rxjs/operators'; import { TestScheduler } from 'rxjs/testing'; import { observableMatcher } from '../helpers/observableMatcher'; @@ -43,7 +43,7 @@ describe('Scheduler.animationFrame', () => { const requestSpy = sinon.spy(animationFrameProvider, 'requestAnimationFrame'); const setSpy = sinon.spy(intervalProvider, 'setInterval'); const clearSpy = sinon.spy(intervalProvider, 'clearInterval'); - + animate(' ----------x--'); const a = cold(' a '); const ta = time(' ----| '); @@ -168,4 +168,50 @@ describe('Scheduler.animationFrame', () => { }, 0, 0); scheduledIndices.push(0); }); + + it('should execute actions scheduled when flushing in a subsequent flush', (done) => { + const sandbox = sinon.createSandbox(); + const stubFlush = (sandbox.stub(animationFrameScheduler, 'flush')).callThrough(); + + let a: Subscription; + let b: Subscription; + let c: Subscription; + + a = animationFrameScheduler.schedule(() => { + expect(stubFlush).to.have.callCount(1); + c = animationFrameScheduler.schedule(() => { + expect(stubFlush).to.have.callCount(2); + sandbox.restore(); + done(); + }); + }); + b = animationFrameScheduler.schedule(() => { + expect(stubFlush).to.have.callCount(1); + }); + }); + + it('should execute actions scheduled when flushing in a subsequent flush when some actions are unsubscribed', (done) => { + const sandbox = sinon.createSandbox(); + const stubFlush = (sandbox.stub(animationFrameScheduler, 'flush')).callThrough(); + + let a: Subscription; + let b: Subscription; + let c: Subscription; + + a = animationFrameScheduler.schedule(() => { + expect(stubFlush).to.have.callCount(1); + c = animationFrameScheduler.schedule(() => { + expect(stubFlush).to.have.callCount(2); + sandbox.restore(); + done(); + }); + b.unsubscribe(); + }); + b = animationFrameScheduler.schedule(() => { + expect(stubFlush).to.have.callCount(1); + }); + }); + + it.skip('should properly cancel an unnecessary flush', (done) => { + }); }); diff --git a/spec/schedulers/AsapScheduler-spec.ts b/spec/schedulers/AsapScheduler-spec.ts index 1c90daeaec..846860747b 100644 --- a/spec/schedulers/AsapScheduler-spec.ts +++ b/spec/schedulers/AsapScheduler-spec.ts @@ -218,4 +218,50 @@ describe('Scheduler.asap', () => { }, 0, 0); scheduledIndices.push(0); }); + + it('should execute actions scheduled when flushing in a subsequent flush', (done) => { + const sandbox = sinon.createSandbox(); + const stubFlush = (sandbox.stub(asapScheduler, 'flush')).callThrough(); + + let a: Subscription; + let b: Subscription; + let c: Subscription; + + a = asapScheduler.schedule(() => { + expect(stubFlush).to.have.callCount(1); + c = asapScheduler.schedule(() => { + expect(stubFlush).to.have.callCount(2); + sandbox.restore(); + done(); + }); + }); + b = asapScheduler.schedule(() => { + expect(stubFlush).to.have.callCount(1); + }); + }); + + it('should execute actions scheduled when flushing in a subsequent flush when some actions are unsubscribed', (done) => { + const sandbox = sinon.createSandbox(); + const stubFlush = (sandbox.stub(asapScheduler, 'flush')).callThrough(); + + let a: Subscription; + let b: Subscription; + let c: Subscription; + + a = asapScheduler.schedule(() => { + expect(stubFlush).to.have.callCount(1); + c = asapScheduler.schedule(() => { + expect(stubFlush).to.have.callCount(2); + sandbox.restore(); + done(); + }); + b.unsubscribe(); + }); + b = asapScheduler.schedule(() => { + expect(stubFlush).to.have.callCount(1); + }); + }); + + it.skip('should properly cancel an unnecessary flush', (done) => { + }); }); From fbaab9fc5de291f903042ec942bd55ea87f8d2aa Mon Sep 17 00:00:00 2001 From: Nicholas Jamieson Date: Sun, 14 Nov 2021 07:52:50 +1000 Subject: [PATCH 3/7] fix: don't execute actions scheduled within flush --- .../scheduler/AnimationFrameScheduler.ts | 16 ++++++++++++---- src/internal/scheduler/AsapScheduler.ts | 16 ++++++++++++---- 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/src/internal/scheduler/AnimationFrameScheduler.ts b/src/internal/scheduler/AnimationFrameScheduler.ts index 888b603553..640afa2488 100644 --- a/src/internal/scheduler/AnimationFrameScheduler.ts +++ b/src/internal/scheduler/AnimationFrameScheduler.ts @@ -4,24 +4,32 @@ import { AsyncScheduler } from './AsyncScheduler'; export class AnimationFrameScheduler extends AsyncScheduler { public flush(action?: AsyncAction): void { this._active = true; + // The async id that effects a call to flush is stored in _scheduled. + // Before executing an action, it's necessary to check the action's async + // id to determine whether it's supposed to be executed in the current + // flush. + // Previous implementations of this method used a count to determine this, + // but that was unsound, as actions that are unsubscribed - i.e. cancelled - + // are removed from the actions array and that can shift actions that are + // scheduled to be executed in a subsequent flush into positions at which + // they are executed within the current flush. + const flushId = this._scheduled; this._scheduled = undefined; const { actions } = this; let error: any; - let index = -1; action = action || actions.shift()!; - const count = actions.length; do { if ((error = action.execute(action.state, action.delay))) { break; } - } while (++index < count && (action = actions.shift())); + } while ((action = actions[0]) && action.id === flushId && actions.shift()); this._active = false; if (error) { - while (++index < count && (action = actions.shift())) { + while ((action = actions[0]) && action.id === flushId && actions.shift()) { action.unsubscribe(); } throw error; diff --git a/src/internal/scheduler/AsapScheduler.ts b/src/internal/scheduler/AsapScheduler.ts index 55408bc4ea..95874bda2e 100644 --- a/src/internal/scheduler/AsapScheduler.ts +++ b/src/internal/scheduler/AsapScheduler.ts @@ -4,24 +4,32 @@ import { AsyncScheduler } from './AsyncScheduler'; export class AsapScheduler extends AsyncScheduler { public flush(action?: AsyncAction): void { this._active = true; + // The async id that effects a call to flush is stored in _scheduled. + // Before executing an action, it's necessary to check the action's async + // id to determine whether it's supposed to be executed in the current + // flush. + // Previous implementations of this method used a count to determine this, + // but that was unsound, as actions that are unsubscribed - i.e. cancelled - + // are removed from the actions array and that can shift actions that are + // scheduled to be executed in a subsequent flush into positions at which + // they are executed within the current flush. + const flushId = this._scheduled; this._scheduled = undefined; const { actions } = this; let error: any; - let index = -1; action = action || actions.shift()!; - const count = actions.length; do { if ((error = action.execute(action.state, action.delay))) { break; } - } while (++index < count && (action = actions.shift())); + } while ((action = actions[0]) && action.id === flushId && actions.shift()); this._active = false; if (error) { - while (++index < count && (action = actions.shift())) { + while ((action = actions[0]) && action.id === flushId && actions.shift()) { action.unsubscribe(); } throw error; From 54688d44f6da1ded311e3710e648393627553ae0 Mon Sep 17 00:00:00 2001 From: Nicholas Jamieson Date: Sun, 14 Nov 2021 08:24:15 +1000 Subject: [PATCH 4/7] test: add failing tests --- .../AnimationFrameScheduler-spec.ts | 23 ++++++++++++++++++- spec/schedulers/AsapScheduler-spec.ts | 23 ++++++++++++++++++- 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/spec/schedulers/AnimationFrameScheduler-spec.ts b/spec/schedulers/AnimationFrameScheduler-spec.ts index 0a80e0c882..df64eb53d0 100644 --- a/spec/schedulers/AnimationFrameScheduler-spec.ts +++ b/spec/schedulers/AnimationFrameScheduler-spec.ts @@ -212,6 +212,27 @@ describe('Scheduler.animationFrame', () => { }); }); - it.skip('should properly cancel an unnecessary flush', (done) => { + it('should properly cancel an unnecessary flush', (done) => { + const sandbox = sinon.createSandbox(); + const cancelAnimationFrameStub = sandbox.stub(animationFrameProvider, 'cancelAnimationFrame').callThrough(); + + let a: Subscription; + let b: Subscription; + let c: Subscription; + + a = animationFrameScheduler.schedule(() => { + expect(animationFrameScheduler.actions).to.have.length(1); + c = animationFrameScheduler.schedule(() => { /* stupid lint rule */ }); + expect(animationFrameScheduler.actions).to.have.length(2); + // What we're testing here is that the unsubscription of action c effects + // the cancellation of the animation frame in a scenario in which the + // actions queue is not empty - it contains action b. + c.unsubscribe(); + expect(animationFrameScheduler.actions).to.have.length(1); + expect(cancelAnimationFrameStub).to.have.callCount(1); + sandbox.restore(); + done(); + }); + b = animationFrameScheduler.schedule(() => { /* stupid lint rule */ }); }); }); diff --git a/spec/schedulers/AsapScheduler-spec.ts b/spec/schedulers/AsapScheduler-spec.ts index 846860747b..1ce8043394 100644 --- a/spec/schedulers/AsapScheduler-spec.ts +++ b/spec/schedulers/AsapScheduler-spec.ts @@ -262,6 +262,27 @@ describe('Scheduler.asap', () => { }); }); - it.skip('should properly cancel an unnecessary flush', (done) => { + it('should properly cancel an unnecessary flush', (done) => { + const sandbox = sinon.createSandbox(); + const clearImmediateStub = sandbox.stub(immediateProvider, 'clearImmediate').callThrough(); + + let a: Subscription; + let b: Subscription; + let c: Subscription; + + a = asapScheduler.schedule(() => { + expect(asapScheduler.actions).to.have.length(1); + c = asapScheduler.schedule(() => { /* stupid lint rule */ }); + expect(asapScheduler.actions).to.have.length(2); + // What we're testing here is that the unsubscription of action c effects + // the cancellation of the microtask in a scenario in which the actions + // queue is not empty - it contains action b. + c.unsubscribe(); + expect(asapScheduler.actions).to.have.length(1); + expect(clearImmediateStub).to.have.callCount(1); + sandbox.restore(); + done(); + }); + b = asapScheduler.schedule(() => { /* stupid lint rule */ }); }); }); From fb9abf2ae896388d99faaa2421e9a2eb11ff75f9 Mon Sep 17 00:00:00 2001 From: Nicholas Jamieson Date: Sun, 14 Nov 2021 08:24:29 +1000 Subject: [PATCH 5/7] fix: avoid calling flush with empty actions queue Closes #6672 --- src/internal/scheduler/AnimationFrameAction.ts | 8 ++++---- src/internal/scheduler/AsapAction.ts | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/internal/scheduler/AnimationFrameAction.ts b/src/internal/scheduler/AnimationFrameAction.ts index 1ab54d82f6..771212f73d 100644 --- a/src/internal/scheduler/AnimationFrameAction.ts +++ b/src/internal/scheduler/AnimationFrameAction.ts @@ -27,10 +27,10 @@ export class AnimationFrameAction extends AsyncAction { if ((delay != null && delay > 0) || (delay == null && this.delay > 0)) { return super.recycleAsyncId(scheduler, id, delay); } - // If the scheduler queue is empty, cancel the requested animation frame and - // set the scheduled flag to undefined so the next AnimationFrameAction will - // request its own. - if (scheduler.actions.length === 0) { + // If the scheduler queue has no remaining actions with the same async id, + // cancel the requested animation frame and set the scheduled flag to + // undefined so the next AnimationFrameAction will request its own. + if (!scheduler.actions.some((action) => action.id === id)) { animationFrameProvider.cancelAnimationFrame(id); scheduler._scheduled = undefined; } diff --git a/src/internal/scheduler/AsapAction.ts b/src/internal/scheduler/AsapAction.ts index a9239b0d56..f8f5116e50 100644 --- a/src/internal/scheduler/AsapAction.ts +++ b/src/internal/scheduler/AsapAction.ts @@ -27,10 +27,10 @@ export class AsapAction extends AsyncAction { if ((delay != null && delay > 0) || (delay == null && this.delay > 0)) { return super.recycleAsyncId(scheduler, id, delay); } - // If the scheduler queue is empty, cancel the requested microtask and - // set the scheduled flag to undefined so the next AsapAction will schedule - // its own. - if (scheduler.actions.length === 0) { + // If the scheduler queue has no remaining actions with the same async id, + // cancel the requested microtask and set the scheduled flag to undefined + // so the next AsapAction will request its own. + if (!scheduler.actions.some((action) => action.id === id)) { immediateProvider.clearImmediate(id); scheduler._scheduled = undefined; } From 1f31101535bcc378c36f251adc3e578e2d85dfac Mon Sep 17 00:00:00 2001 From: Nicholas Jamieson Date: Sun, 14 Nov 2021 08:53:58 +1000 Subject: [PATCH 6/7] chore: remove accidental auto-import --- spec/schedulers/AnimationFrameScheduler-spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/schedulers/AnimationFrameScheduler-spec.ts b/spec/schedulers/AnimationFrameScheduler-spec.ts index df64eb53d0..9ad33b78ea 100644 --- a/spec/schedulers/AnimationFrameScheduler-spec.ts +++ b/spec/schedulers/AnimationFrameScheduler-spec.ts @@ -1,6 +1,6 @@ import { expect } from 'chai'; import * as sinon from 'sinon'; -import { animationFrameScheduler, Subscription, merge, animationFrames } from 'rxjs'; +import { animationFrameScheduler, Subscription, merge } from 'rxjs'; import { delay } from 'rxjs/operators'; import { TestScheduler } from 'rxjs/testing'; import { observableMatcher } from '../helpers/observableMatcher'; From b5d0d170963b296528d27e0d126a97ffd3d2c303 Mon Sep 17 00:00:00 2001 From: Nicholas Jamieson Date: Sun, 14 Nov 2021 10:34:06 +1000 Subject: [PATCH 7/7] test: call all the dones --- spec/schedulers/AnimationFrameScheduler-spec.ts | 9 ++++++--- spec/schedulers/AsapScheduler-spec.ts | 9 ++++++--- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/spec/schedulers/AnimationFrameScheduler-spec.ts b/spec/schedulers/AnimationFrameScheduler-spec.ts index 9ad33b78ea..eae794b1cb 100644 --- a/spec/schedulers/AnimationFrameScheduler-spec.ts +++ b/spec/schedulers/AnimationFrameScheduler-spec.ts @@ -208,7 +208,7 @@ describe('Scheduler.animationFrame', () => { b.unsubscribe(); }); b = animationFrameScheduler.schedule(() => { - expect(stubFlush).to.have.callCount(1); + done(new Error('Unexpected execution of b')); }); }); @@ -222,7 +222,9 @@ describe('Scheduler.animationFrame', () => { a = animationFrameScheduler.schedule(() => { expect(animationFrameScheduler.actions).to.have.length(1); - c = animationFrameScheduler.schedule(() => { /* stupid lint rule */ }); + c = animationFrameScheduler.schedule(() => { + done(new Error('Unexpected execution of c')); + }); expect(animationFrameScheduler.actions).to.have.length(2); // What we're testing here is that the unsubscription of action c effects // the cancellation of the animation frame in a scenario in which the @@ -230,9 +232,10 @@ describe('Scheduler.animationFrame', () => { c.unsubscribe(); expect(animationFrameScheduler.actions).to.have.length(1); expect(cancelAnimationFrameStub).to.have.callCount(1); + }); + b = animationFrameScheduler.schedule(() => { sandbox.restore(); done(); }); - b = animationFrameScheduler.schedule(() => { /* stupid lint rule */ }); }); }); diff --git a/spec/schedulers/AsapScheduler-spec.ts b/spec/schedulers/AsapScheduler-spec.ts index 1ce8043394..54b55349eb 100644 --- a/spec/schedulers/AsapScheduler-spec.ts +++ b/spec/schedulers/AsapScheduler-spec.ts @@ -258,7 +258,7 @@ describe('Scheduler.asap', () => { b.unsubscribe(); }); b = asapScheduler.schedule(() => { - expect(stubFlush).to.have.callCount(1); + done(new Error('Unexpected execution of b')); }); }); @@ -272,7 +272,9 @@ describe('Scheduler.asap', () => { a = asapScheduler.schedule(() => { expect(asapScheduler.actions).to.have.length(1); - c = asapScheduler.schedule(() => { /* stupid lint rule */ }); + c = asapScheduler.schedule(() => { + done(new Error('Unexpected execution of c')); + }); expect(asapScheduler.actions).to.have.length(2); // What we're testing here is that the unsubscription of action c effects // the cancellation of the microtask in a scenario in which the actions @@ -280,9 +282,10 @@ describe('Scheduler.asap', () => { c.unsubscribe(); expect(asapScheduler.actions).to.have.length(1); expect(clearImmediateStub).to.have.callCount(1); + }); + b = asapScheduler.schedule(() => { sandbox.restore(); done(); }); - b = asapScheduler.schedule(() => { /* stupid lint rule */ }); }); });