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

digest is skipping watches when a watch addes watches in its watch function. #15422

Closed
jcompagner opened this issue Nov 23, 2016 · 10 comments

Comments

@jcompagner
Copy link

commented Nov 23, 2016

Note: for support questions, please use one of these channels: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#question. This repository's issues are reserved for feature requests and bug reports.

Do you want to request a feature or report a bug?

Bug

What is the current behavior?

because the $digest function works from length to 0, counting down and the $watch() uses unshift() to prepend the new watch one digest loop will skip watches.. (and executes them in a next one and that can be in a loop hitting the max 10)

as an example i have 100 watches, the current counter (the length variable) is on 75
that 75 watch is then adding 25 watches those are prepended so now i have 125 watches and the new once are 0-25. The length counter is then decremented to 74, But on the 74 position is not the value that was there before but suddenly what was 49, so 50-74 are all skipped.

This is problematic for us because in those 50-74 we have the same kind of watches that all add a few watches. So if that are 10+ then we will get the "digest cycle aborted because of 10 nested calls"
Which is in our eyes not really the case, it should do them as one digest. (or 2 doing all the added watches one time after the first one)

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem via https://plnkr.co or similar (template: http://plnkr.co/edit/tpl:yBpEi4).

http://plnkr.co/edit/ojUQkuVvLLWwxxoksdJK?p=preview

see this plnkr i just expect that this runs with 1 or maybe 2 digest cycles, but now it aborts because it hits 10

What is the expected behavior?

The current behavior with the length counter just counting down in a loop, is not really correct it should be more like a concurrent iterator. even if there are more added it should never skip.
It should always really get the next one and the next one that was really in line.

Personally i could live with that the watch doesn't prepend but just append, that would mean that all the watches that are generated by watches are appended and skipped in that loop, but those are then executed all the next time. IF those then add again watches and that would hit the 10 digest cycles that would be a problem in the code.. That is a developers problem. (you shouldn't not constantly add watches) . But we don't do that we only do it once. That we expect are done in 1 digest cycle.

What is the motivation / use case for changing the behavior?

Which versions of Angular, and which browser / OS are affected by this issue? Did this work in previous versions of Angular? Please also test with the latest stable and snapshot (https://code.angularjs.org/snapshot/) versions.

Angular 1.5.8 (all browsers)

Other information (e.g. stacktraces, related issues, suggestions how to fix)

@gkalpak

This comment has been minimized.

Copy link
Member

commented Nov 23, 2016

According to this comment, reverse traversal is used for speed. I wonder what impact it would have to change the order of traversal.

@gkalpak

This comment has been minimized.

Copy link
Member

commented Nov 23, 2016

BTW, the obvious work-around is to use $evalAsync for adding the extra watchers.

@jcompagner

This comment has been minimized.

Copy link
Author

commented Nov 23, 2016

yes that's what i already did and that works.
That whole counting from length to 0 and unshift seems to be already in angular for a long time, so i don't get why only now this popups up. I guess adding watches in a watch does happen but not in the numbers that we do it currently, so i guess for many it keeps just below that 10.

But then it still quite a performance hit, i looked how many digest cycles where done,with using $evalAsync, now and that's only 3 instead of 15+ (we have TTL set to 15)

@gkalpak

This comment has been minimized.

Copy link
Member

commented Nov 23, 2016

Between:

  1. Calling this a "known issue" and advicing people to not add watchers synchronously during a digest.
  2. Changing the traversal order and evaluating possible performance implications (if any).
  3. Letting Angular automatically defer the addition of new watchers during a digest (i.e. doing $evalAsync under the hood).

I think I lean towards (3).

@jcompagner

This comment has been minimized.

Copy link
Author

commented Nov 23, 2016

some pure "pseudo" code from my end would be something like this:

traverseScopesLoop:
do { // "traverse the scopes" loop
if ((watchers = current.$$watchers)) {
// process our watches
length = watchers.length;
var startLength = length;
while (length--) {

so when starting we really remember the startLength of the watchers.

Then when something is dirty and right after the watch value changed method call:

fn(value, ((last === initWatchVal) ? value : last), current);
length += (watchers.length - startLength)
startLength = watchers.length;

we adjust the length (the counter property) based on if there are new watches.
That also adjust for watches being removed. and that would be fine if the watches that are removed are in the part smaller then the current lenght property (then at least it doesn't execute the same watch twice).
But it would start skipping if the watch was removed that was already executed (so > length)
So this is not really fully bullet proof.

My feeling is that watches shouldn't be prepended but always appended and then the a second digest should happen (its still dirty) and that would just go over the new set. But i am not sure why it is important to really execute the added watches right away. (and now skipping the watches that where already there, so that sounds to me like a worse situation)

@gkalpak

This comment has been minimized.

Copy link
Member

commented Nov 23, 2016

length += (watchers.length - startLength)

I assume doing all this .length checks is what the current implementation is trying to avoid.
If it weren't for that we, could simple do a forward traversal until index < watchrs.length.

--

My feeling is that watches shouldn't be prepended but always appended and then the a second digest should happen (its still dirty) and that would just go over the new set.

Appending while traversing in reverse order breaks the requirement of traversing watchers in the same order in which they were added.

--

But i am not sure why it is important to really execute the added watches right away. (and now skipping the watches that where already there, so that sounds to me like a worse situation)

This is obviously a bug. Let's fix that.

--
As you mentioned, the same issue affcts removed watchers as well 😞

@costescuandrei

This comment has been minimized.

Copy link

commented Nov 23, 2016

What if you do something similar to what jcompagner suggested but with smarter removal of watches?
So when watches are added at the beginning increase the counter by the number of added watches, and when watches are deleted, see if they are deleted from before or after counter and adjust counter again only if needed. The code would be a bit larger then though :).

Option 3 above with $evalAsync under the hood would not work correctly for removal of watches (just for adding watches) I think - as you might end up with watches that client code removed still executing once.

@jcompagner

This comment has been minimized.

Copy link
Author

commented Nov 23, 2016

removing is i think not a big deal right now except that watches are checked twice in the same digest loop
we have 0-10 watches

test 1: i am on 5 and that one removes 0-4 , then the watches will become 0-6 with the counter on 4 so the previous 5-8 will be checked again.

test 2: i am on 10 and that removes 0-9 right away, then the loop will just hit 987654321 and won't find anything thing (there is a check for that) and then 0 which was 10 is checked again

so for now its hitting or undefined entries or entries already checked, but that check should be quite quick (thats the whole point of a watch)

So that doesn't seem to be a big problem, except when really fixing the above issue, because then you have to take into account that also deletes can happen so deletes won't skip...

But if we don't want the length check to be inside the loop (Also not when a watch is dirty) then we have a bit of a problem. Because thats the only fix in the current loop. The only other scenario i can think of now is that we create a copy on write (we could do copy on read but that makes the digest way more expensive and that happens more then a write i think)

So in the digest loop we get the reference to the current watches (like we already do)
but all the watch methods don't touch that array but create a copy with the new stuff in it.
Then the digest loop i guess only needs to check after its done with the current array if the array was updated and so on get that one and start over (i think trying to be smart and making the start index something like: newIndex - oldIndex that we hit the problem with deletes again)

jcompagner added a commit to Servoy/servoy-client that referenced this issue Nov 23, 2016
work around an angular bug:
angular/angular.js#15422

when adding watches in a watch, then angular skips watches that only
executes the next time. (and the next and the next if the skipped
watches would add watches again)
jcompagner added a commit to Servoy/servoy-client that referenced this issue Nov 23, 2016
work around an angular bug:
angular/angular.js#15422

when adding watches in a watch, then angular skips watches that only
executes the next time. (and the next and the next if the skipped
watches would add watches again)
@gkalpak

This comment has been minimized.

Copy link
Member

commented Nov 23, 2016

I have implemented the $evalAsync solution and I think it is reasonable to have the watchers added (and executed) after the current digest cycle.

--
For removed watchers, as @jcompagner said, there is a chance that the removing watcher gets executed twice in a row (within the same digest cycle). This wouldn't be terrible on its own, except for an implementation detail that will currently cause the digest loop to stop and all subsequent watchers to be skipped.
EDIT: I was wrong. This behaves correctly (with the exception of running the same watcher twice).

Keep in mind that $digest() is super hot (as it gets executes very often) and performance is important. If there is no way to solve this with a degrading performance, it is preferrable to just document it as a "known issue" and move on. The work-around is trivial: Just use $evalAsync to register/unregister watchers from within a watch-function or watch-callback.

gkalpak added a commit to gkalpak/angular.js that referenced this issue Nov 23, 2016
Previously, adding a watcher during a `$digest` (i.e. from within a watcher),
would result in the next watcher getting skipped. Similarly, removing a watcher
during a `$digest` could result in the current watcher being run twice (if the
removed watcher had not run yet in the current `$digest`).

This commit fixes both cases by keeping track of the current watcher index
during a digest and properly updating it when adding/removing watchers.

Fixes angular#15422
@gkalpak

This comment has been minimized.

Copy link
Member

commented Nov 23, 2016

Well, it turns out that while asynchronously adding the watcher works, it creates several potential corner cases and hard to debug inconsistencies. E.g. what happens if the scope is destroyed in the meantime? What happens if the user has called the returned deregistration function before adding the watcher?

I have a "proper" fix for both adding and removing in #15424. @jcompagner, can you verify that it solves your issue?

@gkalpak gkalpak closed this in 1c0c260 Dec 9, 2016
gkalpak added a commit that referenced this issue Dec 9, 2016
Previously, adding a watcher during a `$digest` (i.e. from within a watcher),
would result in the next watcher getting skipped. Similarly, removing a watcher
during a `$digest` could result in the current watcher being run twice (if the
removed watcher had not run yet in the current `$digest`).

This commit fixes both cases by keeping track of the current watcher index
during a digest and properly updating it when adding/removing watchers.

Fixes #15422

Closes #15424
gkalpak added a commit that referenced this issue Dec 9, 2016
Previously, adding a watcher during a `$digest` (i.e. from within a watcher),
would result in the next watcher getting skipped. Similarly, removing a watcher
during a `$digest` could result in the current watcher being run twice (if the
removed watcher had not run yet in the current `$digest`).

This commit fixes both cases by keeping track of the current watcher index
during a digest and properly updating it when adding/removing watchers.

Fixes #15422

Closes #15424
ellimist added a commit to ellimist/angular.js that referenced this issue Mar 15, 2017
Previously, adding a watcher during a `$digest` (i.e. from within a watcher),
would result in the next watcher getting skipped. Similarly, removing a watcher
during a `$digest` could result in the current watcher being run twice (if the
removed watcher had not run yet in the current `$digest`).

This commit fixes both cases by keeping track of the current watcher index
during a digest and properly updating it when adding/removing watchers.

Fixes angular#15422

Closes angular#15424
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.