Angular.js: $q: Unhandled rejections should not be stringified

[dcherman]

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

Unhandled rejections are converted to a string prior to being passed to $exceptionHandler. Also, errors aren't correctly stringified either.

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://jsfiddle.net/rogqp47y/28/

What is the expected behavior?

The value that the promise was rejected with should be passed to $exceptionHandler

What is the motivation / use case for changing the behavior?

An unhandled rejection should be treated no differently than an unhandled exception - if I throw an uncaught error, I expect to catch that in the global $exceptionHandler, not a string representing the error. The default implementation of $exceptionHandler that logs to the console should handle the stringification.

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.

Git / Nightly (Future 1.6)

Other information (e.g. stacktraces, related issues, suggestions how to fix)

[gkalpak]

/cc @lgalfaso

[lgalfaso]

I do not think that there is a clear cut (this is that $exceptionHandler should receive the value from an unhandled rejected promise). The reason being that there is no reason to believe that this value will be an Error. This is, if the value is just passed to $exceptionHandler, there is no context to know that this came from an unhandled promise. That said, I am open to change https://github.com/angular/angular.js/blob/master/src/ng/q.js#L383 to exceptionHandler(extends(new UnhandledPromiseError(), {reason: toCheck.value}), errorMessage); (or something equivalent) UnhandledPromiseError being something that extends Error.

[gkalpak]

I agree that it should be clear why this is an error (i.e. possibly unhandled rejection).
I was thinking we might add an independent API about notifying for unhandled rejections. Something similar to the unhandledrejection event.

[dcherman]

So in the current code, I see that we're only passing along one argument to $exceptionHandler, but the second (optional) reason seems like it's be very reasonable to use here in order to report the reason the handler was invoked as "Possibly unhandled rejection" or something. Thoughts?

[lgalfaso]

I find a new service just for this somehow of an overkill. If we are going to change what is sent to $exceptionHandler (that I think it would be a good idea), I would still think it is better to send something that extends Error. How the value is placed in this error (and if we just use Error or something else) all variations are fine with me. About the error message, then having just "Possibly unhandled rejection" or what we currently send are both fine with me.

[vertazzar]

tracebacks look like this:

i know you have to

var defer = $q.defer();

defer.promise.then(function () {
    var xdefer = $q.defer();
    xdefer.resolve(1);
    return xdefer.promise.then(function () {
        console.log(unknownvar); // cause typerr
    });
}).catch(function (e) {
    console.error(e);
});

defer.resolve();

so im asking if there is a way to turn this behaviour off (at least when in development mode) or somehow agument "possible unhandled rejection" with full traceback

[gkalpak]

@vertazzar, you can disable the possibly unhandled rejection errors via $qProvider.errorOnUnhandledRejections(false).

[vertazzar]

that's not what i meant, that just silences the error from the console -- what i mean is that i want the old behaviour - that the thrown error breaks the code

as you can see the config just ignores the error

http://codepen.io/anon/pen/yVQdvd

[gkalpak]

I see. This is a different issue, then. This thread is about reporting "Possibly Unhandled Rejections" (PUR). Your issue is about treating thrown errors differently than regular rejections and passing them to the $exceptionHandler.

This is a documented breaking change for 1.6.x (see e13eeab) and the previous behavior cannot be restored (but we can probably improve the PUR reporting to remedy for that).

Note that thrown errors inside promises never "broke the code". They were just passed to the $exceptionHandler (in addition to rejecting the promise), which by default logs the error and moves on.

[vertazzar]

yeah, i just suggested if the PUR could be done with showing more information about the error than just the traceback like i've shown in the screenshot

[vertazzar]

as for the OP's question
here is what should be done to use the default error reporting

https://github.com/vertazzar/angular.js/commit/ee182f3a0db97b9e50b3b5425b424738f605e074

[graingert]

passing toCheck.value clearly breaks backwards compatibility, because $exceptionHandler expects (error, msg). or (error). @vertazzar

[dcherman]

@graingert Yep. I actually have a WIP branch that conforms to the expected API, but there is no way to make it non-breaking, so I'm in no rush to finish it (even considering it's pretty easy/straightforward).

It would not be considered until 1.7 at the earliest. With 1.6 just having been released, that's quite a ways off. Probably should have pushed this more prior to 1.6, but other things took priority.

[graingert]

@dcherman creating a $unhandledRejection service would be totally fine for my usecase

[graingert]

@dcherman and any service that starts with $ is angular specific, so anyone relying on a $unhandledRejection deserves to be broken.

[dcherman]

@graingert That would still result in changes being made to $exceptionHandler (unless it still received unhandled promise exceptions), therefore it's a breaking change.

There was also opposition from a core member to the addition of a somewhat redundant service when we have a global exception handler already:

https://github.com/angular/angular.js/issues/14631#issuecomment-220384291

[graingert]

@lgalfaso please do not mess with the error presented in the rejection. You should try to be as close to the es6-promise spec as possible.

[vertazzar]

@graingert that is true

however - following the pattern of the current implementation $q is using it could be something like $qProvider.logUnhandledRejectionErrors(true or false)

and then you would have:

if (logUnhandledRejectionErrors) {
   console.error(toCheck.value);
}
var errorMessage = 'Possibly unhandled rejection: ' + toDebugString(toCheck.value);
exceptionHandler(errorMessage);

it definitely looks hackish (it would show two errors for one), but i have no better idea showing errors in the console for better debugging

[graingert]

@vertazzar

function $unhandledErrorHandler(error) {
  var errorMessage = 'Possibly unhandled rejection: ' + toDebugString(toCheck.value);
  exceptionHandler(errorMessage);
}

would be fine, users who want the actual error can register their own service.

[thany]

Why can't Angular just spew out the actual error, instead of this useless generic one?

Or at the very least include a traceback.

[gkalpak]

It can (hopefully). And it will (probably).

[gkalpak]

I feel we are overthinking it (e.g. all this discussion about errorHAndler vs exceptionHandler is put of place). I don't think we should be worrying about a breaking change, just becaue we pass an Error object instead of a string. $exceptionHandler can be called with any value (despite what the docs say :grin:). And before 1.6, any thrown errors would be passed to the $exceptionHandler unstringified.

That said, I believe the most important objective should be to enable easy debugging of such errors, which in this case translates to:

1. Take advantage of console.error's stack reporting/presenting capabilities.
2. Make the context clear (i.e. that this is a _possibly unhandled rejection_). Keep in mind that the rejection value can be anything.

#

With that in mind, I think #15527 is the simplest approach. I would prefer to be able to wrap it in a custom PossiblyUnhandledRejectionError (as per https://github.com/angular/angular.js/issues/14631#issuecomment-220167637), but don't think there is a way to do it (without relying on spurious hacks that will break on Safari 😛), while still getting console.error() to print a "clickable" stack trace.

In this case, I favor ergonomics. The string passed as the second argument will give enough context that this is a possibly unhandled rejection.

WDYT, @lgalfaso, @dcherman?

[niemyjski]

I just came here to log this... We lose the stack trace and everything. I was just trying to track down why I'm not getting stack traces in the exceptionless client (https://github.com/exceptionless/Exceptionless.JavaScript)...

  function processChecks() {
    // eslint-disable-next-line no-unmodified-loop-condition
    while (!queueSize && checkQueue.length) {
      var toCheck = checkQueue.shift();
      if (!toCheck.pur) {
        toCheck.pur = true;
        var errorMessage = 'Possibly unhandled rejection: ' + toDebugString(toCheck.value);
        exceptionHandler(errorMessage);
      }
    }

The

$provide.decorator('$exceptionHandler', ['$delegate', function ($delegate) {
                return function (exception, cause) {

exception is being populated as the message and the cause (what should be the message is undefined)

[thany]

We really are overdoing it. How hard can it be to output the exception to the console? Not very, as it turns out.

Here:

function processChecks() {
    // eslint-disable-next-line no-unmodified-loop-condition
    while (!queueSize && checkQueue.length) {
      var toCheck = checkQueue.shift();
      if (!toCheck.pur) {
        toCheck.pur = true;
        console.error(toCheck);
      }
    }
}

Done.

This "Possibly unhandled rejection" message is totally useless. Literally nobody has any good use for it in place of logging the true error. Just throw it out, we don't need it.

[niemyjski]

@thany It's quite useful if you want to log it to a third party service and actually know about the error and fix your code / underlying issue.

[thany]

Yes, and that's why the actual error needs to be logged, not just a generic error message.

[graingert]

@thany PR is here: https://github.com/angular/angular.js/pull/15527

We can't just do console.error because it needs to go to the $exceptionHandler

[circlingthesun]

@niemyjski, yeah. I have the default error handler hooked up to Rollbar. After deploying 1.6 I a serious regression rendered my app unusable for a large portion of users. Angular swallowed all the errors so I only found out about it when users complained.

[thany]

Push it anywhere you like, as long as the error gets console.error-ed. But I was actually referring to logging the error itself instead of a generic message.

[gkalpak]

Thx for the input, @thany and everyone. This is being worked on (in a way that it can support everyone's usecase), so stay tuned. In anyone has comments about the current implementation proposal, please leave your comment in #15527.
_{(Let's try to keep it focused, so that we iterate faster and get to the bottom of it asap.)}