From dc9fd1aaefa9cf2a019e293581117a8e84ae277c Mon Sep 17 00:00:00 2001 From: JiaLiPassion Date: Wed, 3 Feb 2021 08:43:44 +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, there is a new flag `zone.isCheckStableRunning` added to prevent re-entry when `shouldCoaleascing` flag is enabled. PR Close #40540 --- packages/core/src/zone/ng_zone.ts | 43 ++++++++++++++++++- packages/core/test/zone/ng_zone_spec.ts | 42 ++++++++++++++++++ .../test/dom/events/event_manager_spec.ts | 4 +- 3 files changed, 86 insertions(+), 3 deletions(-) diff --git a/packages/core/src/zone/ng_zone.ts b/packages/core/src/zone/ng_zone.ts index bb230c27f7..74d1748116 100644 --- a/packages/core/src/zone/ng_zone.ts +++ b/packages/core/src/zone/ng_zone.ts @@ -243,6 +243,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,6 +302,21 @@ interface NgZonePrivate extends NgZone { } function checkStable(zone: NgZonePrivate) { + // TODO: @JiaLiPassion, should check zone.isCheckStableRunning to prevent + // re-entry. The case is: + // + // @Component({...}) + // export class AppComponent { + // constructor(private ngZone: NgZone) { + // this.ngZone.onStable.subscribe(() => { + // this.ngZone.run(() => console.log('stable');); + // }); + // } + // + // The onStable subscriber run another function inside ngZone + // which causes `checkStable()` re-entry. + // But this fix causes some issues in g3, so this fix will be + // launched in another PR. if (zone._nesting == 0 && !zone.hasPendingMicrotasks && !zone.isStable) { try { zone._nesting++; @@ -309,7 +335,20 @@ function checkStable(zone: NgZonePrivate) { } function delayChangeDetectionForEvents(zone: NgZonePrivate) { - if (zone.lastRequestAnimationFrameId !== -1) { + /** + * We also need to check _nesting 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 _nesting here to prevent multiple + * change detections. + */ + if (zone.isCheckStableRunning || zone.lastRequestAnimationFrameId !== -1) { return; } zone.lastRequestAnimationFrameId = zone.nativeRequestAnimationFrame.call(global, () => { @@ -326,7 +365,9 @@ function delayChangeDetectionForEvents(zone: NgZonePrivate) { zone.fakeTopEventTask = Zone.root.scheduleEventTask('fakeTopEventTask', () => { zone.lastRequestAnimationFrameId = -1; updateMicroTaskStatus(zone); + zone.isCheckStableRunning = true; checkStable(zone); + zone.isCheckStableRunning = false; }, undefined, () => {}, () => {}); } zone.fakeTopEventTask.invoke(); 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[] = []; diff --git a/packages/platform-browser/test/dom/events/event_manager_spec.ts b/packages/platform-browser/test/dom/events/event_manager_spec.ts index 245ca5bf9f..0369cd615c 100644 --- a/packages/platform-browser/test/dom/events/event_manager_spec.ts +++ b/packages/platform-browser/test/dom/events/event_manager_spec.ts @@ -436,13 +436,13 @@ describe('EventManager', () => { removerChildFocus = manager.addEventListener(child, 'blur', blurHandler); }); const sub = zone.onStable.subscribe(() => { + sub.unsubscribe(); logs.push('begin'); Promise.resolve().then(() => { logs.push('promise resolved'); }); element.appendChild(child); getDOM().dispatchEvent(child, dispatchedBlurEvent); - sub.unsubscribe(); logs.push('end'); }); getDOM().dispatchEvent(element, dispatchedClickEvent); @@ -482,13 +482,13 @@ describe('EventManager', () => { removerChildFocus = manager.addEventListener(child, 'blur', blurHandler); }); const sub = zone.onStable.subscribe(() => { + sub.unsubscribe(); logs.push('begin'); Promise.resolve().then(() => { logs.push('promise resolved'); }); element.appendChild(child); getDOM().dispatchEvent(child, dispatchedBlurEvent); - sub.unsubscribe(); logs.push('end'); }); getDOM().dispatchEvent(element, dispatchedClickEvent);