[Error Reporting] Why do unhandled promises disappear? #2585

Closed
miracle2k opened this Issue Sep 7, 2015 · 23 comments

Comments

Projects
None yet
10 participants
@miracle2k

react-native's error reporting is excellent, but I've started to use promises now, and I see them swallow all kinds of errors now.

When developing in Chrome these days, unhandled, rejected promises leave a stracktrace in the console. When I am debugging react-native apps in Chrome, the JavaScript is being executed in Chrome also, says the documentation.

What is the reason I don't receive the stracktrace?

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Sep 7, 2015

Collaborator

React Native uses its own Promises library so that it works on JSC. I suppose we could have it use the existing Promises library if one exists and polyfill in extension methods like done(). Cc @ForbesLindesay @vjeux

Collaborator

ide commented Sep 7, 2015

React Native uses its own Promises library so that it works on JSC. I suppose we could have it use the existing Promises library if one exists and polyfill in extension methods like done(). Cc @ForbesLindesay @vjeux

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Sep 7, 2015

Collaborator

@miracle2k you should call done() at the end of your promise chain which will log if there are unhandled exceptions. You'll want to do this anyway because it will give you the stack trace when running on any other JS engine.

Collaborator

ide commented Sep 7, 2015

@miracle2k you should call done() at the end of your promise chain which will log if there are unhandled exceptions. You'll want to do this anyway because it will give you the stack trace when running on any other JS engine.

@ForbesLindesay

This comment has been minimized.

Show comment
Hide comment
@ForbesLindesay

ForbesLindesay Sep 7, 2015

.done is the recommended way of handling this. We could add something to track and report errors (along the lines of what chrome does) but it would lead to some false positives, and as such it could not crash the entire app in the way that .done can.

.done is the recommended way of handling this. We could add something to track and report errors (along the lines of what chrome does) but it would lead to some false positives, and as such it could not crash the entire app in the way that .done can.

@miracle2k

This comment has been minimized.

Show comment
Hide comment
@miracle2k

miracle2k Sep 8, 2015

I don't follow the spec discussions very closely, but I got the impression that folks decided done() would be dropped in favor of an automatic solution, and I wouldn't have to care about it.

I am using redux and the redux-promise middleware, and it seems to me that really the right place for done() to be called is the middleware itself - which does not do this, presumably with the assumption too that it is not part of the spec.

This just as a note as to why I would be happy to see the JS environment handle unrejected promises

I don't follow the spec discussions very closely, but I got the impression that folks decided done() would be dropped in favor of an automatic solution, and I wouldn't have to care about it.

I am using redux and the redux-promise middleware, and it seems to me that really the right place for done() to be called is the middleware itself - which does not do this, presumably with the assumption too that it is not part of the spec.

This just as a note as to why I would be happy to see the JS environment handle unrejected promises

@ehd

This comment has been minimized.

Show comment
Hide comment
@ehd

ehd Sep 10, 2015

Thanks for chiming in y'all 😄

.done is the recommended way of handling this.

Since async/await have been officially enabled since 0.10.0-rc (release notes), would the equivalent be "try/catch is the recommended way of handling this"?

I've had a weird issue that cost me some hours where rearranging my entry point JS import caused babel-core's core-js Promise implementation to be loaded before then/promise.

ehd commented Sep 10, 2015

Thanks for chiming in y'all 😄

.done is the recommended way of handling this.

Since async/await have been officially enabled since 0.10.0-rc (release notes), would the equivalent be "try/catch is the recommended way of handling this"?

I've had a weird issue that cost me some hours where rearranging my entry point JS import caused babel-core's core-js Promise implementation to be loaded before then/promise.

@ForbesLindesay

This comment has been minimized.

Show comment
Hide comment
@ForbesLindesay

ForbesLindesay Sep 10, 2015

If you await a promise (or return it) you don't need to call .done on it because the error will propagate naturally. You do need to remember that async functions still return promises, so you should call .done on any call to an async function where you're not awaiting the result. try/catch has the same problem as calling .then(null, errorLogger). I.e. What if there's an error in your error logger.

I'm not sure what do do about the babel-core implementation sneaking in.

If you await a promise (or return it) you don't need to call .done on it because the error will propagate naturally. You do need to remember that async functions still return promises, so you should call .done on any call to an async function where you're not awaiting the result. try/catch has the same problem as calling .then(null, errorLogger). I.e. What if there's an error in your error logger.

I'm not sure what do do about the babel-core implementation sneaking in.

@ehd

This comment has been minimized.

Show comment
Hide comment
@ehd

ehd Sep 10, 2015

@ForbesLindesay Thanks for clearing that up!

If you await a promise (or return it) you don't need to call .done on it because the error will propagate naturally.

Makes sense. So what about this?

async function componentWillMount() {
  throw new Error()
}

I guess lifecycle methods shouldn't be made async, because react-native doesn't make any guarantees as to what happens with rejected Promises that are returned. So I should have put the async code into another async function, call that from componentWillMount and attach .done()?

Maybe react(-native) could either call .done() on returned promises, or warn when lifecycle methods are returning promises so developers don't make the same mistake I did?

ehd commented Sep 10, 2015

@ForbesLindesay Thanks for clearing that up!

If you await a promise (or return it) you don't need to call .done on it because the error will propagate naturally.

Makes sense. So what about this?

async function componentWillMount() {
  throw new Error()
}

I guess lifecycle methods shouldn't be made async, because react-native doesn't make any guarantees as to what happens with rejected Promises that are returned. So I should have put the async code into another async function, call that from componentWillMount and attach .done()?

Maybe react(-native) could either call .done() on returned promises, or warn when lifecycle methods are returning promises so developers don't make the same mistake I did?

@ForbesLindesay

This comment has been minimized.

Show comment
Hide comment
@ForbesLindesay

ForbesLindesay Sep 10, 2015

Interesting point. I think react could at a minimum add a warning for that in development mode. It reminds me of C# which let you do async Task<T> myFunction() to indicate that the function returned a Task (a.k.a. Promise) or async void myFunction() to indicate that the function was "fire and forget". i.e. if the return type of the function is void, then errors are automatically thrown into the global scope, but it also prevents you from calling that function and waiting for it to complete.

Some form of type checker (e.g. flow) could be used to detect when you had forgotten to call .done, but we're a long way off being able to do that reliably. It also doesn't solve the problem in this case, because you still want to be able to just mark the function async. One option would be to do something like:

notAwaitable(async function componentWillMount() {
  throw new Error()
})

where notAwaitable would look something like:

function notAwaitable(fn) {
  return function () {
    fn.apply(this, arguments).done();
  }
}

With ES7 decorators, you could turn this into:

@notAwaitable
async function componentWillMount() {
  throw new Error()
}

Interesting point. I think react could at a minimum add a warning for that in development mode. It reminds me of C# which let you do async Task<T> myFunction() to indicate that the function returned a Task (a.k.a. Promise) or async void myFunction() to indicate that the function was "fire and forget". i.e. if the return type of the function is void, then errors are automatically thrown into the global scope, but it also prevents you from calling that function and waiting for it to complete.

Some form of type checker (e.g. flow) could be used to detect when you had forgotten to call .done, but we're a long way off being able to do that reliably. It also doesn't solve the problem in this case, because you still want to be able to just mark the function async. One option would be to do something like:

notAwaitable(async function componentWillMount() {
  throw new Error()
})

where notAwaitable would look something like:

function notAwaitable(fn) {
  return function () {
    fn.apply(this, arguments).done();
  }
}

With ES7 decorators, you could turn this into:

@notAwaitable
async function componentWillMount() {
  throw new Error()
}
@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Sep 10, 2015

Collaborator

I wrote a decorator called autodone that calls done() on async functions that aren't traditionally async:

@autodone
async componentDidMount() {
  await somethingAsync();
}

BTW it's kind of dangerous to use Promises/callbacks/await since the component can get unmounted between await passes. But I just play it fast & loose for now.

Collaborator

ide commented Sep 10, 2015

I wrote a decorator called autodone that calls done() on async functions that aren't traditionally async:

@autodone
async componentDidMount() {
  await somethingAsync();
}

BTW it's kind of dangerous to use Promises/callbacks/await since the component can get unmounted between await passes. But I just play it fast & loose for now.

@ehd

This comment has been minimized.

Show comment
Hide comment
@ehd

ehd Sep 11, 2015

@ForbesLindesay @ide I like the decorator idea. This could also be a react-specific babel transform, since I'd like to have this on all my async methods.

BTW it's kind of dangerous to use Promises/callbacks/await since the component can get unmounted between await passes. But I just play it fast & loose for now.

Huh, that's a good point. It's just so darn convenient with await/async now. Could we:

  • Add documentation for "Be careful when making any component methods async. Instead consider using flux, relay, etc for asynchronous tasks."?
  • In development mode, warn if a component lifecycle function returns a Promise, point to the above documentation?

ehd commented Sep 11, 2015

@ForbesLindesay @ide I like the decorator idea. This could also be a react-specific babel transform, since I'd like to have this on all my async methods.

BTW it's kind of dangerous to use Promises/callbacks/await since the component can get unmounted between await passes. But I just play it fast & loose for now.

Huh, that's a good point. It's just so darn convenient with await/async now. Could we:

  • Add documentation for "Be careful when making any component methods async. Instead consider using flux, relay, etc for asynchronous tasks."?
  • In development mode, warn if a component lifecycle function returns a Promise, point to the above documentation?
@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Sep 11, 2015

Collaborator

@ehd want to raise those points in the react repo since they are about React in general?

Collaborator

ide commented Sep 11, 2015

@ehd want to raise those points in the react repo since they are about React in general?

@ehd

This comment has been minimized.

Show comment
Hide comment
@ehd

ehd Sep 11, 2015

@ide Yes, good point. Will do :)

ehd commented Sep 11, 2015

@ide Yes, good point. Will do :)

@brentvatne brentvatne changed the title from Why do unhandled promises disappear? to [Error Reporting] Why do unhandled promises disappear? Sep 13, 2015

@burgalon

This comment has been minimized.

Show comment
Hide comment
@burgalon

burgalon Dec 13, 2015

Contributor

This is related to #4142
Consider trying to just import 'core-js' and you should get unhandled promise rejection reporting

Contributor

burgalon commented Dec 13, 2015

This is related to #4142
Consider trying to just import 'core-js' and you should get unhandled promise rejection reporting

@mkonicek mkonicek added the Icebox label Apr 10, 2016

@mkonicek mkonicek closed this Apr 10, 2016

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Apr 10, 2016

Contributor

I believe this is fixed now as unhandled promises produce warnings.

Contributor

mkonicek commented Apr 10, 2016

I believe this is fixed now as unhandled promises produce warnings.

@aksonov

This comment has been minimized.

Show comment
Hide comment
@aksonov

aksonov Jun 13, 2016

@mkonicek Warnings are very short and not informative like

Possible Unhandled Promise Rejection (id: 0):
RawText "" must be wrapped in an explicit <Text> component.

Console.log contains stack trace, but it is for bundle:

invariant@http://127.0.0.1:8081/index.ios.bundle?platform=ios&dev=true:2597:16
mountComponent@http://127.0.0.1:8081/index.ios.bundle?platform=ios&dev=true:34163:71
mountComponent@http://127.0.0.1:8081/index.ios.bundle?platform=ios&dev=true:17828:43
_mountChildAtIndex@http://127.0.0.1:8081/index.ios.bundle?platform=ios&dev=true:18758:46
_updateChildren@http://127.0.0.1:8081/index.ios.bundle?platform=ios&dev=true:18674:48
updateChildren@http://127.0.0.1:8081/index.ios.bundle?platform=ios&dev=true:18634:21

Is there any way to see normal stack trace?

aksonov commented Jun 13, 2016

@mkonicek Warnings are very short and not informative like

Possible Unhandled Promise Rejection (id: 0):
RawText "" must be wrapped in an explicit <Text> component.

Console.log contains stack trace, but it is for bundle:

invariant@http://127.0.0.1:8081/index.ios.bundle?platform=ios&dev=true:2597:16
mountComponent@http://127.0.0.1:8081/index.ios.bundle?platform=ios&dev=true:34163:71
mountComponent@http://127.0.0.1:8081/index.ios.bundle?platform=ios&dev=true:17828:43
_mountChildAtIndex@http://127.0.0.1:8081/index.ios.bundle?platform=ios&dev=true:18758:46
_updateChildren@http://127.0.0.1:8081/index.ios.bundle?platform=ios&dev=true:18674:48
updateChildren@http://127.0.0.1:8081/index.ios.bundle?platform=ios&dev=true:18634:21

Is there any way to see normal stack trace?

@ForbesLindesay

This comment has been minimized.

Show comment
Hide comment
@ForbesLindesay

ForbesLindesay Jun 13, 2016

I'm not sure there's a good way to get around that. You could use the .done method to terminate promise chains, which would result in the proper stack trace.

I'm not sure there's a good way to get around that. You could use the .done method to terminate promise chains, which would result in the proper stack trace.

@ThaJay

This comment has been minimized.

Show comment
Hide comment
@ThaJay

ThaJay Jun 30, 2016

Would calling .catch() or passing an onReject to a then act the same as calling .done()? Or is there something special about .done() in this case?

ThaJay commented Jun 30, 2016

Would calling .catch() or passing an onReject to a then act the same as calling .done()? Or is there something special about .done() in this case?

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Jun 30, 2016

Collaborator

.done() also makes it so that you can't continue to chain the promise -- you're done with it.

Collaborator

ide commented Jun 30, 2016

.done() also makes it so that you can't continue to chain the promise -- you're done with it.

@ForbesLindesay

This comment has been minimized.

Show comment
Hide comment
@ForbesLindesay

ForbesLindesay Jun 30, 2016

Calling .catch is a no-op unless you pass it an onRejected handler. In general, if your onRejected handler throws an exception, the error will be silenced. .done ensures that rejections are never silenced. React-Native does also track "unhandled rejections" which is designed to catch when you forget to add .done but it isn't quite as fast or reliable as .done

Calling .catch is a no-op unless you pass it an onRejected handler. In general, if your onRejected handler throws an exception, the error will be silenced. .done ensures that rejections are never silenced. React-Native does also track "unhandled rejections" which is designed to catch when you forget to add .done but it isn't quite as fast or reliable as .done

@ThaJay

This comment has been minimized.

Show comment
Hide comment
@ThaJay

ThaJay Jul 6, 2016

thanks for the info but can't use them for handling the last onRejected I just found out. I .catch() conditionals (either continue with or without fetched data for instance) so can't have it raise everything it gets.
I do get a stacktrace this way but most of it is react stuff, have to go really deep to find my own code.

ThaJay commented Jul 6, 2016

thanks for the info but can't use them for handling the last onRejected I just found out. I .catch() conditionals (either continue with or without fetched data for instance) so can't have it raise everything it gets.
I do get a stacktrace this way but most of it is react stuff, have to go really deep to find my own code.

@petermikitsh

This comment has been minimized.

Show comment
Hide comment
@petermikitsh

petermikitsh Mar 8, 2017

As discussed in #4142 (and thank you to @burgalon):

// override RN's default promise implementation
// which is https://github.com/then/promise
import 'core-js';

window.onunhandledrejection = function(promise, reason) {
  console.log('window.onunhandledrejection', promise, reason);
};

Works 👍

And if you want fancier logging, you can use the global.ErrorUtils.setGlobalHandler and global.ErrorUtils.getGlobalHandler API's to make a 'middleware' for logging purposes, which I have done here: https://github.com/collegepulse/react-native-stacktrace

As discussed in #4142 (and thank you to @burgalon):

// override RN's default promise implementation
// which is https://github.com/then/promise
import 'core-js';

window.onunhandledrejection = function(promise, reason) {
  console.log('window.onunhandledrejection', promise, reason);
};

Works 👍

And if you want fancier logging, you can use the global.ErrorUtils.setGlobalHandler and global.ErrorUtils.getGlobalHandler API's to make a 'middleware' for logging purposes, which I have done here: https://github.com/collegepulse/react-native-stacktrace

slorber referenced this issue Aug 11, 2017

Add support for promise rejection tracking
Summary:
Adds support for tracking unhandled rejections with `console.warn` (= yellow box).

I will create a follow-up with proper error stack formatting.

related: #4971
fixes: #4045, #4142

public

{F59857438}

{F59857439}

Reviewed By: bestander

Differential Revision: D2803126

fb-gh-sync-id: 376b33e42a967675a04338cbff3ec315a77d1037
@slorber

This comment has been minimized.

Show comment
Hide comment
@slorber

slorber Aug 11, 2017

Contributor

Hey, I can confirm it seems to work.

Does importing core-js have other effects that could be armful to my app?

Contributor

slorber commented Aug 11, 2017

Hey, I can confirm it seems to work.

Does importing core-js have other effects that could be armful to my app?

@slorber

This comment has been minimized.

Show comment
Hide comment
@slorber

slorber Aug 11, 2017

Contributor

Note that core-js doesn't have the .finally() method in former versions, you need at least release 2.5.0

Even with adding this version to my package.json it seems that it still fails to call finally, and I also have propTypes warning due to PropTypes.instanceOf(Promise)

Contributor

slorber commented Aug 11, 2017

Note that core-js doesn't have the .finally() method in former versions, you need at least release 2.5.0

Even with adding this version to my package.json it seems that it still fails to call finally, and I also have propTypes warning due to PropTypes.instanceOf(Promise)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment