digest is skipping watches when a watch addes watches in its watch function. #15422
Comments
|
According to this comment, reverse traversal is used for speed. I wonder what impact it would have to change the order of traversal. |
|
BTW, the obvious work-around is to use |
|
yes that's what i already did and that works. 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) |
|
Between:
I think I lean towards (3). |
|
some pure "pseudo" code from my end would be something like this: traverseScopesLoop: 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); we adjust the length (the counter property) based on if there are new watches. 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) |
I assume doing all this --
Appending while traversing in reverse order breaks the requirement of traversing watchers in the same order in which they were added. --
This is obviously a bug. Let's fix that. -- |
|
What if you do something similar to what jcompagner suggested but with smarter removal of watches? 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. |
|
removing is i think not a big deal right now except that watches are checked twice in the same digest loop 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) |
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)
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)
|
I have implemented the -- Keep in mind that |
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
|
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? |
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
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
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
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)
The text was updated successfully, but these errors were encountered: