From 22f9e454a46a8b017479d518bedd40efe1e7be2b Mon Sep 17 00:00:00 2001 From: JiaLiPassion Date: Sat, 23 Jan 2021 19:49:22 +0800 Subject: [PATCH] fix(core): NgZone coaleascing options should trigger onStable correctly (#40540) fix https://github.com/angular/components/issues/21674 When setting `ngZoneRunCoalescing` to true, `onStable` is not emitted correctly. The reason is before this commit, the code looks like this ``` // Application code call `ngZone.run()` ngZone.run(() => {}); // step 1 // Inside NgZone, in the OnInvoke hook, NgZone try to delay the checkStable() function delayChangeDetectionForEvents(zone: NgZonePrivate) { if (zone.lastRequestAnimationFrameId !== -1) { // step 9 return; } zone.lastRequestAnimationFrameId = zone.nativeRequestAnimationFrame.call(global, () => { // step 2 if (!zone.fakeTopEventTask) { zone.fakeTopEventTask = Zone.root.scheduleEventTask('fakeTopEventTask', () => { zone.lastRequestAnimationFrameId = -1; // step 3 updateMicroTaskStatus(zone); // step 4 checkStable(zone); // step 6 }, undefined, () => {}, () => {}); } zone.fakeTopEventTask.invoke(); }); updateMicroTaskStatus(zone); } function updateMicroTaskStatus(zone: NgZonePrivate, ignoreCheckRAFId = false) { if (zone._hasPendingMicrotasks || ((zone.shouldCoalesceEventChangeDetection || zone.shouldCoalesceRunChangeDetection) && zone.lastRequestAnimationFrameId !== -1)) { // step 5 zone.hasPendingMicrotasks = true; } else { zone.hasPendingMicrotasks = false; } } function checkStable(zone: NgZonePrivate) { if (zone._nesting == 0 && !zone.hasPendingMicrotasks && !zone.isStable) { // step 7 try { zone._nesting++; zone.onMicrotaskEmpty.emit(null); ... } // application ref subscribe onMicroTaskEmpty ngZone.onMicroTaskEmpty.subscribe(() => { ngZone.run(() => { // step 8 tick(); }); }); ``` And the process is: 1. step 1: application call ngZone.run() 2. step 2: NgZone delay the checkStable() call in a requestAnimationFrame, and also set zone.lastRequestAnimationFrameId 3. step 3: Inside the requestAnimationFrame callback, reset zone.lastRequestAnimationFrameId first 4. step 4: update microTask status 5, step 5: if zone.lastRequestAnimationFrameId is -1, that means no microTask pending. 6. step 6: checkStable and trigger onMicrotaskEmpty emitter. 7. step 7: ApplicationRef subscribed onMicrotaskEmpty, so it will call another `ngZone.run()` to process tick() 8. step 8: And this new `ngZone.run()` will try to check `zone.lastRequestAnimationFrameId` in `step 9` when trying to delay the checkStable(), and since the zone.lastRequestAnimationFrameId is already reset to -1 in step 3, so this ngZone.run() will run into step 2 again. 9. And become a infinite loop..., so onStable is never emit In this commit, the `zone.lastRequestAnimationFrameId` reset is moved after `checkStable()` call. PR Close #40540 --- packages/core/src/zone/ng_zone.ts | 32 +++++++++++++++++-- packages/core/test/zone/ng_zone_spec.ts | 42 +++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 2 deletions(-) diff --git a/packages/core/src/zone/ng_zone.ts b/packages/core/src/zone/ng_zone.ts index bb230c27f7..33152592cb 100644 --- a/packages/core/src/zone/ng_zone.ts +++ b/packages/core/src/zone/ng_zone.ts @@ -148,6 +148,7 @@ export class NgZone { !shouldCoalesceRunChangeDetection && shouldCoalesceEventChangeDetection; self.shouldCoalesceRunChangeDetection = shouldCoalesceRunChangeDetection; self.lastRequestAnimationFrameId = -1; + self.isCheckStableRunning = false; self.nativeRequestAnimationFrame = getNativeRequestAnimationFrame().nativeRequestAnimationFrame; forkInnerZoneWithAngularBehavior(self); } @@ -243,6 +244,17 @@ interface NgZonePrivate extends NgZone { hasPendingMacrotasks: boolean; hasPendingMicrotasks: boolean; lastRequestAnimationFrameId: number; + /** + * A flag to indicate if NgZone is currently inside + * checkStable and to prevent re-entry. The flag is + * needed because it is possible to invoke the change + * detection from within change detection leading to + * incorrect behavior. + * + * For detail, please refer here, + * https://github.com/angular/angular/pull/40540 + */ + isCheckStableRunning: boolean; isStable: boolean; /** * Optionally specify coalescing event change detections or not. @@ -291,7 +303,9 @@ interface NgZonePrivate extends NgZone { } function checkStable(zone: NgZonePrivate) { - if (zone._nesting == 0 && !zone.hasPendingMicrotasks && !zone.isStable) { + if (!zone.isCheckStableRunning && zone._nesting == 0 && !zone.hasPendingMicrotasks && + !zone.isStable) { + zone.isCheckStableRunning = true; try { zone._nesting++; zone.onMicrotaskEmpty.emit(null); @@ -304,12 +318,26 @@ function checkStable(zone: NgZonePrivate) { zone.isStable = true; } } + zone.isCheckStableRunning = false; } } } function delayChangeDetectionForEvents(zone: NgZonePrivate) { - if (zone.lastRequestAnimationFrameId !== -1) { + /** + * We also need to check isCheckStableRunning here + * Consider the following case with shouldCoalesceRunChangeDetection = true + * + * ngZone.run(() => {}); + * ngZone.run(() => {}); + * + * We want the two `ngZone.run()` only trigger one change detection + * when shouldCoalesceRunChangeDetection is true. + * And because in this case, change detection run in async way(requestAnimationFrame), + * so we also need to check the isCheckStableRunning here to prevent multiple + * change detections. + */ + if (zone.isCheckStableRunning || zone.lastRequestAnimationFrameId !== -1) { return; } zone.lastRequestAnimationFrameId = zone.nativeRequestAnimationFrame.call(global, () => { diff --git a/packages/core/test/zone/ng_zone_spec.ts b/packages/core/test/zone/ng_zone_spec.ts index 3f11518a0e..dd9b6cc5ad 100644 --- a/packages/core/test/zone/ng_zone_spec.ts +++ b/packages/core/test/zone/ng_zone_spec.ts @@ -982,6 +982,7 @@ function commonTests() { describe('shouldCoalesceEventChangeDetection = true, shouldCoalesceRunChangeDetection = false', () => { let nativeRequestAnimationFrame: (fn: FrameRequestCallback) => void; + let nativeSetTimeout: any = (global as any)[Zone.__symbol__('setTimeout')]; if (!(global as any).requestAnimationFrame) { nativeRequestAnimationFrame = function(fn: Function) { (global as any)[Zone.__symbol__('setTimeout')](fn, 16); @@ -1042,6 +1043,32 @@ function commonTests() { }); }); + it('should only emit onMicroTaskEmpty once within the same event loop for ngZone.run in onMicrotaskEmpty subscription', + (done: DoneFn) => { + const tasks: Task[] = []; + coalesceZone.onMicrotaskEmpty.subscribe(() => { + coalesceZone.run(() => {}); + }); + coalesceZone.run(() => { + tasks.push(Zone.current.scheduleEventTask('myEvent', () => { + logs.push('eventTask1'); + }, undefined, () => {})); + }); + coalesceZone.run(() => { + tasks.push(Zone.current.scheduleEventTask('myEvent', () => { + logs.push('eventTask2'); + }, undefined, () => {})); + }); + tasks.forEach(t => t.invoke()); + expect(logs).toEqual(['microTask empty', 'microTask empty', 'eventTask1', 'eventTask2']); + nativeSetTimeout(() => { + expect(logs).toEqual([ + 'microTask empty', 'microTask empty', 'eventTask1', 'eventTask2', 'microTask empty' + ]); + done(); + }, 100); + }); + it('should emit onMicroTaskEmpty once within the same event loop for not only event tasks, but event tasks are before other tasks', (done: DoneFn) => { const tasks: Task[] = []; @@ -1094,6 +1121,7 @@ function commonTests() { describe('shouldCoalesceRunChangeDetection = true', () => { let nativeRequestAnimationFrame: (fn: FrameRequestCallback) => void; + let nativeSetTimeout: any = (global as any)[Zone.__symbol__('setTimeout')]; if (!(global as any).requestAnimationFrame) { nativeRequestAnimationFrame = function(fn: Function) { (global as any)[Zone.__symbol__('setTimeout')](fn, 16); @@ -1139,6 +1167,20 @@ function commonTests() { }); }); + it('should only emit onMicroTaskEmpty once within the same event loop for ngZone.run in onMicrotaskEmpty subscription', + (done: DoneFn) => { + coalesceZone.onMicrotaskEmpty.subscribe(() => { + coalesceZone.run(() => {}); + }); + coalesceZone.run(() => {}); + coalesceZone.run(() => {}); + expect(logs).toEqual([]); + nativeSetTimeout(() => { + expect(logs).toEqual(['microTask empty']); + done(); + }, 100); + }); + it('should only emit onMicroTaskEmpty once within the same event loop for multiple tasks', (done: DoneFn) => { const tasks: Task[] = [];