honeymoose
/
angular-cn
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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
This commit is contained in:
JiaLiPassion 2021-02-03 08:43:44 +08:00 committed by atscott
parent d28197db15
commit dc9fd1aaef
3 changed files with 86 additions and 3 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

43
packages/core/src/zone/ng_zone.ts
View File

@ -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();

42
packages/core/test/zone/ng_zone_spec.ts
View File

@ -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[] = [];

4
packages/platform-browser/test/dom/events/event_manager_spec.ts
View File

@ -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);