JS exceptions are hidden from developer when using Promises (async functions)

[mkonicek]

Take the following example, which (correctly) produces a redbox:

IntentAndroid.canOpenURL(url, (canOpen) => {
  // Some code that can throw ...
  throw new Error('This should redbox');
});

The Java signature is:

@ReactMethod
public void canOpenURL(String url, Callback callback)

Now change this to an async function:

@ReactMethod
public void canOpenURL(String url, Promise promise)

IntentAndroid.canOpenURL(url).then((canOpen) => {
  // Some code that can throw ...
  throw new Error('This should redbox');
});

No redbox is shown and nothing is logged which is in my opinion very poor developer experience. We've all been there - you're trying to debug a program which doesn't work but since there are no error messages and you have no idea what's wrong.

The way to "fix" this is to call done():

IntentAndroid.canOpenURL(url).then((canOpen) => {
  // Some code that can throw ...
  throw new Error('This should redbox');
}).done();  // Now throws and produces a redbox

But this is super easy to forget :(

For now, I'll go with a callback in the IntentAndroid. Feel like we need to find a good solution to this problem first before we migrate all APIs to async functions.

[davidaurelio]

• unfortunately, done() is not part of the es2015 standard
• maybe we should extend our internal promise implementation so that it produces a red box if no rejection handler is registered.

[mkonicek]

maybe we should extend our internal promise implementation so that it produces a red box if no rejection handler is registered

I like this idea a lot.

@javache, @martinbigio Do you agree?

[ide]

I like the idea too. When I thought about this before (for ObjC PromiseKit) I concluded that the promise library should check if it has an unhandled error when the promise has been deallocated (this is the only time we know that no code can add an error handler to it). So I don't think it is possible to come up with a pure JS solution that is accurate. The good news is that in RN we can likely use the native JSC API to learn when promise objects have been deallocated, but it may require some ingenuity.

[satya164]

That would be great!

[davidaurelio]

So I don't think it is possible to come up with a pure JS solution that is accurate.

Hm, I think we can do it, but maybe I’m too naive:

1. Given promise A that is rejected, we have to redbox if

   1. there is no rejection handler registered (easy to detect)

   2. any registered rejection handler throws, or returns a rejected promise, and there is no rejection handler further down the stack.

      Since .then() and .catch() return a promise A′ themselves, this is covered by case 1.i – at least one promise is eventually going to be rejected without having a registered rejection handler. These “null” promises cannot be deallocated, as the parent promise needs to keep a reference.

2. Given promise A that is resolved, we have to redbox if
   1. any resolve handler throws an error or returns a rejected promise and the corresponding child promise A′ doesn’t have a rejection handler. This is also covered by 1.i. As explained, A must keep a reference to A′, i.e. it is not garbage collected.

[ide]

A promise can be rejected and then handled later, which is why we want to learn when the programmer is done with the promise. That is, I believe (1.i) is nuanced. It's going to be really noisy if the promise library warns about potential errors that are properly handled later. False positives in such a core language feature are frustrating.

For example:

function maybeFetchDataSecurelyAsync(url) {
  if (url.startsWith('http:')) {
    return Promise.reject(new Error('only https is supported'));
  }
  return fetch(....);
}

If you call this with an http URL, the constructed promise has already been rejected but we obviously don't want to warn because someone could add a catch handler to the rejected promise later -- maybe right away, maybe a few ticks from now.

So done() is the crude way to signal that the promise isn't going to get a catch handler if it doesn't have one already, but another way would be to learn when there are no more references to the promise in the execution environment -- an approximation of this is to learn when the promise has been GC'd.

[davidaurelio]

Promise.reject() does not get rejected before the next tick, does it?
Can you think of a use case where a promise would be produced and not consumed right away?

[ide]

Can you think of a use case where a promise would be produced and not consumed right away?

Say you wanted to prefetch some data for the next screen. You might write code like this:

componentDidMount() {
  this.prefetchedDataPromise = fetch(...);
}

onNavigateToNextScreen() {
  this.prefetchedDataPromise.then(data => ..., error => {
    // fetch again or maybe just show a network error
  });
}

The cheap workaround is to add a no-op error handler:

this.prefetchedDataPromise = fetch(...);
this.prefetchedDataPromise.catch(error => {});

The downsides are that now we're back a square one and won't get an error message if the programmer forgets to add a real error handler to the promise, and this doesn't address third-party code that doesn't handle rejected promises right away. If it were possible to tell the devtools to ignore unhandled promises in specific swaths of code, the latter point wouldn't be a problem, though.

It might end up being totally fine in practice, especially if this becomes standard behavior, de facto or officially.

[davidaurelio]

I might play around with Chrome a little bit to see how it behaves (it logs unhandled rejections).

If we can create a superior solution on the native side, let’s do it. Solving it in JS just wouldn’t require us to write and maintain two solutions.

[bestander]

Can we solve the problem of missing .done() statement with linters?

[bestander]

Maybe I am dreaming here but it could be: "if promise is not returned by method than it must be .done()-d"

[bestander]

Also Firefox outputs warning if an exception is silent in a Promise, this seems to be a native feature.
Chrome team promised to do the same AFAIK

[davidaurelio]

Chrome has it, I just checked it. Promise.reject() prints an error on the console immediately.

[bestander]

Yeah, this code prints error to log:

new Promise(function(done){throw new Error("Bazinga")}).then(()=>{console.log("done")})

This one not because promise has error handling:

new Promise(function(done){throw new Error("Bazinga")}).then(()=>{console.log("done")}, ()=>{})

[mkonicek]

Linters won't work because not everyone uses a linter.

[davidaurelio]

I thought about the deallocation idea, and it has the same problem as warning on unhandled rejections: There might be unhandled rejections that are legitimately deallocated.

[ide]

@davidaurelio The fix for unhandled rejections that have been deallocated would be to add done() or an error handler.

Chrome has it, I just checked it. Promise.reject() prints an error on the console immediately.

OK. This seems like a good place to start. It's nice that it's deterministic unlike deallocation.

[philikon]

maybe we should extend our internal promise implementation so that it produces a red box if no rejection handler is registered.

Absolutely, that's the desired behaviour IMHO. But I thought Babel's Promise polyfill already did that? It does at least for me, but then again, I don't use the React Native packager -- I use webpack + a babel loader via https://github.com/mjohnston/react-native-webpack-server/.

[ide]

Someone had a good idea in the AMA today to have uncaught errors get fed into a global JS error handler. Then you could hook up the redbox to the global handler, or a custom handler as well.

[philikon]

So, I get to see a redbox due to this console.error call when my code doesn't catch a promise rejection: https://github.com/zloirock/core-js/blob/e00042d166ebbc3e4ad08dc483ba40de83af2618/library/modules/es6.promise.js#L134

Is this not triggering for anybody? If not, I wonder why. In any case, I support doing whatever we can to preserve the original stack (instead of having core-js/es6.promise.js on the stack).

[davidaurelio]

I’m on it, but will turn on yellow boxes first

[vjeux]

cc @ForbesLindesay

[ForbesLindesay]

I've written the changes needed to track unhandled rejections in promise. If a few people want to review then/promise#116 I'll merge and release a new version. It should then be fairly simple for someone to buidl the yellowbox into react-native. It shouldn't be redbox because of the high probability of false-positives. Ideally the yellow box should be updated with a method to turn it green and indicate that the report was a false-positive.

[bestander]

As for API: promise vs callbacks.
During the transition period we could support both.

https://gist.githubusercontent.com/gergelyke/5511a7fa7c9d94a42154/raw/b8989eeb5b362f33b8d5585036be7dc0e4565f31/index.js

[ForbesLindesay]

You could, but promises are the better option. If you want to do that, you can just use promise.nodeify(callback) (if you include node-extensions in your build)

[bestander]

👍

[satya164]

@ForbesLindesay We can't really do that, because the RN APIs are too different ATM - #4971

But I'm all for adding promise based APIs and eventually fading out the non-promise APIs.

[lukewis]

@davidaurelio , thanks for adding this support (a long time ago)! Are there any "hooks" that will allow custom behavior when this occurs? I understand that there may be a probability for false positives, but in my experience with my own apps so far, unhandled promise rejections are typically a development error. Having an event or other "hook" would allow developers to choose how to handle this scenario (yellow box, red box, etc).

[davidaurelio]

you should be able to paste this code into your app, and throw an error:

react-native/Libraries/Promise.js

Lines 17 to 49 in c6b96c0

	/* $FlowFixMe(>=0.54.0 site=react_native_oss) This comment suppresses an
	* error found when Flow v0.54 was deployed. To see the error delete this
	* comment and run Flow. */
	require('promise/setimmediate/rejection-tracking').enable({
	allRejections: true,
	onUnhandled: (id, error = {}) => {
	let message: string;
	let stack: ?string;
	const stringValue = Object.prototype.toString.call(error);
	if (stringValue === '[object Error]') {
	message = Error.prototype.toString.call(error);
	stack = error.stack;
	} else {
	/* $FlowFixMe(>=0.54.0 site=react_native_oss) This comment suppresses
	* an error found when Flow v0.54 was deployed. To see the error delete
	* this comment and run Flow. */
	message = require('pretty-format')(error);
	}
	const warning =
	`Possible Unhandled Promise Rejection (id: ${id}):\n` +
	`${message}\n` +
	(stack == null ? '' : stack);
	console.warn(warning);
	},
	onHandled: (id) => {
	const warning =
	`Promise Rejection Handled (id: ${id})\n` +
	'This means you can ignore any previous messages of the form ' +
	`"Possible Unhandled Promise Rejection (id: ${id}):"`;
	console.warn(warning);
	},

In case this does nothing, use setImmediate to throw outside of the promise rejection handler