Improve tracability of "$digest already in progress" error

[bevacqua]

We need to improve this error as it is currently very obscure and hard to track down.

My suggestion would be to turn it into a $log warning that doesn't throw an error but still prints a stack trace.

Could we make it so that anything that would eventually trigger a $digest cycle check for $$phase at their entry point, and then have them print a warning and return?

If not, in which ways do you believe we could improve the traceability of $digest invocation overlaps?

_{Refer to this StackOverflow question for background}

[IgorMinar]

I don't think that we can get rid of the error as when this situation happens, we need to prevent the second digest otherwise the whole system could get into a weird state.

What we could do is keep the stack trace from when each digest is triggered and if double digest occurs, we could then print both the original stack trace as well as the current stack trace.

Would that be sufficient?

[IgorMinar]

(btw I edited the subject of the issue because as I said we can't get rid of the error)

[mhelvens]

@IgorMinar: But isn't the original stack trace —by the very nature of this error— contained inside the current stack trace?

PS: I took the opportunity to post an answer to the Stackoverflow question. It includes the reason I find the error message useful (my guess as to the motivation of the Angular developers), as well as a general background and a way of debugging it. This is all based on my own experience. If I made any mistakes, please do correct them.

[bevacqua]

@IgorMinar that would be pretty useful.

In the last occasion I ran into the bug, I added a couple of logging statements in Angular, and found out that my issue was because I was adding a watch on a directive, and using ngModel.$setViewValue in the watch. That ended up triggering a second digest loop, this feels like a bug to me. If you want me to, I can work out a simplified example and open another issue.

I can work on a pull request for this issue, keeping track of a stack trace for the previous digest, and then reporting both.

[afischer-sfly]

+1 for the suggestion to change this into a log warning instead of a thrown exception: an exception is too harsh for this situation. In most cases, ignoring the second $digest won't actually cause any visible problems. I think the only risk is that the code calling the second $digest might have just made a model change which will be missed until the next cycle, because the current cycle already looked at that model field.

Message could be something like "Warning: $digest already in progress, this $digest call will be ignored. Any recent scope modifications may have been missed by this cycle."

Oh and another +1 for showing traces of both $digest calls.

[IgorMinar]

@petebacondarwin and I went through the scenarios that could be affected if we stop throwing the error and instead just ignored the second digest and ensured that the first digest loop observed all the changes made by the nested apply() call.

Everything we could think of worked as expected using this approach with the exception of testing.

In tests it is common to not wrap the code under test into an $apply and instead just call $digest or $apply before assertions. If we ignored the nested digests, that would mean that they would turn into real digests in tests and would affect timing and execution order:

function MyController($scope) {
  $scope.userAge = null;

  $scope.doStuff = function(val) {
    $scope.userAge = val;
    if ($scope.userAge) {
      doA();
      doSomethingElse(); //calls $apply

      $http.get('/user/by/age/' + $scope.userAge) // makes request to /user/by/age/200 if called with val===200
    }
  }  

  $scope.$watch('userAge', function(newAge) {
    if (newAge > 150) $scope.userAge = null;
  });
}


it('should ', function() {
  $httpBackend.expectGET('/user/by/age/200');

  myController.doStuff(200); // fails because we called /user/by/age/null
  $scope.$digest();
});

Making the code running the same in tests as in production is something we care a lot about. This is why removing the check is not desirable.

If you use safeApply or $$phase approach then you are at the risk of hitting the scenario described above.

Interestingly removing the $apply calls via zone.js would deal with this problem but zone.js is IE10+ so that's currently not useful.

[petebacondarwin]

For general information here are a couple of Plunkers related to the original issue:

Broken:
http://plnkr.co/edit/rAPvuRLx64uavXvPDoRq?p=preview

Workaround:
http://plnkr.co/edit/OXjKjYgfGg9scd2c5BgU?p=preview

I am going to work on improving the documentation to help when faced with this exception.

[petebacondarwin]

@IgorMinar - I have merged the docs changes. I don't think that there is much more we can do about improving the error message for this as the stack trace has all the information we can provide already.

Shall we close this issue?