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

[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?

Not sure what you mean. Isn't that already implemented?

[youurayy]

No, the behavior I'm observing is that the exceptionHandler gets called even when the exception is handled by an user-space .catch handler. Moreover, the exceptionHandler gets to handle the exception before the user-defined .catch handler.

[gkalpak]

@youurayy, there are several exceptionHandler calls (inside $q). I think the main ones are 2:

1. Called when an error is thrown (as in throw ...). This is done in addition to rejecting the promise.
2. Called when there is a possibly unhandled rejection.

The second category is not relevant with what we are discussing here. This issue is about the first category of calls. Regardless of what we do with the first category though, the unhandled rejections will still be treated as they are today (i.e. calling exceptionHandler).

[gkalpak]

Or maybe I am totally missing your point (it's not that it hasn't hapened before 😃).

An example might help:

// Assuming:
function onSuccess1(val) { console.log('onSuccess1'); return val; }
function onSuccess2(val) { console.log('onSuccess2'); return val; }
function onError1(err) { console.log('onError1'); return $q.reject(err); }
function onError2(err) { console.log('onError2'); return $q.reject(err); }
function onError3(err) { console.log('onError3'); }

p1 = somePromise1.then(function () { throw 'Error'; }).then(onSuccess1, onError1);
p2 = somePromise2.then(function () { throw 'Error'; }).then(onSuccess2, onError2).catch(onError3);

For p1, you will see onError1 in the console and the exceptionHandler will be called twice: once for the thrown error and once for the possibly unhandled rejection.
For p2, you will see onError2\nonError3 in the console and the exceptionHandler will be called once: for the thrown error.

I suggest that we make it configurable whether the first call to exceptionHandler is made or not (the one for the thrown error).

[mgol]

We should treat errors thrown in promise callbacks and returned rejections in the same way, all promise libraries do it like that. Let's do it in 1.6.0.