Skip to content
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

Cannot bind uncaughtException & unhandledRejection together #5

Open
vekexasia opened this issue Nov 28, 2017 · 10 comments
Open

Cannot bind uncaughtException & unhandledRejection together #5

vekexasia opened this issue Nov 28, 2017 · 10 comments
Assignees

Comments

@vekexasia
Copy link

Hello,

it looks like you can bind to both. Is there any particoular reason?

Also, would you expose the exit function so that one can eventually calll it manually?

@Tapppi Tapppi self-assigned this Dec 11, 2017
@Tapppi
Copy link
Owner

Tapppi commented Dec 19, 2017

Sorry for the late response!

it looks like you can bind to both. Is there any particoular reason?

Yes, one might want to bind different handlers. Should make a generic exit handler though.. Any suggestions for names etc.?

Also, would you expose the exit function so that one can eventually calll it manually?

Interesting idea, I'll have to give this further thought, but possibly.

@rijnhard
Copy link

on the exit function, that would be a good idea, as then there's a safe way to exit the process.
Possibly even override process.exit to be safe (but that should be optional)

@Tapppi
Copy link
Owner

Tapppi commented Feb 20, 2018

on the exit function, that would be a good idea, as then there's a safe way to exit the process. Possibly even override process.exit to be safe (but that should be optional)

Currently figuring out all the implications of this. I'll try to carve out some more OSS time next weekend and get this in. Small change but node's process lifecycle is quite unfriendly to customisation, in part due to OS differences.

@rijnhard
Copy link

@Tapppi working with AsyncExitHook more essentially what I think it needs to do is simply
on asyncExitHook.exit(0);

  • fire exit handlers
  • wait until all are complete (maybe have a timeout here.)
  • then call process.exit(code);

@rijnhard
Copy link

@Tapppi any movement here?

@rijnhard
Copy link

Just saying, I ran across this again today

@rijnhard
Copy link

rijnhard commented Apr 24, 2019

Workaround for now. dont use either function and do this instead.

import asyncExitHook from 'async-exit-hook';

let stopped = false;

/**
 * closes thingy before exiting
 * @param {function} cb callback for async exit hook
 */
function shutdown(cb = () => {}) {
    if (!stopped) {
        stopped = true;
        Logger.info('thingy is shutting down...');

        thingy1.stop((success) => {
            thingy2.shutdown().finally(() => cb());
        });
    }
}

asyncExitHook((cb) => {
    Logger.info('thingy is exiting');
    shutdown(cb);
});

asyncExitHook.hookEvent('uncaughtException', 1, (err) => {
    Logger.fatal('thingy is shutting down due to UncaughtException...');
    return false;
});

asyncExitHook.hookEvent('unhandledRejection', 2, (err) => {
    Logger.fatal('thingy is shutting down due to unhandledRejectionHandler...');
    return false;
});

@meotimdihia
Copy link

@Tapppi: this problem needs to write in the documentation. Or need to fix it.

@geogeim
Copy link

geogeim commented Sep 19, 2019

please fix this, i lost a few hours debugging

@larvanitis
Copy link

There are two problems with the current implementation:

1. Only the first hooked event is registered.

Both registration functions check for the single errHooks array's length.

if (errHooks.length === 1) {
if (errHooks.length === 1) {

2. All error hook handlers will be called, even if they weren't registered on that event (from 1).

Both errors trigger all errHooks.

errHooks.map(runHook.bind(null, 1, err));

Solution

Both can be fixed by using separate arrays - eg uncaughtExceptionHooks and unhandledRejectionHooks and checking for the actual error at

if (err) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants