Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit e35f589

Browse filesBrowse files
cartantcartant
authored
fix: schedulers will no longer error while rescheduling and unsubscribing during flushes
* chore: use sinon sandbox consistently * test: add failing flush tests * fix: don't execute actions scheduled within flush * test: add failing tests * fix: avoid calling flush with empty actions queue Closes #6672 * chore: remove accidental auto-import * test: call all the dones
1 parent c2b3e88 commit e35f589
Copy full SHA for e35f589

File tree

Expand file treeCollapse file tree

6 files changed

+180
-27
lines changed
Filter options
Expand file treeCollapse file tree

6 files changed

+180
-27
lines changed

‎spec/schedulers/AnimationFrameScheduler-spec.ts

Copy file name to clipboardExpand all lines: spec/schedulers/AnimationFrameScheduler-spec.ts
+71-1Lines changed: 71 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ describe('Scheduler.animationFrame', () => {
4343
const requestSpy = sinon.spy(animationFrameProvider, 'requestAnimationFrame');
4444
const setSpy = sinon.spy(intervalProvider, 'setInterval');
4545
const clearSpy = sinon.spy(intervalProvider, 'clearInterval');
46-
46+
4747
animate(' ----------x--');
4848
const a = cold(' a ');
4949
const ta = time(' ----| ');
@@ -168,4 +168,74 @@ describe('Scheduler.animationFrame', () => {
168168
}, 0, 0);
169169
scheduledIndices.push(0);
170170
});
171+
172+
it('should execute actions scheduled when flushing in a subsequent flush', (done) => {
173+
const sandbox = sinon.createSandbox();
174+
const stubFlush = (sandbox.stub(animationFrameScheduler, 'flush')).callThrough();
175+
176+
let a: Subscription;
177+
let b: Subscription;
178+
let c: Subscription;
179+
180+
a = animationFrameScheduler.schedule(() => {
181+
expect(stubFlush).to.have.callCount(1);
182+
c = animationFrameScheduler.schedule(() => {
183+
expect(stubFlush).to.have.callCount(2);
184+
sandbox.restore();
185+
done();
186+
});
187+
});
188+
b = animationFrameScheduler.schedule(() => {
189+
expect(stubFlush).to.have.callCount(1);
190+
});
191+
});
192+
193+
it('should execute actions scheduled when flushing in a subsequent flush when some actions are unsubscribed', (done) => {
194+
const sandbox = sinon.createSandbox();
195+
const stubFlush = (sandbox.stub(animationFrameScheduler, 'flush')).callThrough();
196+
197+
let a: Subscription;
198+
let b: Subscription;
199+
let c: Subscription;
200+
201+
a = animationFrameScheduler.schedule(() => {
202+
expect(stubFlush).to.have.callCount(1);
203+
c = animationFrameScheduler.schedule(() => {
204+
expect(stubFlush).to.have.callCount(2);
205+
sandbox.restore();
206+
done();
207+
});
208+
b.unsubscribe();
209+
});
210+
b = animationFrameScheduler.schedule(() => {
211+
done(new Error('Unexpected execution of b'));
212+
});
213+
});
214+
215+
it('should properly cancel an unnecessary flush', (done) => {
216+
const sandbox = sinon.createSandbox();
217+
const cancelAnimationFrameStub = sandbox.stub(animationFrameProvider, 'cancelAnimationFrame').callThrough();
218+
219+
let a: Subscription;
220+
let b: Subscription;
221+
let c: Subscription;
222+
223+
a = animationFrameScheduler.schedule(() => {
224+
expect(animationFrameScheduler.actions).to.have.length(1);
225+
c = animationFrameScheduler.schedule(() => {
226+
done(new Error('Unexpected execution of c'));
227+
});
228+
expect(animationFrameScheduler.actions).to.have.length(2);
229+
// What we're testing here is that the unsubscription of action c effects
230+
// the cancellation of the animation frame in a scenario in which the
231+
// actions queue is not empty - it contains action b.
232+
c.unsubscribe();
233+
expect(animationFrameScheduler.actions).to.have.length(1);
234+
expect(cancelAnimationFrameStub).to.have.callCount(1);
235+
});
236+
b = animationFrameScheduler.schedule(() => {
237+
sandbox.restore();
238+
done();
239+
});
240+
});
171241
});

‎spec/schedulers/AsapScheduler-spec.ts

Copy file name to clipboardExpand all lines: spec/schedulers/AsapScheduler-spec.ts
+77-10Lines changed: 77 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -39,9 +39,10 @@ describe('Scheduler.asap', () => {
3939

4040
it('should cancel asap actions when delay > 0', () => {
4141
testScheduler.run(({ cold, expectObservable, flush, time }) => {
42-
const setImmediateSpy = sinon.spy(immediateProvider, 'setImmediate');
43-
const setSpy = sinon.spy(intervalProvider, 'setInterval');
44-
const clearSpy = sinon.spy(intervalProvider, 'clearInterval');
42+
const sandbox = sinon.createSandbox();
43+
const setImmediateSpy = sandbox.spy(immediateProvider, 'setImmediate');
44+
const setSpy = sandbox.spy(intervalProvider, 'setInterval');
45+
const clearSpy = sandbox.spy(intervalProvider, 'clearInterval');
4546

4647
const a = cold(' a ');
4748
const ta = time(' ----| ');
@@ -57,17 +58,15 @@ describe('Scheduler.asap', () => {
5758
expect(setImmediateSpy).to.have.not.been.called;
5859
expect(setSpy).to.have.been.calledOnce;
5960
expect(clearSpy).to.have.been.calledOnce;
60-
setImmediateSpy.restore();
61-
setSpy.restore();
62-
clearSpy.restore();
61+
sandbox.restore();
6362
});
6463
});
6564

6665
it('should reuse the interval for recursively scheduled actions with the same delay', () => {
6766
const sandbox = sinon.createSandbox();
6867
const fakeTimer = sandbox.useFakeTimers();
6968
// callThrough is missing from the declarations installed by the typings tool in stable
70-
const stubSetInterval = (<any> sinon.stub(global, 'setInterval')).callThrough();
69+
const stubSetInterval = (<any> sandbox.stub(global, 'setInterval')).callThrough();
7170
const period = 50;
7271
const state = { index: 0, period };
7372
type State = typeof state;
@@ -86,15 +85,14 @@ describe('Scheduler.asap', () => {
8685
fakeTimer.tick(period);
8786
expect(state).to.have.property('index', 2);
8887
expect(stubSetInterval).to.have.property('callCount', 1);
89-
stubSetInterval.restore();
9088
sandbox.restore();
9189
});
9290

9391
it('should not reuse the interval for recursively scheduled actions with a different delay', () => {
9492
const sandbox = sinon.createSandbox();
9593
const fakeTimer = sandbox.useFakeTimers();
9694
// callThrough is missing from the declarations installed by the typings tool in stable
97-
const stubSetInterval = (<any> sinon.stub(global, 'setInterval')).callThrough();
95+
const stubSetInterval = (<any> sandbox.stub(global, 'setInterval')).callThrough();
9896
const period = 50;
9997
const state = { index: 0, period };
10098
type State = typeof state;
@@ -114,7 +112,6 @@ describe('Scheduler.asap', () => {
114112
fakeTimer.tick(period);
115113
expect(state).to.have.property('index', 2);
116114
expect(stubSetInterval).to.have.property('callCount', 3);
117-
stubSetInterval.restore();
118115
sandbox.restore();
119116
});
120117

@@ -221,4 +218,74 @@ describe('Scheduler.asap', () => {
221218
}, 0, 0);
222219
scheduledIndices.push(0);
223220
});
221+
222+
it('should execute actions scheduled when flushing in a subsequent flush', (done) => {
223+
const sandbox = sinon.createSandbox();
224+
const stubFlush = (sandbox.stub(asapScheduler, 'flush')).callThrough();
225+
226+
let a: Subscription;
227+
let b: Subscription;
228+
let c: Subscription;
229+
230+
a = asapScheduler.schedule(() => {
231+
expect(stubFlush).to.have.callCount(1);
232+
c = asapScheduler.schedule(() => {
233+
expect(stubFlush).to.have.callCount(2);
234+
sandbox.restore();
235+
done();
236+
});
237+
});
238+
b = asapScheduler.schedule(() => {
239+
expect(stubFlush).to.have.callCount(1);
240+
});
241+
});
242+
243+
it('should execute actions scheduled when flushing in a subsequent flush when some actions are unsubscribed', (done) => {
244+
const sandbox = sinon.createSandbox();
245+
const stubFlush = (sandbox.stub(asapScheduler, 'flush')).callThrough();
246+
247+
let a: Subscription;
248+
let b: Subscription;
249+
let c: Subscription;
250+
251+
a = asapScheduler.schedule(() => {
252+
expect(stubFlush).to.have.callCount(1);
253+
c = asapScheduler.schedule(() => {
254+
expect(stubFlush).to.have.callCount(2);
255+
sandbox.restore();
256+
done();
257+
});
258+
b.unsubscribe();
259+
});
260+
b = asapScheduler.schedule(() => {
261+
done(new Error('Unexpected execution of b'));
262+
});
263+
});
264+
265+
it('should properly cancel an unnecessary flush', (done) => {
266+
const sandbox = sinon.createSandbox();
267+
const clearImmediateStub = sandbox.stub(immediateProvider, 'clearImmediate').callThrough();
268+
269+
let a: Subscription;
270+
let b: Subscription;
271+
let c: Subscription;
272+
273+
a = asapScheduler.schedule(() => {
274+
expect(asapScheduler.actions).to.have.length(1);
275+
c = asapScheduler.schedule(() => {
276+
done(new Error('Unexpected execution of c'));
277+
});
278+
expect(asapScheduler.actions).to.have.length(2);
279+
// What we're testing here is that the unsubscription of action c effects
280+
// the cancellation of the microtask in a scenario in which the actions
281+
// queue is not empty - it contains action b.
282+
c.unsubscribe();
283+
expect(asapScheduler.actions).to.have.length(1);
284+
expect(clearImmediateStub).to.have.callCount(1);
285+
});
286+
b = asapScheduler.schedule(() => {
287+
sandbox.restore();
288+
done();
289+
});
290+
});
224291
});

‎src/internal/scheduler/AnimationFrameAction.ts

Copy file name to clipboardExpand all lines: src/internal/scheduler/AnimationFrameAction.ts
+4-4Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,10 +27,10 @@ export class AnimationFrameAction<T> extends AsyncAction<T> {
2727
if ((delay != null && delay > 0) || (delay == null && this.delay > 0)) {
2828
return super.recycleAsyncId(scheduler, id, delay);
2929
}
30-
// If the scheduler queue is empty, cancel the requested animation frame and
31-
// set the scheduled flag to undefined so the next AnimationFrameAction will
32-
// request its own.
33-
if (scheduler.actions.length === 0) {
30+
// If the scheduler queue has no remaining actions with the same async id,
31+
// cancel the requested animation frame and set the scheduled flag to
32+
// undefined so the next AnimationFrameAction will request its own.
33+
if (!scheduler.actions.some((action) => action.id === id)) {
3434
animationFrameProvider.cancelAnimationFrame(id);
3535
scheduler._scheduled = undefined;
3636
}

‎src/internal/scheduler/AnimationFrameScheduler.ts

Copy file name to clipboardExpand all lines: src/internal/scheduler/AnimationFrameScheduler.ts
+12-4Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,24 +4,32 @@ import { AsyncScheduler } from './AsyncScheduler';
44
export class AnimationFrameScheduler extends AsyncScheduler {
55
public flush(action?: AsyncAction<any>): void {
66
this._active = true;
7+
// The async id that effects a call to flush is stored in _scheduled.
8+
// Before executing an action, it's necessary to check the action's async
9+
// id to determine whether it's supposed to be executed in the current
10+
// flush.
11+
// Previous implementations of this method used a count to determine this,
12+
// but that was unsound, as actions that are unsubscribed - i.e. cancelled -
13+
// are removed from the actions array and that can shift actions that are
14+
// scheduled to be executed in a subsequent flush into positions at which
15+
// they are executed within the current flush.
16+
const flushId = this._scheduled;
717
this._scheduled = undefined;
818

919
const { actions } = this;
1020
let error: any;
11-
let index = -1;
1221
action = action || actions.shift()!;
13-
const count = actions.length;
1422

1523
do {
1624
if ((error = action.execute(action.state, action.delay))) {
1725
break;
1826
}
19-
} while (++index < count && (action = actions.shift()));
27+
} while ((action = actions[0]) && action.id === flushId && actions.shift());
2028

2129
this._active = false;
2230

2331
if (error) {
24-
while (++index < count && (action = actions.shift())) {
32+
while ((action = actions[0]) && action.id === flushId && actions.shift()) {
2533
action.unsubscribe();
2634
}
2735
throw error;

‎src/internal/scheduler/AsapAction.ts

Copy file name to clipboardExpand all lines: src/internal/scheduler/AsapAction.ts
+4-4Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,10 +27,10 @@ export class AsapAction<T> extends AsyncAction<T> {
2727
if ((delay != null && delay > 0) || (delay == null && this.delay > 0)) {
2828
return super.recycleAsyncId(scheduler, id, delay);
2929
}
30-
// If the scheduler queue is empty, cancel the requested microtask and
31-
// set the scheduled flag to undefined so the next AsapAction will schedule
32-
// its own.
33-
if (scheduler.actions.length === 0) {
30+
// If the scheduler queue has no remaining actions with the same async id,
31+
// cancel the requested microtask and set the scheduled flag to undefined
32+
// so the next AsapAction will request its own.
33+
if (!scheduler.actions.some((action) => action.id === id)) {
3434
immediateProvider.clearImmediate(id);
3535
scheduler._scheduled = undefined;
3636
}

‎src/internal/scheduler/AsapScheduler.ts

Copy file name to clipboardExpand all lines: src/internal/scheduler/AsapScheduler.ts
+12-4Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,24 +4,32 @@ import { AsyncScheduler } from './AsyncScheduler';
44
export class AsapScheduler extends AsyncScheduler {
55
public flush(action?: AsyncAction<any>): void {
66
this._active = true;
7+
// The async id that effects a call to flush is stored in _scheduled.
8+
// Before executing an action, it's necessary to check the action's async
9+
// id to determine whether it's supposed to be executed in the current
10+
// flush.
11+
// Previous implementations of this method used a count to determine this,
12+
// but that was unsound, as actions that are unsubscribed - i.e. cancelled -
13+
// are removed from the actions array and that can shift actions that are
14+
// scheduled to be executed in a subsequent flush into positions at which
15+
// they are executed within the current flush.
16+
const flushId = this._scheduled;
717
this._scheduled = undefined;
818

919
const { actions } = this;
1020
let error: any;
11-
let index = -1;
1221
action = action || actions.shift()!;
13-
const count = actions.length;
1422

1523
do {
1624
if ((error = action.execute(action.state, action.delay))) {
1725
break;
1826
}
19-
} while (++index < count && (action = actions.shift()));
27+
} while ((action = actions[0]) && action.id === flushId && actions.shift());
2028

2129
this._active = false;
2230

2331
if (error) {
24-
while (++index < count && (action = actions.shift())) {
32+
while ((action = actions[0]) && action.id === flushId && actions.shift()) {
2533
action.unsubscribe();
2634
}
2735
throw error;

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.