New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Difficult to get onPossiblyUnhandledRejection to work all of the time in Node.js due to submodules #357

Closed
sheltond opened this Issue Oct 17, 2014 · 22 comments

Comments

Projects
None yet
4 participants
@sheltond

sheltond commented Oct 17, 2014

onPossiblyUnhandledRejection sets some global state in the module instance to record the function to call in the event of an unhandled rejection.

However, since npm's default is to install a separate instance of modules in the "node_modules" directory of modules that the app depends on, it's difficult for the app to set up error handling for instances of Bluebird which are referenced from another module.

I came across this with Sequelize - I set up an error handler at the top level in my app, and it usually works fine. However, sometimes the default error handler is used instead. It turned out that this is due to the separate instance of Bluebird that Sequelize is using.

(In fact, certain versions of Sequelize use the sequelize-bluebird module instead, which has some differences, but the same problem applies)

The problem gets worse as your app uses more modules that make use of Bluebird.

I don't really have a good suggested solution, but it seems like this is the sort of thing that an app wants to configure for the entire system, rather than a specific instance of the module.

The only ways that I can really think of to do this is for Bluebird to use some global object in Node.js to co-ordinate the error handling mechanism across the different Bluebird instances. The most likely candidate is the 'process' object, I guess.

For example, this could be done by recording the error handling function in a property of the process object, which would be shared by all Bluebird instances, or by emitting an event on the process object instead of using the onPossiblyUnhandledRejection mechanism, which the app could listen for.

I realise that npm installs modules this way to isolate modules from behavioural changes in their dependencies when other modules in the system have difference versions of the module, but in this case I think it might be worth putting in a 'global' mechanism to set the error handler.

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Oct 25, 2014

Owner

You should be able to work around it by promisifying the callback api of a module through a bluebird instance you control

Owner

petkaantonov commented Oct 25, 2014

You should be able to work around it by promisifying the callback api of a module through a bluebird instance you control

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Jan 7, 2015

Owner

If a library is using their own private instance, they should expose the hook.

e.g. with sequelize

Sequlize.Promise.onPossiblyUnhandledRejection(function() {
    // Your application specific handler        
});
Owner

petkaantonov commented Jan 7, 2015

If a library is using their own private instance, they should expose the hook.

e.g. with sequelize

Sequlize.Promise.onPossiblyUnhandledRejection(function() {
    // Your application specific handler        
});
@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Jan 7, 2015

Collaborator

@petkaantonov I think what he's asking for is a way to hook up to every bluebird instance currently running (yours, Sequelize's etc).

That's like adding a:

process.on("bluebirdLoaded", yourHandler); 

And having every instance of bluebird do:

process.emit("bluebirdLoaded", Promise)

When it loads.

Collaborator

benjamingr commented Jan 7, 2015

@petkaantonov I think what he's asking for is a way to hook up to every bluebird instance currently running (yours, Sequelize's etc).

That's like adding a:

process.on("bluebirdLoaded", yourHandler); 

And having every instance of bluebird do:

process.emit("bluebirdLoaded", Promise)

When it loads.

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Jan 7, 2015

Owner

But you still need to know about each library specifically whether you can just override their onUnhandledRejection hook, the library might depend on it.

Owner

petkaantonov commented Jan 7, 2015

But you still need to know about each library specifically whether you can just override their onUnhandledRejection hook, the library might depend on it.

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Jan 7, 2015

Collaborator

@petkaantonov that's true - it might be wise to expose the old handler in the function so:

Promise.onPossiblyUnhandledRejection(function(e, promise, oldHandler) {
    //...
});

Which would let you in turn do:

Promise.onPossiblyUnhandledRejection(function(e, promise, oldHandler) {
    // your own before hooks.
    try{
      var res = oldHandler.apply(this, arguments);
    } catch(e){
         // hook on the library throwing, most likely a rethrow
    } finally {
        // your after hooks here
    }
    return res; // or override, return value isn't used here anyway
});

With a similar parameter for handled possibly unhandled rejections. This might also be useful for adding to the default pretty print behavior.

I don't mind writing a quick PR this weekend if you think this is a good idea. Will help me get acquainted with the code changes anyway.

Collaborator

benjamingr commented Jan 7, 2015

@petkaantonov that's true - it might be wise to expose the old handler in the function so:

Promise.onPossiblyUnhandledRejection(function(e, promise, oldHandler) {
    //...
});

Which would let you in turn do:

Promise.onPossiblyUnhandledRejection(function(e, promise, oldHandler) {
    // your own before hooks.
    try{
      var res = oldHandler.apply(this, arguments);
    } catch(e){
         // hook on the library throwing, most likely a rethrow
    } finally {
        // your after hooks here
    }
    return res; // or override, return value isn't used here anyway
});

With a similar parameter for handled possibly unhandled rejections. This might also be useful for adding to the default pretty print behavior.

I don't mind writing a quick PR this weekend if you think this is a good idea. Will help me get acquainted with the code changes anyway.

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Jan 7, 2015

Owner

So when is a good time to fire the onloadedevent so that if the loader were to add a custom hook, the oldHandler would reference it? just setTimeout for the next turn or?

Owner

petkaantonov commented Jan 7, 2015

So when is a good time to fire the onloadedevent so that if the loader were to add a custom hook, the oldHandler would reference it? just setTimeout for the next turn or?

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Jan 7, 2015

Collaborator

Ah, that indeed is an interesting problem to guard against - I'll have a quick look at what bookshelf and a few other libraries do and see if anyone is adding an async unPossiblyUnhandledRejection hook.

Intuition is an async.invoke should definitely be enough - is there any scenario where it'd run after an unhandled rejection is reported?

Collaborator

benjamingr commented Jan 7, 2015

Ah, that indeed is an interesting problem to guard against - I'll have a quick look at what bookshelf and a few other libraries do and see if anyone is adding an async unPossiblyUnhandledRejection hook.

Intuition is an async.invoke should definitely be enough - is there any scenario where it'd run after an unhandled rejection is reported?

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Jan 7, 2015

Owner

Btw it could take a long time to get the benefit from this as the older versions obviously don't fire the event.

Owner

petkaantonov commented Jan 7, 2015

Btw it could take a long time to get the benefit from this as the older versions obviously don't fire the event.

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Jan 7, 2015

Owner

All unhandled rejections are notified in an invokeLater queue, so async.invoke queue is fine

Owner

petkaantonov commented Jan 7, 2015

All unhandled rejections are notified in an invokeLater queue, so async.invoke queue is fine

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Jan 7, 2015

Owner

I really wonder though what a library could possibly do in the handler that it would depend on or couldn't have the application control. What if we just made them global, but allowed multiple listeners, in 3.0.0

Owner

petkaantonov commented Jan 7, 2015

I really wonder though what a library could possibly do in the handler that it would depend on or couldn't have the application control. What if we just made them global, but allowed multiple listeners, in 3.0.0

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Jan 7, 2015

Collaborator

I'm collecting stats, give me 10 minutes.

Collaborator

benjamingr commented Jan 7, 2015

I'm collecting stats, give me 10 minutes.

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Jan 7, 2015

Owner

I really like

process.on("possiblyUnhandledRejection")
process.on("unhandledRejectionHandled")
  • It allows multiple listeners

  • It allows the application to remove any custom listeners libraries added

    Like with uncaughtException, if there is not any listener it would only then do the default behavior of console.error (not crash cos it's still only "possible")

Owner

petkaantonov commented Jan 7, 2015

I really like

process.on("possiblyUnhandledRejection")
process.on("unhandledRejectionHandled")
  • It allows multiple listeners

  • It allows the application to remove any custom listeners libraries added

    Like with uncaughtException, if there is not any listener it would only then do the default behavior of console.error (not crash cos it's still only "possible")

@petkaantonov petkaantonov reopened this Jan 7, 2015

@petkaantonov petkaantonov added the 3.0.0 label Jan 7, 2015

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Jan 7, 2015

Collaborator

I like it too, just give me a few more minutes to collect data.

On Wed, Jan 7, 2015 at 8:36 AM, Petka Antonov notifications@github.com
wrote:

Reopened #357 #357.


Reply to this email directly or view it on GitHub
#357 (comment).

Collaborator

benjamingr commented Jan 7, 2015

I like it too, just give me a few more minutes to collect data.

On Wed, Jan 7, 2015 at 8:36 AM, Petka Antonov notifications@github.com
wrote:

Reopened #357 #357.


Reply to this email directly or view it on GitHub
#357 (comment).

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Jan 7, 2015

Collaborator

Some data from GitHub (NPM data incoming):

Ignored people who actually cloned Bluebird's code for this.
Testing code not included - those were the vast majority.
Lots of libraries (> 20) document having onPossiblyUnhandledRejection usable (in HTML and MD).
A lot of people copied Bluebird's source - mostly frontend projects.

Large (>1000 stars) libraries typically don't change onPossiblyUnhandledRejection:

  • Largest ORMS: Sequelize, Waterline and Bookshelf do not hook on it

Libraries and projects using onPossiblyUnhandledRejection in their code:

Collaborator

benjamingr commented Jan 7, 2015

Some data from GitHub (NPM data incoming):

Ignored people who actually cloned Bluebird's code for this.
Testing code not included - those were the vast majority.
Lots of libraries (> 20) document having onPossiblyUnhandledRejection usable (in HTML and MD).
A lot of people copied Bluebird's source - mostly frontend projects.

Large (>1000 stars) libraries typically don't change onPossiblyUnhandledRejection:

  • Largest ORMS: Sequelize, Waterline and Bookshelf do not hook on it

Libraries and projects using onPossiblyUnhandledRejection in their code:

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Jan 7, 2015

Collaborator

Some conclusions:

  • A lot of people find unPossiblyUnhandledRejection is really useful.
  • We might want to have a "users" section advertising all these people are using Bluebird as users tend to trust that stuff :P
  • The idea @petkaantonov proposed here (with the process events) seems like a pretty viable option but indeed it'll require older bluebird to upgrade. We might want a 1.x branch patch that emits these too. The bigger projects usually update to new versions of Bluebird.
Collaborator

benjamingr commented Jan 7, 2015

Some conclusions:

  • A lot of people find unPossiblyUnhandledRejection is really useful.
  • We might want to have a "users" section advertising all these people are using Bluebird as users tend to trust that stuff :P
  • The idea @petkaantonov proposed here (with the process events) seems like a pretty viable option but indeed it'll require older bluebird to upgrade. We might want a 1.x branch patch that emits these too. The bigger projects usually update to new versions of Bluebird.
@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Jan 7, 2015

It may also be useful to at least glance at http://lists.w3.org/Archives/Public/public-whatwg-archive/2014Sep/0024.html which we plan on for the browser. I doubt it makes sense to be entirely compatible (since e.g. ErrorEvent is not a thing in Node) but I'd love to make sure the concepts are aligned. Both of them are global handlers (window or process), is the idea.

domenic commented Jan 7, 2015

It may also be useful to at least glance at http://lists.w3.org/Archives/Public/public-whatwg-archive/2014Sep/0024.html which we plan on for the browser. I doubt it makes sense to be entirely compatible (since e.g. ErrorEvent is not a thing in Node) but I'd love to make sure the concepts are aligned. Both of them are global handlers (window or process), is the idea.

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Jan 7, 2015

Owner

@domenic I agree but since in node we are not reusing the uncaughtException event, it feels inconsistent to reuse the equivalent window error event in browsers. It also has weird parameters (message, url, line, column etc).

Owner

petkaantonov commented Jan 7, 2015

@domenic I agree but since in node we are not reusing the uncaughtException event, it feels inconsistent to reuse the equivalent window error event in browsers. It also has weird parameters (message, url, line, column etc).

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Jan 7, 2015

Collaborator

@domenic it seems like this proposal is about logging unhandled rejected promises to window.onerror so it's not useful for this particular issue (multiple instances of bluebird and a global hook on errors).

@petkaantonov It might be interesting to call window.onerror on a possibly unhandled rejection (and that's definitely something we should consider) - I'll open a new issue.

Collaborator

benjamingr commented Jan 7, 2015

@domenic it seems like this proposal is about logging unhandled rejected promises to window.onerror so it's not useful for this particular issue (multiple instances of bluebird and a global hook on errors).

@petkaantonov It might be interesting to call window.onerror on a possibly unhandled rejection (and that's definitely something we should consider) - I'll open a new issue.

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Jan 8, 2015

Collaborator

Some more stats, out of all the libraries that are on GH in npm and are dependent on Bluebird:

  • 1020 do not override onPossiblyUnhandledRejection in their code.
  • 60 do, most for testing or for d.ts definitions.
  • 35 say they have a github repo but really don't :)

I think we're ready to go forward with the event emission idea. Let's specify:

  • What does this do on browsers? (I believe nothing or call window.onPossiblyUnhandledRejection if it's a function are a good idea)
  • What does it do if the emit call throws? (suppress or raise a second unhandled rejection, throw seems unreasonable)
  • What does it get called with? (I believe the current err, promise is good)
Collaborator

benjamingr commented Jan 8, 2015

Some more stats, out of all the libraries that are on GH in npm and are dependent on Bluebird:

  • 1020 do not override onPossiblyUnhandledRejection in their code.
  • 60 do, most for testing or for d.ts definitions.
  • 35 say they have a github repo but really don't :)

I think we're ready to go forward with the event emission idea. Let's specify:

  • What does this do on browsers? (I believe nothing or call window.onPossiblyUnhandledRejection if it's a function are a good idea)
  • What does it do if the emit call throws? (suppress or raise a second unhandled rejection, throw seems unreasonable)
  • What does it get called with? (I believe the current err, promise is good)
@benjamingr

This comment has been minimized.

Show comment
Hide comment
@sheltond

This comment has been minimized.

Show comment
Hide comment
@sheltond

sheltond Jan 8, 2015

I think what he's asking for is a way to hook up to every bluebird instance
currently running (yours, Sequelize's etc).

Yes, thanks, that's what I meant.

process.on("possiblyUnhandledRejection")
process.on("unhandledRejectionHandled")

Great - that looks like it would solve my problem.

sheltond commented Jan 8, 2015

I think what he's asking for is a way to hook up to every bluebird instance
currently running (yours, Sequelize's etc).

Yes, thanks, that's what I meant.

process.on("possiblyUnhandledRejection")
process.on("unhandledRejectionHandled")

Great - that looks like it would solve my problem.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Jan 8, 2015

What does this do on browsers? (I believe nothing or call window.onPossiblyUnhandledRejection if it's a function are a good idea)

I would suggest constructing a new CustomEvent, setting a .promise property, and firing it on window. Basically, polyfill the future API, except choose the variant that does not overload window.onerror.

domenic commented Jan 8, 2015

What does this do on browsers? (I believe nothing or call window.onPossiblyUnhandledRejection if it's a function are a good idea)

I would suggest constructing a new CustomEvent, setting a .promise property, and firing it on window. Basically, polyfill the future API, except choose the variant that does not overload window.onerror.

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