Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/ng/q.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,11 @@ function qFactory(nextTick, exceptionHandler, errorOnUnhandledRejections) {
if (!toCheck.pur) {
toCheck.pur = true;
var errorMessage = 'Possibly unhandled rejection: ' + toDebugString(toCheck.value);
Copy link
Member

@gkalpak gkalpak Dec 20, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

exceptionHandler(errorMessage);
if (toCheck.value instanceof Error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

exceptionHandler(toCheck.value, errorMessage);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@graingert graingert Dec 21, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, OK. I mostly wondered because this:
screen shot 2016-12-21 at 15 48 43
looks a little weird.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@graingert graingert Dec 21, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about just doing

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

with no ifs or errorMessage generation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@mgol mgol Dec 21, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} else {
exceptionHandler(errorMessage);
}
}
}
}
Expand Down
129 changes: 90 additions & 39 deletions test/ng/qSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,16 +181,14 @@ describe('q', function() {
};


function exceptionHandler(reason) {
exceptionHandlerCalls.push(reason);
}


function exceptionHandlerStr() {
return exceptionHandlerCalls.join('; ');
function exceptionHandler(exception, reason) {
if (typeof reason === 'undefined') {
exceptionHandlerCalls.push({ reason: exception });
} else {
exceptionHandlerCalls.push({ reason: reason, exception: exception });
}
}


beforeEach(function() {
q = qFactory(mockNextTick.nextTick, exceptionHandler, true);
q_no_error = qFactory(mockNextTick.nextTick, exceptionHandler, false);
Expand Down Expand Up @@ -2167,45 +2165,98 @@ describe('q', function() {


describe('when exceptionHandler is called', function() {
it('should log an unhandled rejected promise', function() {
var defer = q.defer();
defer.reject('foo');
mockNextTick.flush();
expect(exceptionHandlerStr()).toBe('Possibly unhandled rejection: foo');
});
function CustomError() { }
CustomError.prototype = Object.create(Error.prototype);

var errorEg = new Error('Fail');
var errorStr = toDebugString(errorEg);

it('should not log an unhandled rejected promise if disabled', function() {
var defer = q_no_error.defer();
defer.reject('foo');
expect(exceptionHandlerStr()).toBe('');
});
var customError = new CustomError('Custom');
var customErrorStr = toDebugString(customError);

var nonErrorObj = { isATest: 'this is' };
var nonErrorObjStr = toDebugString(nonErrorObj);

it('should log a handled rejected promise on a promise without rejection callbacks', function() {
var defer = q.defer();
defer.promise.then(noop);
defer.reject('foo');
mockNextTick.flush();
expect(exceptionHandlerStr()).toBe('Possibly unhandled rejection: foo');
});
var fixtures = [
{
type: 'Error object',
value: errorEg,
expected: {
exception: errorEg,
reason: 'Possibly unhandled rejection: ' + errorStr
}
},
{
type: 'custom Error object',
value: customError,
expected: {
exception: customError,
reason: 'Possibly unhandled rejection: ' + customErrorStr
}
},
{
type: 'non-Error object',
value: nonErrorObj,
expected: {
reason: 'Possibly unhandled rejection: ' + nonErrorObjStr
}
},
{
type: 'string primitive',
value: 'foo',
expected: {
reason: 'Possibly unhandled rejection: foo'
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  • One with a non-Error object.
  • One with an object that "extends" Error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah makes sense

];
forEach(fixtures, function(fixture) {
var type = fixture.type;
var value = fixture.value;
var expected = fixture.expected;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra points for putting some space before it blocks 😃

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or describe blocks...


describe('with ' + type, function() {

it('should not log a handled rejected promise', function() {
var defer = q.defer();
defer.promise.catch(noop);
defer.reject('foo');
mockNextTick.flush();
expect(exceptionHandlerStr()).toBe('');
});
it('should log an unhandled rejected promise', function() {
var defer = q.defer();
defer.reject(value);
mockNextTick.flush();
expect(exceptionHandlerCalls).toEqual([expected]);
});


it('should not log a handled rejected promise that is handled in a future tick', function() {
var defer = q.defer();
defer.promise.catch(noop);
defer.resolve(q.reject('foo'));
mockNextTick.flush();
expect(exceptionHandlerStr()).toBe('');
it('should not log an unhandled rejected promise if disabled', function() {
var defer = q_no_error.defer();
defer.reject(value);
expect(exceptionHandlerCalls).toEqual([]);
});


it('should log a handled rejected promise on a promise without rejection callbacks', function() {
var defer = q.defer();
defer.promise.then(noop);
defer.reject(value);
mockNextTick.flush();
expect(exceptionHandlerCalls).toEqual([expected]);
});


it('should not log a handled rejected promise', function() {
var defer = q.defer();
defer.promise.catch(noop);
defer.reject(value);
mockNextTick.flush();
expect(exceptionHandlerCalls).toEqual([]);
});


it('should not log a handled rejected promise that is handled in a future tick', function() {
var defer = q.defer();
defer.promise.catch(noop);
defer.resolve(q.reject(value));
mockNextTick.flush();
expect(exceptionHandlerCalls).toEqual([]);
});
});
});
});
});