bug(upgrade): infinite digests with setTimeout (repro inside) #6385

andreialecu · 2016-01-10T16:30:42Z

angular2/upgrade has a bug that occurs when an angular1 watcher calls setTimeout, as is the case with many angular1 components.

Repro: http://plnkr.co/edit/5iCEQOSOer5c630FStXv?p=preview

This results in infinite angular1 digests because of this line:

https://github.com/angular/angular/blob/master/modules/angular2/src/upgrade/upgrade_adapter.ts#L341

Changing ngZone.run to ngZone.runOutsideAngular there seems to fix it or make it more manageable.

The text was updated successfully, but these errors were encountered:

Fixes angular#6385

See angular#6385

This commit effectively reverts 7e0f02f as it was an invalid fix for angular#6385, that created a more significant bug, which was that changes were not always being detected. Angular 1 digests should be run inside the ngZone to ensure that async changes are detected. We don't know how to fix angular#6385 without breaking change detection at this stage. That issue is triggered by async operations, such as `setTimeout`, being triggered inside scope watcher functions. One could argue that watcher functions should be pure and not do work such as triggering async operations. It is possible that the original use case could be supported by moving the debounce logic into the watch listener function, which is only called if the watched value actually changes. Closes angular#10660, angular#12318, angular#12034

This commit effectively reverts 7e0f02f as it was an invalid fix for #6385, that created a more significant bug, which was that changes were not always being detected. Angular 1 digests should be run inside the ngZone to ensure that async changes are detected. We don't know how to fix #6385 without breaking change detection at this stage. That issue is triggered by async operations, such as `setTimeout`, being triggered inside scope watcher functions. One could argue that watcher functions should be pure and not do work such as triggering async operations. It is possible that the original use case could be supported by moving the debounce logic into the watch listener function, which is only called if the watched value actually changes. Closes #10660, #12318, #12034 PR Close #13812

This commit effectively reverts 7e0f02f but for `upgrade/static` as it was an invalid fix for angular#6385, that created a more significant bug, which was that changes were not always being detected. Angular 1 digests should be run inside the ngZone to ensure that async changes are detected. We don't know how to fix angular#6385 without breaking change detection at this stage. That issue is triggered by async operations, such as `setTimeout`, being triggered inside scope watcher functions. One could argue that watcher functions should be pure and not do work such as triggering async operations. It is possible that the original use case could be supported by moving the debounce logic into the watch listener function, which is only called if the watched value actually changes. See angular#13812

This commit effectively reverts 7e0f02f but for `upgrade/static` as it was an invalid fix for #6385, that created a more significant bug, which was that changes were not always being detected. Angular 1 digests should be run inside the ngZone to ensure that async changes are detected. We don't know how to fix #6385 without breaking change detection at this stage. That issue is triggered by async operations, such as `setTimeout`, being triggered inside scope watcher functions. One could argue that watcher functions should be pure and not do work such as triggering async operations. It is possible that the original use case could be supported by moving the debounce logic into the watch listener function, which is only called if the watched value actually changes. See #13812 PR Close #14039

This commit effectively reverts 7e0f02f as it was an invalid fix for angular#6385, that created a more significant bug, which was that changes were not always being detected. Angular 1 digests should be run inside the ngZone to ensure that async changes are detected. We don't know how to fix angular#6385 without breaking change detection at this stage. That issue is triggered by async operations, such as `setTimeout`, being triggered inside scope watcher functions. One could argue that watcher functions should be pure and not do work such as triggering async operations. It is possible that the original use case could be supported by moving the debounce logic into the watch listener function, which is only called if the watched value actually changes. Closes angular#10660, angular#12318, angular#12034 PR Close angular#13812

dbpieter · 2017-02-16T10:04:56Z

@petebacondarwin Shouldn't this bug be reopened after #13812 ?

We are experiencing issues resulting in the infamous "$digest already in progress"

louwie17 · 2017-03-02T20:31:13Z

@petebacondarwin Did you have any updates on this problem, or a tentative idea on how to tackle/implement it?

This fixes angular#6385 by running `$rootScope.$digest` in a child zone. This prevents tasks started in the digest cycle from triggering the digest again, thus preventing the infinite loop.

kempcarlos · 2017-04-18T11:10:21Z

@petebacondarwin any update? Will this be fixed in the near future?

KirillMetrik · 2017-06-21T12:44:06Z

+1 Any update on this please?

dreef3 · 2017-08-26T07:37:33Z

Hi,
This issue effectively prevents us from upgrading a hybrid ng1/ng2 app to Angular 2.4.6+. Do you have any thoughts on how this could be resolved? I'd submit a PR then.

What about the comment here that states that #13812 also reverted the line which replaced $digest with $evalAsync?

This was originally introduced in #9054.

andreialecu · 2017-08-26T08:30:13Z

I'm also blocked by this. Haven't been able to upgrade to new versions since this was reverted.

I'm not sure the frequency of this issue is low either. It's hard to track because infinite digests still allow the app run properly but it's just that the performance is very bad. I was only able to realize what was happening via a chrome extension that was showing me angular 1 digest frequency.

gkalpak · 2017-08-26T14:27:31Z

We are generally not keen on changing $digest to $evalAsync for the digest that is run when the microtask queue is empty, because this tends to hide programming errors (which often have hard-to-track-down performance implications).

This unfortunately means that running setTimeout in a watcher will lead to infinite digest errors, unless you change it to run outside of the Angular zone.
_{To be clear, you can still use setTimeout in the watch listener function:}

scope.$watch(
  () => {
    // Watch function.
    // Do NOT use `setTimeout()` here (unless you explicitly run it outside the Angular zone).
  },
  () => {
    // Watch listener function.
    // You can use `setTimeout()` here.
  }
)

The new downgradeModule(), which decouples the change detections of the two frameworks, could possibly help in such situations. (Docs will be added with #18487.)

This commit effectively reverts 7e0f02f as it was an invalid fix for angular#6385, that created a more significant bug, which was that changes were not always being detected. Angular 1 digests should be run inside the ngZone to ensure that async changes are detected. We don't know how to fix angular#6385 without breaking change detection at this stage. That issue is triggered by async operations, such as `setTimeout`, being triggered inside scope watcher functions. One could argue that watcher functions should be pure and not do work such as triggering async operations. It is possible that the original use case could be supported by moving the debounce logic into the watch listener function, which is only called if the watched value actually changes. Closes angular#10660, angular#12318, angular#12034 PR Close angular#13812

This commit effectively reverts 7e0f02f but for `upgrade/static` as it was an invalid fix for angular#6385, that created a more significant bug, which was that changes were not always being detected. Angular 1 digests should be run inside the ngZone to ensure that async changes are detected. We don't know how to fix angular#6385 without breaking change detection at this stage. That issue is triggered by async operations, such as `setTimeout`, being triggered inside scope watcher functions. One could argue that watcher functions should be pure and not do work such as triggering async operations. It is possible that the original use case could be supported by moving the debounce logic into the watch listener function, which is only called if the watched value actually changes. See angular#13812 PR Close angular#14039

KirillMetrik · 2018-03-22T14:50:21Z

Have you please got any update on this? The bug really prevents us from upgrading to latest Angular and we're quite stuck with AngularJS 1.

splincode · 2018-07-30T10:57:48Z

@andreialecu @trotyl What do you think? Is the issue relevant?

It seems to me that the problem is connected with calling setTimeout
#24133

mhevery · 2018-08-07T18:34:08Z

Will not fix: not feasible.

KirillMetrik · 2018-10-05T11:48:54Z

How should we address upgrading our app from Angular 1 if this is not feasible? We're having a big enterprise project and have chosen AngularJS (1) because it was well supported and promoted by Google. One of the reasons was that we were hoping the project will stay maintanable in time. Instead, Angular 2-6 is a completely new framework, practically incompatible with AngularJS and there is no way to upgrade because of this issue.
@mhevery do you please have any idea what to do in our case? Or should we just throw away all our code and write everything from scratch?
If that's the only option it might make sense for us to switch to React.

gkalpak · 2018-10-05T14:27:42Z

@KirillMetrik, the thing that is not feasible is using setTimeout() in a watch function, unless you either explicitly run it outside the Angular zone or use downgradeModule() (see #6385 (comment)).

All this means is that (if you choose not to use downgradeModule()) you have to modify your code to either not use setTimeout() inside a watch function (which is weird anyway) or modify your code to run it outside the Angular zone. I don't see how this is preventing your from upgrading. You will have to change that code anyway sooner or later (i.e. you will have to get rid of scope.$watch() at some point).

angular-automatic-lock-bot · 2019-09-14T02:52:22Z

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.}

This commit effectively reverts 7e0f02f as it was an invalid fix for angular#6385, that created a more significant bug, which was that changes were not always being detected. Angular 1 digests should be run inside the ngZone to ensure that async changes are detected. We don't know how to fix angular#6385 without breaking change detection at this stage. That issue is triggered by async operations, such as `setTimeout`, being triggered inside scope watcher functions. One could argue that watcher functions should be pure and not do work such as triggering async operations. It is possible that the original use case could be supported by moving the debounce logic into the watch listener function, which is only called if the watched value actually changes. Closes angular#10660, angular#12318, angular#12034 PR Close angular#13812

andreialecu mentioned this issue Jan 10, 2016

bug(perf): angular2/upgrade keeps running $rootScope.apply() if watchers call setTimeout #6245

Closed

andreialecu added a commit to andreialecu/angular that referenced this issue Jan 10, 2016

fix(upgrade): fix infinite $rootScope.$digest()

2eeb483

Fixes angular#6385

andreialecu mentioned this issue Jan 10, 2016

fix(upgrade): fix infinite $rootScope.$digest() #6386

Closed

andreialecu added a commit to andreialecu/angular that referenced this issue Jan 10, 2016

fix(upgrade): fix infinite $rootScope.$digest()

faf2ef5

Fixes angular#6385

andreialecu changed the title ~~bug: infinite digests with angular2/upgrade and setTimeout (repro inside)~~ bug(upgrade): infinite digests with setTimeout (repro inside) Jan 11, 2016

andreialecu added a commit to andreialecu/angular that referenced this issue Jan 13, 2016

fix(upgrade): fix infinite $rootScope.$digest()

011f42f

Fixes angular#6385

IgorMinar closed this as completed in 7e0f02f Feb 9, 2016

kvantetore mentioned this issue Feb 24, 2016

When upgrading from angular1, updating a $watched value from a setTimeout ends up outside ngZone #7252

Closed

huyph mentioned this issue Aug 11, 2016

[upgrade-adapter-rc.4] change detection does not work sometime on a hybrid app #10660

Closed

petebacondarwin added a commit to petebacondarwin/angular that referenced this issue Dec 16, 2016

test(upgrade): async operations in watchers should not trigger digest

124b305

See angular#6385

petebacondarwin added a commit to petebacondarwin/angular that referenced this issue Dec 17, 2016

test(upgrade): async operations in watchers should not trigger digest

63a9eac

See angular#6385

petebacondarwin added a commit to petebacondarwin/angular that referenced this issue Dec 17, 2016

test(upgrade): async operations in watchers should not trigger digest

803b05c

See angular#6385

petebacondarwin added a commit to petebacondarwin/angular that referenced this issue Dec 18, 2016

test(upgrade): async operations in watchers should not trigger digest

d86cd7c

See angular#6385

petebacondarwin mentioned this issue Jan 6, 2017

fix(upgrade): detect async downgrade component changes #13812

Closed

petebacondarwin mentioned this issue Jan 20, 2017

fix(upgrade): detect async downgrade component changes #14039

Closed

12 tasks

petebacondarwin added state: Needs Design type: bug/fix workaround2: non-obvious labels Feb 16, 2017

usernamealreadyis mentioned this issue Mar 3, 2017

Rolup 5.2.0 failing with 4.0 rc2 and RxJS 5.2.0 #14900

Closed

heathkit mentioned this issue Mar 15, 2017

fix(upgrade): Run digest cycle for upgraded components in a child zone. #15157

Closed

3 tasks

heathkit mentioned this issue Apr 24, 2017

Timeout with angular 4/2 app with protractor 5.x angular/protractor#4234

Open

heathkit mentioned this issue May 17, 2017

fix(upgrade): call setInterval outside the Angular zone #16836

Merged

1 task

ngbot bot added this to the Backlog milestone Jan 23, 2018

mhevery closed this as completed Aug 7, 2018

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bug(upgrade): infinite digests with setTimeout (repro inside) #6385

bug(upgrade): infinite digests with setTimeout (repro inside) #6385

andreialecu commented Jan 10, 2016

dbpieter commented Feb 16, 2017 •

edited

Loading

louwie17 commented Mar 2, 2017 •

edited

Loading

kempcarlos commented Apr 18, 2017

KirillMetrik commented Jun 21, 2017

dreef3 commented Aug 26, 2017

andreialecu commented Aug 26, 2017

gkalpak commented Aug 26, 2017 •

edited

Loading

KirillMetrik commented Mar 22, 2018

splincode commented Jul 30, 2018 •

edited

Loading

mhevery commented Aug 7, 2018

KirillMetrik commented Oct 5, 2018

gkalpak commented Oct 5, 2018

angular-automatic-lock-bot bot commented Sep 14, 2019

bug(upgrade): infinite digests with setTimeout (repro inside) #6385

bug(upgrade): infinite digests with setTimeout (repro inside) #6385

Comments

andreialecu commented Jan 10, 2016

dbpieter commented Feb 16, 2017 • edited Loading

louwie17 commented Mar 2, 2017 • edited Loading

kempcarlos commented Apr 18, 2017

KirillMetrik commented Jun 21, 2017

dreef3 commented Aug 26, 2017

andreialecu commented Aug 26, 2017

gkalpak commented Aug 26, 2017 • edited Loading

KirillMetrik commented Mar 22, 2018

splincode commented Jul 30, 2018 • edited Loading

mhevery commented Aug 7, 2018

KirillMetrik commented Oct 5, 2018

gkalpak commented Oct 5, 2018

angular-automatic-lock-bot bot commented Sep 14, 2019

dbpieter commented Feb 16, 2017 •

edited

Loading

louwie17 commented Mar 2, 2017 •

edited

Loading

gkalpak commented Aug 26, 2017 •

edited

Loading

splincode commented Jul 30, 2018 •

edited

Loading