$q: Control flow duplication (forking) when uncaught error is handled in .then()

[youurayy]

Triggered when:

$q.resolve().then(function() {
  throw new Error();
}).catch(function(e) {
  // ok, but the catch-all handler also receives `e` (incorrect)
});

Correctly handled when:

$q.resolve().then(function() {
  try {
    throw new Error();
  }
  catch(e) {
    return $q.reject(e);
  }
}).catch(function(e) {
  // ok, and the catch-all handler has not been activated (correct)
});

Relevant code:

angular.js/src/ng/q.js

Line 366 in c9dffde

exceptionHandler(e);

1. line 365: the Error is correctly passed up the promise chain: deferred.reject(e);
2. line 366: the Error is also passed to the exceptionHandler(e);, which effectively forks the control flow, if the Promise chain in 1. was not ignored.

Normally this is not a ciritical issue, since exceptionHandler() is usually used to just print errors to the console, however correctly the error would make it to the exceptionHandler() only if it was not ignored by the deferred.reject(e); chain (which, I'm not sure if it's even feasible to implement).

Possibly related; #7992, #13653

[gkalpak]

This is so by design. Passing it to the $exceptionHandler is not forking the control flow. $exceptionHandler can hardly be used for controlling the flow (in a sensible way) - it's more like a notification hook for the developer imo.

There has been a debate (of medium length) about whether we should pass the thrown error to the $exceptionHandler or not. I think the main problem is that we can't distinguish a deliberately thrown error (e.g. throw new Error('Thrown just to reject the promise')) from an actual programming error (e.g. (undefined).foo).

I think the verdict was that making it configurable won't harm (but we haven't decided on the default setting yet 😛 )

If anyone wants to submit a PR, that would be awesome (should be an easy change - famous last words).

[youurayy]

Thanks for the response!

The control flow is "forked" in a sense that if there is e.g. an "email to developer" in the catch-all handler, and also in the .catch() on the Promise chain, the developer will get two emails:)

I'll see about the PR, I'm super-busy lately.. (aren't we all?)

[wesleycho]

This sounds like a breaking change, and one goes against the Promise A+ spec - see relevant line here. I would recommend against this.

[youurayy]

@wesleycho do you mean "If either onFulfilled or onRejected throws an exception e, promise2 must be rejected with e as the reason." ?

Yes that is the correct behavior and it already happens.

However the problem is that it is not the only thing that happens in that case, but the error is also (and firstly) reported to Angular's uncaught exception handler - which is incorrect because the definition of an uncaught exception is, well, one that is uncaught. However in this particular case the exception is caught by the Promise.then() and then, passed to .catch(), and only if there are no callbacks registered in any of the .catch() upwards in the Promise call chain, only then is the Exception actually uncaught and should be passed to the Angular's $exceptionHandler - IMHO.

https://docs.angularjs.org/api/ng/service/$exceptionHandler

[wesleycho]

Ah, so you're proposing to just have the exceptionHandler call be removed and let it defer down to the unprocessed exception check then. That sounds reasonable, probably wouldn't be difficult to put together a PR doing so.

[lgalfaso]

@youurayy as stated #14745 (comment) this was discussed. I cannot speak in behalf of the core team, but I think that the transformation of catching an exception and generating a rejected promise is something that needs to be tracked (and if there is a need for this not to be tracked, then the promise should return a rejected promise).

[gkalpak]

To be clear, what we are talking about here is just this line and that line; whether or not exceptionHandler should be called. We are not debating the rejection; it must happen according to the spec.

What I was proposing above, is to make it configurable - similar to how the user can turn on/off the unhandled rejection error. The user should be able to decide whether the exceptionHandler should be called or not when an error is thrown inside a promise.

[youurayy]

@gkalpak - the behavior where the exceptionHandler gets called only when the rejected promise was not handled by any user-space Promise .catch handler -- would that behavior be hard to implement? At the end of the error promise chain, that's where the angular catch-all redirector to exceptionHandler should be sitting I think.