Skip to content

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

Closed
JiaLiPassion wants to merge 1 commit intoangular:masterfrom
JiaLiPassion:ngzone-run-coalesce-issue
Closed

fix(core): NgZone coaleascing options should trigger onStable correctly#40540
JiaLiPassion wants to merge 1 commit intoangular:masterfrom
JiaLiPassion:ngzone-run-coalesce-issue

Conversation

@JiaLiPassion
Copy link
Copy Markdown
Contributor

fix angular/components#21674

When setting ngZoneRunCoalescing to true, onStable is not emitted correctly.
The reason is before this commit, the code looks like is,

// 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.
  5. step 6: checkStable and trigger onMicrotaskEmpty emitter.
  6. step 7: ApplicationRef subscribed onMicrotaskEmpty, so it will call another ngZone.run() to process
    tick()
  7. 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.
  8. And become a infinite loop..., so onStable is never emit

In this commit, the zone.lastRequestAnimationFrameId reset is moved after checkStable() call.

@JiaLiPassion JiaLiPassion requested a review from mhevery January 23, 2021 12:07
@google-cla google-cla bot added the cla: yes label Jan 23, 2021
@JiaLiPassion JiaLiPassion added the area: zones Issues related to zone.js label Jan 23, 2021
@ngbot ngbot bot added this to the Backlog milestone Jan 23, 2021
@JiaLiPassion JiaLiPassion added the target: minor This PR is targeted for the next minor release label Jan 23, 2021
@JiaLiPassion JiaLiPassion force-pushed the ngzone-run-coalesce-issue branch from 3f882d8 to 76a579d Compare January 23, 2021 21:35
Copy link
Copy Markdown
Contributor

@mhevery mhevery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also can you add some jsdocs explaining the purpose of these flags?

@JiaLiPassion JiaLiPassion force-pushed the ngzone-run-coalesce-issue branch from 76a579d to ef0548a Compare January 26, 2021 12:59
@JiaLiPassion
Copy link
Copy Markdown
Contributor Author

JiaLiPassion commented Jan 26, 2021

@mhevery , I updated the PR and added the comment to explain the flag, also added a comment here #40540 (comment), please review, thanks.

@JiaLiPassion JiaLiPassion requested a review from mhevery January 26, 2021 13:00
@JiaLiPassion JiaLiPassion force-pushed the ngzone-run-coalesce-issue branch from ef0548a to 53cb493 Compare January 26, 2021 14:21
@google-cla
Copy link
Copy Markdown

google-cla bot commented Jan 26, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Jan 26, 2021
@google-cla
Copy link
Copy Markdown

google-cla bot commented Jan 27, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

Copy link
Copy Markdown
Contributor

@mhevery mhevery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I like this approach much better. Way cleaner and easier to follow.

@mhevery mhevery added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jan 27, 2021
@JiaLiPassion JiaLiPassion force-pushed the ngzone-run-coalesce-issue branch from f6ecbc8 to 453790f Compare January 28, 2021 00:25
@google-cla google-cla bot added cla: yes and removed cla: no labels Jan 28, 2021
@mhevery mhevery removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jan 28, 2021
@JiaLiPassion JiaLiPassion added the action: merge The PR is ready for merge by the caretaker label Jan 28, 2021
@JiaLiPassion JiaLiPassion force-pushed the ngzone-run-coalesce-issue branch from 453790f to 2b7f044 Compare January 28, 2021 00:44
@mhevery
Copy link
Copy Markdown
Contributor

mhevery commented Jan 28, 2021

presubmit

@mhevery mhevery closed this in 22f9e45 Jan 28, 2021
@mhevery mhevery reopened this Jan 29, 2021
@mhevery
Copy link
Copy Markdown
Contributor

mhevery commented Feb 12, 2021

presubmit

@JiaLiPassion JiaLiPassion force-pushed the ngzone-run-coalesce-issue branch 2 times, most recently from 2ed45f9 to b39bbbe Compare February 13, 2021 10:22
@pullapprove pullapprove bot requested a review from atscott February 15, 2021 20:15
@mhevery
Copy link
Copy Markdown
Contributor

mhevery commented Feb 16, 2021

deflake1
deflake2

passed

@JiaLiPassion
Copy link
Copy Markdown
Contributor Author

@mhevery , I have rebased the PR, please review. Thanks!

@JiaLiPassion JiaLiPassion force-pushed the ngzone-run-coalesce-issue branch from b39bbbe to 71346fd Compare February 17, 2021 01:07
@josephperrott josephperrott added action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: merge The PR is ready for merge by the caretaker labels Feb 17, 2021
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, there is a new flag `zone.isCheckStableRunning` added to
prevent re-entry when `shouldCoaleascing` flag is enabled.
@atscott
Copy link
Copy Markdown
Contributor

atscott commented Feb 17, 2021

FYI - I reverted the change to the size tracking golden since there was only a newline change at the end of the file after a rebase. That removes the need for the size tracking approval.

@atscott atscott added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Feb 17, 2021
@atscott
Copy link
Copy Markdown
Contributor

atscott commented Feb 17, 2021

caretaker note: Zones change that was previously reverted. Merge & sync with caution

@atscott atscott added the risky Identifies a PR that has a level of risk to merging label Feb 17, 2021
@atscott atscott closed this in dc9fd1a Feb 22, 2021
@angular-automatic-lock-bot
Copy link
Copy Markdown

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker area: zones Issues related to zone.js cla: yes merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note risky Identifies a PR that has a level of risk to merging target: minor This PR is targeted for the next minor release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(cdk/overlay): use of ngZoneRunCoalescing opens the overlay at wrong position

5 participants