Consider exposing promise unhandled rejection hook

[benjamingr]

Hey, I've written the following proposal here: https://gist.github.com/benjamingr/0237932cee84712951a2

I'd love your feedback.

[stefanpenner]

it is exposed. RSVP.on('error'

[benjamingr]

@stefanpenner right, and I'm asking you expose it on "process" in addition like the proposal says, Please consider reading it :p

[stefanpenner]

Sounds good. I'll add it to rsvp and es6promise this weekend

[benjamingr]

Awesome - thanks a ton :)

[stefanpenner]

thanks for investing in the promise community :)

[vkurchatkin]

please, don't emit this event on process object. It could end up conflicting with native node.js/io.js implementation or other libs. If the purpose is to share emitter between dupes, the event should be namespaced somehow (e.g. process.on('rsvp:possiblyUnhandledRejection'))

[stefanpenner]

@benjamingr thoughts?

[benjamingr]

@stefanpenner

The whole idea here is to is to fire the same events as native promises and with other libraries so you get a global place to handle every promise error from any library (or native promises) in one place - please see the design document (at the start of the issue) on what problems it solves.

What @vkurchatkin is describing is why we want to do this.

[vkurchatkin]

@benjamingr the idea is good, but there is a reason why developing a standard takes time. There is no consensus on how this should be done.

[benjamingr]

@vkurchatkin I believe there is, and several libraries have already implemented this. This standard is just a minimal first stage as a preparation for more interesting standards and ideas which is why all it takes is exposing two events on process and nothing more.

If you have any interesting feedback I would greatly value it here: https://gist.github.com/benjamingr/0237932cee84712951a2

You can also find me regularly in the Stack Overflow JavaScript chatroom and I tend to frequent #promises too. You can also contact me by email if you'd prefer (benji at tipranks) (dot com)

[stefanpenner]

@benjamingr i believe the best thing todo is to namespace the event for all implementations. This way it wont conflict with any current or future built-ins.

After-which we can bug standards/language folks to consider this proposal.

something like:
process.on('promise:possiblyUnhandledRejection')

[benjamingr]

@stefanpenner there are no standards/language folks. This is just NodeJS/io.js and the libraries. The name "possiblyUnhandledRejection" doesn't make much sense outside of promises anyway so prefixing it doesn't make sense IMO.

On the other hand if the need ever arises you can always give it a different name - "possiblyUnhandledRejection" as a name was chosen just because it made sense - there is no obligation to support it in the future but hopefully it will be used and people will find it useful.

The idea is again - to create a global point all promise libraries and native promises report unhandled rejections to so the user can choose what to do on an unhandled rejection (report it, restart the server, special handling, ignore it or whatever they want).

[benjamingr]

@stefanpenner note the events have been renamed to unhandledRejection and unhandledRejectionHandled as Domenic and Petka suggested in various threads and as when and bluebird implement.

[domenic]

Where did I suggest that? I liked possiblyUnhandledRejection... no big deal though.

[benjamingr]

@domenic To be fair - Petka told me that you suggested these names in a spec and encourages implementing them with this name in bluebird as real events. If you care I can ask him where (cc @petkaantonov) - apologies if I misrepresented your opinion.

[petkaantonov]

@domenic In the original email you suggested unhandledrejection and rejectionhandled and the camelCased versions as per node convention are unhandledRejection and rejectionHandled. I went with unhandledRejection because it is in better symmetry with rejectionHandled.

[domenic]

@petkaantonov I suggested error and rejectionhandled actually, then at the bottom had

We may be better off with a separate unhandledrejection event (or, more accurately and as popular libraries call it, possiblyunhandledrejection)

[benjamingr]

I just want to mark the time and date - @domenic said something nice about bluebird :D (that it's popular)

[benjamingr]

@stefanpenner this is now implemented in io.js in the 1.4 release. Please consider adding it to RSVP.

[stefanpenner]

@stefanpenner this is now implemented in io.js in the 1.4 release. Please consider adding it to RSVP.

yup I believe I am on board. Unfortunately pretty busy now, will likely look into it further post ember-conf (which is next week)

[benjamingr]

Q now has this, you're pretty much the last ones left.

[benjamingr]

Ping @stefanpenner

[benjamingr]

So, it's been like half a year and native node does this. Would be nice to finally have it in.

[petkaantonov]

Pr welcome? :D

[stefanpenner]

Sure.

[benjamingr]

Basically

    if (typeof process === "object" && typeof process.emit === "function") {
        try { 
             process.emit("unhandledRejection", promise._result, promise);
        } catch (e) { /* don't throw for rejection hooks failing in hostile environment */ }
   }

In

rsvp.js/lib/rsvp/promise.js

Line 179 in 7684756

config['trigger']('error', reason, promise._label);

[stefanpenner]

Yup exactly

[benjamingr]

This just cost me like 4 hours stef :(

[vkurchatkin]

@benjamingr why didn't you use RSVP.on('error'? emitting events from objects you don't own is generally a bad idea

[benjamingr]

Because I worked on a code base I did not author that used es6-promise - I just assumed implementations don't swallow exceptions anymore :/

[stefanpenner]

@benjamingr :( sorry about that.

I'm in the midst of some rsvp maintenance, and would like to tackle that as part of this.

Out of curiosity is there a test suite for unhandledRejections ala promise/a+ or some prior art re tests I could use to double check my own interpretation?

[benjamingr]

cc @domenic @petkaantonov w.r.t tests.

I think that both the Node spec and the browser unhandled rejection tracking spec in Chrome are based off the bluebird spec for unhandled rejections but I might be wrong.

https://github.com/petkaantonov/bluebird/blob/master/test/mocha/unhandled_rejections.js

[benjamingr]

@stefanpenner srsly :(

[joepie91]

@benjamingr To be clear - just checking for process.emit does not work in a bundled environment. There is currently an outstanding issue for that on WhenJS as well.

You'll probably want to check for process.browser as well.

EDIT: Also, I've been documenting the various APIs for unhandledRejection events, and there's... quite some variation.

[benjamingr]

@joepie91 first of all thank you for your work, I've been in a really busy time in my life and the moments I get to deal with the web platform and node are pretty scarce - hopefully I'll have time in a month :)

Second of all - I'm surprised the APIs are so consistent, I was expecting a lot less variation. The browser events are named the way they are intentionally in bluebird, and native promises now fire the same events with the same name.

[joepie91]

@benjamingr There's actually quite a lot of variation, but not so much in the event names - for example, some implementations will use a different event format from other implementations, and the behaviour varies between implementations when you define both a modern handler (window.addEventListener) and a legacy handler (window.onunhandledrejection) in a browser environment.

Anyhow, I've just completed a module that I've been working on for a while (and for which I'd initially compiled the Gist I linked): unhandled-rejection - it basically provides an environment-independent and implementation-independent way to deal with unhandledRejection handlers. I hope I can add RSVP.js to the list soon :)

[benjamingr]

@stefanpenner ping