Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(upgrade): do not break if onMicrotaskEmpty emits while a $digest is in progress #29794

Closed
wants to merge 1 commit into from

Conversation

samjulien
Copy link
Contributor

@samjulien samjulien commented Apr 9, 2019

PR Checklist

PR Type

  • Bugfix

What is the current behavior?

Previously, under certain circumstances, NgZone#onMicrotaskEmpty could emit while a $digest was in progress, thus triggering another $digest, which in turn would throw a $digest already in progress error. Furthermore, throwing an error from inside the onMicrotaskEmpty subscription would result in unsubscribing and stop triggering further $digests, when onMicrotaskEmpty emitted.

Usually, emitting while a $digest was already in progress was a result of unintentionally running some part of AngularJS outside the Angular zone, but there are valid (if rare) usecases where this can happen (see #24680 for details).

Issue Number: #24680

What is the new behavior?

This PR addresses the issue as follows:

  • If a $digest is in progress when onMicrotaskEmpty emits, do not trigger another $digest (to avoid the error). $evalAsync() is used instead, to ensure that the bindings are evaluated at least once more.
  • Since there is still a high probability that the situation is a result of programming error (i.e. some AngularJS part running outside the Angular Zone), a warning will be logged, but only if the app is in dev mode.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Fixes #24680.

@samjulien samjulien requested a review from gkalpak April 9, 2019 20:14
@benlesh benlesh added the area: upgrade Issues related to AngularJS → Angular upgrade APIs label Apr 17, 2019
@ngbot ngbot bot added this to the needsTriage milestone Apr 17, 2019
@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that 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 here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

@samjulien samjulien marked this pull request as ready for review April 23, 2019 14:27
@samjulien samjulien requested a review from a team as a code owner April 23, 2019 14:27
@gkalpak gkalpak added type: bug/fix effort1: hours target: patch This PR is targeted for the next patch release risk: low labels Apr 23, 2019
@ngbot ngbot bot modified the milestones: needsTriage, Backlog Apr 23, 2019
@gkalpak gkalpak added the action: merge The PR is ready for merge by the caretaker label Apr 23, 2019
@ngbot
Copy link

ngbot bot commented Apr 23, 2019

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure status "cla/google" is failing
    pending missing required labels: cla: yes
    pending forbidden labels detected: cla: no
    pending status "google3" is pending
    pending 1 pending code review

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@gkalpak gkalpak added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: merge The PR is ready for merge by the caretaker labels Apr 23, 2019
@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

LGTM 🎉
Can you change the commit message to be more descriptive and follow our guidelines more closely; e.g.:

fix(upgrade): do not break if onMicrotaskEmpty emits while a $digest is in progress

/* ...explain what this commit fixes... */

Fixes #24680

@samjulien
Copy link
Contributor Author

🎉

How's my amended commit message?

@gkalpak gkalpak changed the title fix(upgrade): check for ng1 digest triggers fix(upgrade): do not break if onMicrotaskEmpty emits while a $digest is in progress Apr 24, 2019
…st` is in progress

Previously, under certain circumstances, `NgZone#onMicrotaskEmpty` could
emit while a `$digest` was in progress, thus triggering another
`$digest`, which in turn would throw a `$digest already in progress`
error. Furthermore, throwing an error from inside the `onMicrotaskEmpty`
subscription would result in unsubscribing and stop triggering further
`$digest`s, when `onMicrotaskEmpty` emitted.

Usually, emitting while a `$digest` was already in progress was a result
of unintentionally running some part of AngularJS outside the Angular
zone, but there are valid (if rare) usecases where this can happen
(see angular#24680 for details).

This commit addresses the issue as follows:
- If a `$digest` is in progress when `onMicrotaskEmpty` emits, do not
  trigger another `$digest` (to avoid the error). `$evalAsync()` is used
  instead, to ensure that the bindings are evaluated at least once more.
- Since there is still a high probability that the situation is a result
  of programming error (i.e. some AngularJS part running outside the
  Angular Zone), a warning will be logged, but only if the app is in
  [dev mode][1].

[1]: https://github.com/angular/angular/blob/78146c189/packages/core/src/util/ng_dev_mode.ts#L12

Fixes angular#24680
@gkalpak gkalpak 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: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Apr 24, 2019
@gkalpak
Copy link
Member

gkalpak commented Apr 24, 2019

merge-assistance: Requires g3sync presubmit.

@AndrewKushnir
Copy link
Contributor

Presubmit

@AndrewKushnir AndrewKushnir added action: presubmit The PR is in need of a google3 presubmit target: major This PR is targeted for the next major release and removed action: presubmit The PR is in need of a google3 presubmit target: patch This PR is targeted for the next patch release labels Apr 25, 2019
@AndrewKushnir
Copy link
Contributor

@samjulien @gkalpak merging this PR to patch branch failed due to some conflicts, so I merged these changes to master only. Could you please create additional PR with the necessary changes and target patch branch? Thank you.

gkalpak added a commit to gkalpak/angular that referenced this pull request Apr 25, 2019
With angular#30058, the ngUpgrade internal `angular.module()` method was
renamed to `angular.module_()` (to avoid a webpack bug).

Merging angular#29794 afterwards resulted in some broken tests, because it
still used the old `angular.module()` method name. (The PR had been
tested on CI against a revision that did not contain the rename.)

This commit fixes the broken tests by renaming the remaining occurrences
of `angular.module()`.
@gkalpak
Copy link
Member

gkalpak commented Apr 25, 2019

On it! Thx, @AndrewKushnir 👍

gkalpak pushed a commit to gkalpak/angular that referenced this pull request Apr 25, 2019
…st` is in progress (angular#29794)

Previously, under certain circumstances, `NgZone#onMicrotaskEmpty` could
emit while a `$digest` was in progress, thus triggering another
`$digest`, which in turn would throw a `$digest already in progress`
error. Furthermore, throwing an error from inside the `onMicrotaskEmpty`
subscription would result in unsubscribing and stop triggering further
`$digest`s, when `onMicrotaskEmpty` emitted.

Usually, emitting while a `$digest` was already in progress was a result
of unintentionally running some part of AngularJS outside the Angular
zone, but there are valid (if rare) usecases where this can happen
(see angular#24680 for details).

This commit addresses the issue as follows:
- If a `$digest` is in progress when `onMicrotaskEmpty` emits, do not
  trigger another `$digest` (to avoid the error). `$evalAsync()` is used
  instead, to ensure that the bindings are evaluated at least once more.
- Since there is still a high probability that the situation is a result
  of programming error (i.e. some AngularJS part running outside the
  Angular Zone), a warning will be logged, but only if the app is in
  [dev mode][1].

[1]: https://github.com/angular/angular/blob/78146c189/packages/core/src/util/ng_dev_mode.ts#L12

Fixes angular#24680

PR Close angular#29794
gkalpak added a commit to gkalpak/angular that referenced this pull request Apr 25, 2019
With angular#30058, the ngUpgrade internal `angular.module()` method wa
renamed to `angular.module_()` (to avoid a webpack bug).

Merging angular#29794 afterwards resulted in some broken tests, because it
still used the old `angular.module()` method name. (The PR had been
tested on CI against a revision that did not contain the rename.)

This commit fixes the broken tests by renaming the remaining occurrences
of `angular.module()`.
@gkalpak
Copy link
Member

gkalpak commented Apr 25, 2019

I have incorporated the change in #30107.

AndrewKushnir pushed a commit that referenced this pull request Apr 25, 2019
With #30058, the ngUpgrade internal `angular.module()` method was
renamed to `angular.module_()` (to avoid a webpack bug).

Merging #29794 afterwards resulted in some broken tests, because it
still used the old `angular.module()` method name. (The PR had been
tested on CI against a revision that did not contain the rename.)

This commit fixes the broken tests by renaming the remaining occurrences
of `angular.module()`.

PR Close #30126
AndrewKushnir pushed a commit that referenced this pull request Apr 25, 2019
…st` is in progress (#29794) (#30107)

Previously, under certain circumstances, `NgZone#onMicrotaskEmpty` could
emit while a `$digest` was in progress, thus triggering another
`$digest`, which in turn would throw a `$digest already in progress`
error. Furthermore, throwing an error from inside the `onMicrotaskEmpty`
subscription would result in unsubscribing and stop triggering further
`$digest`s, when `onMicrotaskEmpty` emitted.

Usually, emitting while a `$digest` was already in progress was a result
of unintentionally running some part of AngularJS outside the Angular
zone, but there are valid (if rare) usecases where this can happen
(see #24680 for details).

This commit addresses the issue as follows:
- If a `$digest` is in progress when `onMicrotaskEmpty` emits, do not
  trigger another `$digest` (to avoid the error). `$evalAsync()` is used
  instead, to ensure that the bindings are evaluated at least once more.
- Since there is still a high probability that the situation is a result
  of programming error (i.e. some AngularJS part running outside the
  Angular Zone), a warning will be logged, but only if the app is in
  [dev mode][1].

[1]: https://github.com/angular/angular/blob/78146c189/packages/core/src/util/ng_dev_mode.ts#L12

Fixes #24680

PR Close #29794

PR Close #30107
AndrewKushnir pushed a commit that referenced this pull request Apr 25, 2019
With #30058, the ngUpgrade internal `angular.module()` method wa
renamed to `angular.module_()` (to avoid a webpack bug).

Merging #29794 afterwards resulted in some broken tests, because it
still used the old `angular.module()` method name. (The PR had been
tested on CI against a revision that did not contain the rename.)

This commit fixes the broken tests by renaming the remaining occurrences
of `angular.module()`.

PR Close #30107
BioPhoton pushed a commit to BioPhoton/angular that referenced this pull request May 21, 2019
…st` is in progress (angular#29794)

Previously, under certain circumstances, `NgZone#onMicrotaskEmpty` could
emit while a `$digest` was in progress, thus triggering another
`$digest`, which in turn would throw a `$digest already in progress`
error. Furthermore, throwing an error from inside the `onMicrotaskEmpty`
subscription would result in unsubscribing and stop triggering further
`$digest`s, when `onMicrotaskEmpty` emitted.

Usually, emitting while a `$digest` was already in progress was a result
of unintentionally running some part of AngularJS outside the Angular
zone, but there are valid (if rare) usecases where this can happen
(see angular#24680 for details).

This commit addresses the issue as follows:
- If a `$digest` is in progress when `onMicrotaskEmpty` emits, do not
  trigger another `$digest` (to avoid the error). `$evalAsync()` is used
  instead, to ensure that the bindings are evaluated at least once more.
- Since there is still a high probability that the situation is a result
  of programming error (i.e. some AngularJS part running outside the
  Angular Zone), a warning will be logged, but only if the app is in
  [dev mode][1].

[1]: https://github.com/angular/angular/blob/78146c189/packages/core/src/util/ng_dev_mode.ts#L12

Fixes angular#24680

PR Close angular#29794
BioPhoton pushed a commit to BioPhoton/angular that referenced this pull request May 21, 2019
…lar#30126)

With angular#30058, the ngUpgrade internal `angular.module()` method was
renamed to `angular.module_()` (to avoid a webpack bug).

Merging angular#29794 afterwards resulted in some broken tests, because it
still used the old `angular.module()` method name. (The PR had been
tested on CI against a revision that did not contain the rename.)

This commit fixes the broken tests by renaming the remaining occurrences
of `angular.module()`.

PR Close angular#30126
@angular-automatic-lock-bot
Copy link

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 Sep 15, 2019
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: upgrade Issues related to AngularJS → Angular upgrade APIs cla: yes effort1: hours merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note risk: low target: major This PR is targeted for the next major release type: bug/fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UpgradeModule: Exception in $digest breaks change propagation
5 participants