Consider exposing promise unhandled rejection hook

[benjamingr]

Promise unhandled rejection

It is common for promise libraries to emit possibly unhandled rejections (promise chains no .catch listener or then with a second argument is attached to).

Most libraries as well as native promises in V8 support detecting these cases.

It is common for code to have multiple promise libraries, native promises and multiple copies of the same promise library included. Sometimes people are interested in hook on such unhandled rejections in order to add custom logging, terminate the process, suppress the rejection etc. - a survey (in the later linked gist) indicates that this is used in practice by users.

What I'm asking for

I'm asking for native promises to emit events on possibly unhandled rejections so that user level code can handle them to provide better debugging.

Here is the actual proposal

It is currently in the seeking feedback stage.

Effect on user APIs

None, there is no resource penalty, API changes or backwards incompatible changes introduced by this.

Implementation in userland

Since this change requires talking to the native V8 promise implementation which in turn cannot implement this itself since it is unaware of Node, there is no possibility to implement this at a user level either because the hooks are not exposed.

Non native promises (promise libraries) will implement it at a userlevel but since people are writing code that depends on native promises and they are a language feature this hook is required.

[benjamingr]

Anyone?

[benjamingr]

Would you at least consider backporting this from io.js when it lands there?

[benjamingr]

@trevnorris since you're assigned - updating you that the io.js PR was merged here: nodejs/node#758

It would be really awesome if someone could backport it into Node so node users won't have suppressed errors hint hint :)

[trevnorris]

/cc @tjfontaine @cjihrig Thoughts on porting the patch?

[cjihrig]

I'm +1 on it.

[trevnorris]

Ditto.

[benjamingr]

Nice, thanks. @tjfontaine ?

[benjamingr]

@misterdjules how can I push this? Who would be willing to do the work on this? It shouldn't be a lot of work and the benefit for developers is huge IMO.

[misterdjules]

Thank you very much @benjamingr for writing this proposal!

I'm not familiar with promises, so I'm not in the best position to have an educated opinion, let alone to make a decision.

After reading the proposal and the implementation in iojs, I have a few questions:

• It seems that this proposal is more about debugging/tracing than errors that prevent the program from running correctly. Thus, I was wondering if the added events should be emitted by the process object, or if it should be part of some tracing or debugging API.
• It seems that no decision about a default behavior for unhandled rejected promises has been made yet. I think that we should probably wait for this discussion to happen before integrating it into node. I haven't read this thread, but I have the feeling that this could change significant parts of the original proposal and/or implementation.

Since the proposal adds new APIs, the required changes would need to be merged into the master branch. It seems that the current version of V8 in this branch is missing the SetPromiseRejectCallback API and might not notify of potential unhandled rejects. Do you know when V8 started notifying potentially unhandled rejects even when the debugger is not active?

When and If we decide to integrate these changes, my suggestion would then be to, in this order:

• Determine if we really need to upgrade V8 to support these hooks (my understanding is that yes, we'll need to upgrade V8).
• If so, suggest a good candidate for a version of V8 we'd want to upgrade to and submit a PR with an upgrade to this version against the master branch. The latest stable V8 comes to mind, but @trevnorris and others in @joyent/node-coreteam would probably have some interesting input on that.
• Submit another PR against the master branch that ports the support for these hooks from io.js. I noticed that the PR in iojs lacked some documentation, we would need to add that.

How does that sound?

Adding this issue to the 0.13.1 milestone so that we can track it properly.

[benjamingr]

Hi @misterdjules - thanks for the detailed response.

It seems that this proposal is more about debugging/tracing than errors that prevent the program from running correctly. Thus, I was wondering if the added events should be emitted by the process object, or if it should be part of some tracing or debugging API.

These hooks might be seen as a debugging/tracing tool or they might be seen as part of the business logic of the server. I think they should be wherever process.on("uncaughtException" is - that is, they're used for pretty much the same reasons. For example - it is completely reasonable to want to restart your server if one of these errors occurs.

It seems that no decision about a default behavior for unhandled rejected promises has been made yet. I think that we should probably wait for this discussion to happen before integrating it into node. I haven't read this thread, but I have the feeling that this could change significant parts of the original proposal and/or implementation.

I'm afraid that realistically a decision will likely not happen for a while because it's a highly opinionated choice. The good news is that integrating these hooks into node will not require any default behaviour. These hooks are the minimal thing required in order to make promises supported in node - personally I find these hooks immensely useful even without the default action.

Default behavior is something that can be worked out later. I don't think that making opinionated choices on how people write their code is the node way anyway :)

Do you know when V8 started notifying potentially unhandled rejects even when the debugger is not active?

Not sure, but the internal v8 hooks were introduced in
v8/v8@e68e62c from Sep 30th. This means that any v8 version since then should probably do. I'm probably not the right person to pick the relevant v8 version since there are plenty of other considerations when picking a version.

---

What are the next logical steps in order to move this forward? Who would be a good person to choose the relevant v8 version to upgrade to in 0.13.x?

[misterdjules]

@benjamingr Thank you for your response too!

I've read a bit more about promises but I'm still far from getting close to be familiar with them, so like I said before I won't be able to have an educated opinion until then.

I think @trevnorris, @bnoordhuis and @indutny would have a better suggestion for which version of V8 we'd like to upgrade to in master, but I would think that the latest stable release could be a good candidate. According to https://omahaproxy.appspot.com, that would be version 41.0.2272.89.

We could start by upgrading to that version, which would probably have its fair share of challenges (IIRC recent V8 versions require C++11 features, among other things).

As far as I know, most of the team is focused on getting v0.10.37 and v0.12.1 out, so I don't know when we'll get some time to do that. If you're interested, your help would be more than welcome.

Once that is done we'll be in a better position to consider (and test) whether we want to integrate nodejs/node#758.

[benjamingr]

@misterdjules any idea on when a V8 that supports this will be upgraded to? Any updates?

[misterdjules]

@benjamingr We are still focused on stabilizing v0.12, and haven't had the time to work on upgrading V8 to a newer version on master. Any help on that front would be very much appreciated.

[dmail]

I'm eagerly waiting for this to happen :)

[benjamingr]

@dmail that's great but it's not going to happen in the 0.12 lifespan since the v8 being used is too old, I doubt joyent would agree to hack v8 or use non-native promises.

[dmail]

@benjamingr Thus, I hope version 0.13 will be released quickly. Promises are unusable without this, I force the polyfill because node 0.12 promises are missing this feature.

[benjamingr]

I can close this now that io.js and nodejs are merging I think :) Big win for all of us.