fix(core): NgZone coaleascing options should trigger onStable correctly (#40540)

JiaLiPassion · mhevery · commit 22f9e454a46a · 2021-01-28T15:53:38.000-08:00
fix angular/components#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
diff --git 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
@@ -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[] = [];
