Add traceback to unhandled promise rejections, Fixes: #14631 #15527

graingert · 2016-12-19T23:04:57Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Feature

What is the current behavior? (You can also link to an open issue here)
Console is filled with meaningless: Possibly unhandled rejection: {} errors

What is the new behavior (if this is a feature change)?
Console is filled with juicy traceback info.

Does this PR introduce a breaking change?
I don't think so

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

Other information:

graingert · 2016-12-20T12:01:32Z

@gkalpak uhh, are the tests for the unhandled rejection even run? I added these to intentionally fail...

gkalpak · 2016-12-20T12:21:55Z

Not sure what you mean. The tests should run (and I am quite sure they do).
(The tests need some fixes, because they are not currently testing what they should be testing, but that is unrelated.)

graingert · 2016-12-20T12:25:07Z

@gkalpak https://github.com/angular/angular.js/pull/15527/files#diff-0cfb390955e411a4a5437fa337d52481R2181 should fail on the fixture.type === 'exception' because it doesn't stringify to 'foo'.

graingert · 2016-12-20T12:26:07Z

@gkalpak are you in IRC?

gkalpak · 2016-12-20T12:28:53Z

@graingert, if you want it to fail, then you better not reject the promise with foo 😛
_{(I am rarely (read "never") on IRC these days, but you can find me on gitter.)}

graingert · 2016-12-20T12:30:54Z

@gkalpak oh I am a silly... Fixing...

gkalpak · 2016-12-20T12:40:28Z

BTW, it would made our life a tiny bit easier if you followed our commit message guidelines 😉

graingert · 2016-12-20T12:42:14Z

@gkalpak will do once I've got the tests working. Also your URL is out of date: you want https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format

gkalpak · 2016-12-20T12:56:37Z

src/ng/q.js

@@ -381,7 +381,11 @@ function qFactory(nextTick, exceptionHandler, errorOnUnhandledRejections) {
      if (!toCheck.pur) {
        toCheck.pur = true;
        var errorMessage = 'Possibly unhandled rejection: ' + toDebugString(toCheck.value);


Here is another idea (either for toDebugString() or just here):
If toCheck.value has a toString() method that is not the default Object.prototype.toString(), then use that for serializing it. Something similar to stringify.

gkalpak · 2016-12-20T12:58:48Z

Thx! (My URL was fine btw - just intentionally pointing to the supersection of yours 😛)

graingert · 2016-12-20T13:03:20Z

@gkalpak ah it didn't go to a section for me at all. (it works now)

graingert · 2016-12-20T15:51:25Z

@gkalpak and we're green!

gkalpak · 2016-12-20T20:49:02Z

@graingert, awesome! Now on to that commit message 😛

gkalpak

I left some comments, but the general idea LGTM.

gkalpak · 2016-12-20T20:55:29Z

test/ng/qSpec.js

+    var exceptionStr = toDebugString(exceptionEg);
+    var fixtures = [
+      {
+        type: 'exception',


Change "exception" to "Error object".

gkalpak · 2016-12-20T20:56:20Z

test/ng/qSpec.js

+      var type = fixture.type;
+      var value = fixture.value;
+      var expected = fixture.expected;
+      it('should log an unhandled' + type + ' rejected promise', function() {


Instead of repeating type in all it blocks, create a describe block for each fixture and put type in its description.

Ah makes sense

gkalpak · 2016-12-20T20:57:06Z

test/ng/qSpec.js

+    forEach(fixtures, function(fixture) {
+      var type = fixture.type;
+      var value = fixture.value;
+      var expected = fixture.expected;


Extra points for putting some space before it blocks 😃

Or describe blocks...

gkalpak · 2016-12-20T21:00:38Z

test/ng/qSpec.js

+        expected: {
+          reason: 'Possibly unhandled rejection: foo'
+        }
+      }


A couple more fixtures would be in order imo. E.g.:

One with a non-Error object.

One with an object that "extends" Error.

Yeah makes sense

graingert · 2016-12-21T12:21:47Z

@gkalpak done

gkalpak

A couple of nitpicks, but otherwise LGTM.

gkalpak · 2016-12-21T12:27:56Z

test/ng/qSpec.js

+        }
+      },
+      {
+        type: 'plain value',


Now everyone is capitalized except for plain value. Just because it is nothing more than a plain value, doesn't mean you have to treat it like a 2nd-class citizen 😛

I went with all lower case, and more accurate names

gkalpak · 2016-12-21T12:28:50Z

test/ng/qSpec.js

+    forEach(fixtures, function(fixture) {
+      var type = fixture.type;
+      var value = fixture.value;
+      var expected = fixture.expected;


Or describe blocks...

gkalpak · 2016-12-21T12:29:42Z

test/ng/qSpec.js

+      var type = fixture.type;
+      var value = fixture.value;
+      var expected = fixture.expected;
+      describe(type, function() {


Nit: Using something like 'with ' + type will make the description read better (in case of an error for example).

graingert · 2016-12-21T12:40:56Z

@gkalpak done

gkalpak

LGTM, but let's wait for some feedback from others as well.
_{(In the mean time, feel free to fix up that commit message 😁)}

graingert · 2016-12-21T13:11:42Z

@gkalpak I did fix the commit message... What's wrong with it?

gkalpak · 2016-12-21T13:14:54Z

Actual:

fix($q): Add traceback to unhandled promise rejections, Fixes: #14631

Expected something like:

fix($q): add traceback to unhandled promise rejections

Fixes #14631

(The change may seem trivial, but the format is important, because we auto-generate the changelog from these commit messages. So small deviations can make a lot of difference in the resulting changelog.)

mgol · 2016-12-21T13:34:23Z

@graingert Thanks for the PR, this is awesome. 👏

I don't have any remarks apart from the incorrect commit message format.

Fixes: angular#14631

graingert · 2016-12-21T14:15:26Z

@gkalpak commit message fixed

mgol · 2016-12-21T14:30:02Z

src/ng/q.js

@@ -381,7 +381,11 @@ function qFactory(nextTick, exceptionHandler, errorOnUnhandledRejections) {
      if (!toCheck.pur) {
        toCheck.pur = true;
        var errorMessage = 'Possibly unhandled rejection: ' + toDebugString(toCheck.value);
-        exceptionHandler(errorMessage);
+        if (toCheck.value instanceof Error) {
+          exceptionHandler(toCheck.value, errorMessage);


Is this the correct order? It looks weird when the error message is after the stack trace.

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

$exceptionHandler(exception, [cause]);

Param Type Details

exception Error Exception associated with the error.

cause (optional) string Optional information...

Hmm, OK. I mostly wondered because this:

looks a little weird.

There is nothing in the $exceptionHandler code that would make the order important; that's all of it:

function $ExceptionHandlerProvider() { this.$get = ['$log', function($log) { return function(exception, cause) { $log.error.apply($log, arguments); }; }]; }

I wonder what's the reason for the current documentation. @gkalpak, @petebacondarwin does any one of you know?

what about just doing

exceptionHandler(toCheck.value, 'Possibly unhandled rejection')

with no ifs or errorMessage generation.

That sounds sensible but I'd still swap the order of operations. Note that currently we are trying to show the 'Possibly unhandled rejection' message first and the error afterwards; we just put it all in one string argument so the stack trace gets lost. So I'd like:

exceptionHandler('Possibly unhandled rejection', toCheck.value);

but that would require updating the docs to no longer require the cause to be the second parameter (no code changes required as it already works as I mentioned before).

I'd just want to know if there's any reason for the current signature so let's see what others say. :)

@mgol my custom $exceptionHandlers are configured to pass the first argument as the Error value, as per the documentation.

I presume others follow the same documentation also.

@graingert Is the issue that some automated tools might only pick up the first argument and discard others, losing the error context? Because otherwise, in Angular source, the order influences only one thing - the order in which messages/errors are logged. And it makes more sense to log the summary first and the error second. In other words, the following:

exceptionHandler('Possibly unhandled rejection', toCheck.value);

is analogous to:

exceptionHandler('Possibly unhandled rejection: ' + toDebugString(toCheck.value));

just with more details printed to the console as the error stack is not lost.

@mgol swapping the types of the parameters of $exceptionHandler is another issue for another PR, and would be a breaking change.

OK, fair enough. In that case, maybe it's better to leave it as it is for now; your proposal to just write:

exceptionHandler(toCheck.value, 'Possibly unhandled rejection');

sounds nice but we pass other data types through toDebugString and we'd lose it in that case. If we want to tweak that, it would be better to tackle that separately after this PR lands.

dcherman · 2016-12-21T18:33:22Z

src/ng/q.js

@@ -381,7 +381,11 @@ function qFactory(nextTick, exceptionHandler, errorOnUnhandledRejections) {
      if (!toCheck.pur) {
        toCheck.pur = true;
        var errorMessage = 'Possibly unhandled rejection: ' + toDebugString(toCheck.value);
-        exceptionHandler(errorMessage);
+        if (toCheck.value instanceof Error) {


Though I don't think it necessarily a concern here, do we care that this will not identify errors that originate from an iframe? Don't know how common of a use case that is.

I doubt it's common but if it turns out to be an issue we can revisit.

Ideally we would convert to a string (via toDebugString) only certain types of values and log others unchanged. The problem is that then we'd have to pass Possibly unhandled rejection and toCheck.value as separate arguments to $exceptionHandler and if we do it in a documented way (error first, (optional) reason second) the message would get swapped from e.g.:

'Possibly unhandled rejection: MY_THING'

to:

'MY_THING' 'Possibly unhandled rejection'

which is less pleasant.

We can discuss it but let's do it in a separate issue if desired.

Fixes #14631 Closes #15527

mgol · 2016-12-21T19:14:05Z

Landed, thanks!

mgol · 2016-12-21T19:15:04Z

@graingert One nit (I fixed it myself): there should be no colon after Fixes in the commit description.

Fixes angular#14631 Closes angular#15527

googlebot added cla: yes labels Dec 19, 2016

graingert mentioned this pull request Dec 19, 2016

Add traceback to unhandled promise rejections #15522

Closed

3 tasks

graingert force-pushed the patch-3 branch from fff5148 to 59fade3 Compare December 20, 2016 11:07

graingert force-pushed the patch-3 branch from 59fade3 to b7ceb4e Compare December 20, 2016 12:33

gkalpak mentioned this pull request Dec 20, 2016

$q: Unhandled rejections should not be stringified #14631

Closed

gkalpak added component: $q frequency: high severity: inconvenient type: feature labels Dec 20, 2016

gkalpak added this to the 1.6.1 milestone Dec 20, 2016

gkalpak reviewed Dec 20, 2016

View reviewed changes

graingert force-pushed the patch-3 branch 2 times, most recently from 6e2531b to 013779c Compare December 20, 2016 12:58

graingert force-pushed the patch-3 branch 5 times, most recently from c014f10 to 1654b47 Compare December 20, 2016 15:28

gkalpak suggested changes Dec 20, 2016

View reviewed changes

gkalpak added needs: test needs: work and removed needs: test labels Dec 20, 2016

graingert force-pushed the patch-3 branch 3 times, most recently from c75796b to 1dd6336 Compare December 21, 2016 11:05

gkalpak approved these changes Dec 21, 2016

View reviewed changes

graingert force-pushed the patch-3 branch from 1dd6336 to 7d681d3 Compare December 21, 2016 12:39

gkalpak approved these changes Dec 21, 2016

View reviewed changes

fix($q): Add traceback to unhandled promise rejections

d73fdde

Fixes: angular#14631

graingert force-pushed the patch-3 branch from 7d681d3 to d73fdde Compare December 21, 2016 14:15

mgol suggested changes Dec 21, 2016

View reviewed changes

mgol approved these changes Dec 21, 2016

View reviewed changes

dcherman reviewed Dec 21, 2016

View reviewed changes

mgol closed this in 316f60f Dec 21, 2016

mgol pushed a commit that referenced this pull request Dec 21, 2016

fix($q): Add traceback to unhandled promise rejections

174cb4a

Fixes #14631 Closes #15527

gkalpak mentioned this pull request Jan 11, 2017

Possible unhandled rejection #15599

Closed

ellimist pushed a commit to ellimist/angular.js that referenced this pull request Mar 15, 2017

fix($q): Add traceback to unhandled promise rejections

2260217

Fixes angular#14631 Closes angular#15527

graingert mentioned this pull request Mar 15, 2017

log usable error on unhandled non error rejections Fixes #15816 #15818

Open

3 tasks

Param	Type	Details
exception	Error	Exception associated with the error.
cause (optional)	string	Optional information...

Add traceback to unhandled promise rejections, Fixes: #14631 #15527

Add traceback to unhandled promise rejections, Fixes: #14631 #15527

Conversation

graingert commented Dec 19, 2016 • edited Loading

graingert commented Dec 20, 2016

gkalpak commented Dec 20, 2016 • edited Loading

graingert commented Dec 20, 2016

graingert commented Dec 20, 2016

gkalpak commented Dec 20, 2016 • edited Loading

graingert commented Dec 20, 2016

gkalpak commented Dec 20, 2016

graingert commented Dec 20, 2016

gkalpak Dec 20, 2016 • edited Loading

Choose a reason for hiding this comment

gkalpak commented Dec 20, 2016

graingert commented Dec 20, 2016 • edited Loading

graingert commented Dec 20, 2016

gkalpak commented Dec 20, 2016

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak Dec 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graingert commented Dec 21, 2016

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graingert Dec 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graingert commented Dec 21, 2016

gkalpak left a comment

Choose a reason for hiding this comment

graingert commented Dec 21, 2016

gkalpak commented Dec 21, 2016 • edited Loading

mgol commented Dec 21, 2016

graingert commented Dec 21, 2016

Choose a reason for hiding this comment

graingert Dec 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graingert Dec 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgol Dec 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgol commented Dec 21, 2016

mgol commented Dec 21, 2016

graingert commented Dec 19, 2016 •

edited

Loading

gkalpak commented Dec 20, 2016 •

edited

Loading

gkalpak commented Dec 20, 2016 •

edited

Loading

gkalpak Dec 20, 2016 •

edited

Loading

graingert commented Dec 20, 2016 •

edited

Loading

gkalpak Dec 20, 2016 •

edited

Loading

graingert Dec 21, 2016 •

edited

Loading

gkalpak commented Dec 21, 2016 •

edited

Loading

graingert Dec 21, 2016 •

edited

Loading

graingert Dec 21, 2016 •

edited

Loading

mgol Dec 21, 2016 •

edited

Loading