$q reports errors in promise callbacks even when they are caught

[domenic]

Plunker: http://plnkr.co/edit/5vMj4AJibWcnTKYR5z73?p=preview

I have caught the error and used it to display useful information to my user in the UI. However, Angular still logs a big ol' stack trace to the console.

This isn't a spec compliance issue (since the error isn't thrown, just logged), but it's a developer ergonomics issue. I don't understand how using $q on any reasonably large promise project would not just result in a console filled with spurious errors.

As always, the correct time to report a rejection as unhandled is debatable. Bluebird does it at the end of turn, whereas Q doesn't do it at all, relying on .done() instead. But this isn't even about unhandled rejections---it's about all rejections!

The Bluebird strategy seems like a pure win: it would remove this spurious reporting, but keep it for any cases that could potentially be real.

[caitp]

This doesn't seem unreasonable, I can put together a patch for this tomorrow

[pkozlowski-opensource]

Agreed that the systematic logging of exceptions thrown from callbacks is a bit "noisy". This was already discussed here: #7187 (comment)

[caitp]

So, I don't think I fully understand Bluebird's way of doing this --- they manage to determine if a promise is handled synchronously and asynchronously (example http://jsfiddle.net/8ga3U/).

I think this is fixable by adding some extra state to $q Promise objects, so that tracking whether a rejection is handled or not can happen when then() is called, rather than during resolution (which my current prototype does)

My current prototype "solves" this by adding an extra task queue to be consumed after the nextTick() queue, so that handled rejections can turn off their 'logging'. Only thrown exceptions behave this way, so normal rejections are just processed regularly (talking about my code here, although I believe the code in the tree behaves the same way). Thrown errors are wrapped, and error handlers can mark errors as handled, so that a scheduled error logger doesn't need to do anything. This works, but obviously only hides messages when rejections are handled in the same turn that they're resolved in, which is basically never

---

So there's a current implementation (by which I mean, my work-in-progress, not code in the tree) which doesn't work very well in a lot of cases, and a proposed solution (which I think should be good enough, but I'm not sure without trying it). There were plans a while ago to change the Promise implementation to use prototype chains to avoid creating so much garbage, but not much progress has been made there yet. However, finishing that would make it a bit easier to add extra state to the promise objects in a manageable way, I think.

It's a bit sad that it's turning out to be harder than expected to improve the error logging in $q, but I can keep hacking on it.

[domenic]

Only thrown exceptions behave this way, so normal rejections are just processed regularly

This is a bit frustrating; they should be symmetric.

Tagging in @petkaantonov to see if he can help explain Bluebird's technique.

[caitp]

I think I sorted out my understanding of that particular part of it while writing my little summary there -- I guess Bluebird must be deciding of a rejection is handled at the time handlers are registered (eg then), and not during resolution. That would make sense.

So angular doesn't really have a single then-able, but more like 3 different ones, and it seems not very fun to try and make them all behave the same way in this case, which is why I think unifying those into a single class might make sense first

[petkaantonov]

The simple version goes like this:

1. When a promise gets rejected and it doesn't have any rejection handlers attached to it, turn on a flag on the promise and queue a method check to be called after the current queue is completely flushed.

Adding a rejection handler to a promise sets the flag off if it's on. (In cases like promise.all which is optimized to not even attach any handlers on a promise, this is simulated).

In function check

1. If the promise's flag is still on, trigger a possibly unhandled rejection event with the promise and the rejection reason as data.

Then the default handler for the event is to log stack trace to stderr or console.error.

The actual implementation used in bluebird is a bit different because it provides another event for possibly unhandled rejection handled which requires another flag. There is no default handler for that event - but if something like console.unlog existed, it could be used in response to this event.

[prescod]

The issue report says: "This isn't a spec compliance issue (since the error isn't thrown, just logged), but it's a developer ergonomics issue."

But this is incorrect. The error is not just logged. The error is sent to whatever exceptionHandler service is configured, which by default logs. But in some apps it might pop up a modal for the user, or send a message to the server or log to localStorage.

[caitp]

The exceptionHandler service (with the exception of the mock exceptionHandler service) isn't really "configurable".

You could decorate the exceptionHandler with a replacement to show a modal dialog, but in practice nobody really does this (*read: very few people do this, if any). Since there are so many things which cause $exceptionHandler to be invoked, and there are so many things you probably don't want to display a modal dialog for, and you don't have the context of whatever caused the exception to be thrown, the chances of doing anything interesting with it are next to nil.

We can effectively treat $exceptionHandler as a logging service

[prescod]

Even if we agreed that it is only a logging service, it's a fact that some people are logging to the server side:

http://www.davecap.com/post/46522098029/using-sentry-raven-js-with-angularjs-to-catch

So the amount of useless stuff in this log could get crazy.

But with respect to popping up a modal: my intent in an app was to look at the exception name and pop up a modal based on exception type. That's not much different than having a "top level exception handler" which reports some exceptions more aggressively than others.

[just-boris]

Now I can silently rethrow error using $q.reject()

$http.get().catch(function(err) {
   if(!canBeHandled) {
       // propagate error to further promises
       return $q.reject(err);
   }
});

But that means that all my services/controllers which use $http should include $q. This is a bit annoying. It would be better to propagate error using throw as well as other promise libraries do.

$http.get().catch(function(err) {
   if(!canBeHandled) {
       // causes call to $exceptionHandler
       throw new Error(err);
   }
});

Now it calls $exceptionHandler and that means that it can't catch and suppress that error in further promises in chain.

[op1ekun]

@just-boris I totally agree 👍

I run into this problem while simplifying services based on $http (I was using Bluebird for a while, but calling $apply on consumer's side was cumbersome).

I spent some time to figure out that throw new Error() doesn't reject $http promise.
Quite annoying...

Thankfully the project is quite fresh so refactoring is not a major issue.
However I can feel the pain of ppl throwing errors in their $http services.

[xieranmaya]

What is thought to be the desired behavior is that it only report rejected promises' errors when there is no rejection handler registered on the promise(that is, never called then(*,handler) on it), which is like Q or Bluebird or ES6 primitive Promise or Node.js onerror event(if there is a onerror handler, the process will not exit).

#13653