emit signal source for `cleanup`

[huan]

I would like to receive a reason in cleanup callback:

nodeCleanup(function (reason) {
    // release resources here before node exits 
});

so I could log the reason by myself, or send to my log server.

can we do this by emit the reason with 'cleanup'?

        process.emit('cleanup', reason);

If it's ok, I can send a PR.

[jtlapp]

Sounds useful, but I don't understand where you'll be getting the reason. It seems that the most we can pass is the error code, which is a number. I guess that could be useful. (See the exit event.)

[huan]

The error code is not enough for my case.

I have a chatbot, I want this bot to say something before it dies. For example, something like "Ah! I dead because of someone press Ctrl+C...

Now, I can say nothing about the reason... https://github.com/wechaty/wechaty/blob/master/example/ding-dong-bot.ts#L85

[jtlapp]

Go ahead and show me how you'll add the reason to node-cleanup. If you have a way to supply that reason, sure, we can add it. But right now I don't see how you'll be getting the reason.

[huan]

get it done. :)

nodeCleanup((reason, code) => {
  const exitMsg = `Wechaty exit ${code} because of ${reason} `
  console.log(exitMsg)
  bot.say(exitMsg)
})

Wechaty exit 2 because of [ctrl-C] 

[jtlapp]

But your reason is redundant with the exit code, except that because there are more exit codes than reasons, exit code is more informative.

Also, different users may need different statements of the reason, such as for different spoken languages.

I think we should pass the exit code to the cleanup handler, but I think it's the cleanup handler's responsibility to translate that exit code into text.

[huan]

Hi @jtlapp ,

I believe the reason is useful because it is the parameter value of process.on().

Maybe rename the reason to signal is more readable, as in my Pull Request, it is one of the following signal names:

1. SIGINT
2. uncaughtException
3. exit

I had added a new commit to the PR, please have a look and let me know if you think it makes sense.

[jtlapp]

You can do the same thing far more simply and far more flexibly. The only change to node-cleanup would be to pass the exit code to the cleanup handler. Nothing else.

After that, your cleanup handler can switch on the code:

nodeCleanup((code) => {
    var reason = 'default reason'; // e.g. 'exit'
    switch (code) {
    case 2:
        reason = 'SIGINT';
        break;
    case 99:
        reason = 'uncaughtException';
        break;
    }
    // do something with your reason
})

This way, anyone can translate exit codes into any reason text they want, even varying that text for different spoken languages.

Having node-cleanup post the reason makes node-cleanup complicated, and it does so at a huge disadvantage in flexibility. It just doesn't make sense to do things that way.

[huan]

Sorry, but I don't think so.

What if I called process.exit(2) directly in my node program?

I will expect a exit: 2 but your code will give me SIGINT 2 which is not what I want(also not correct because there's no SIGINT at all).

How do you think about this use case?

[jtlapp]

You're supposed to use different exit codes for different purposes. On Unix, it is conventional for exit code 2 to be SIGINT, so you shouldn't use it for anything else.

[jtlapp]

In any case, each application decides what its non-zero exit codes mean. You control the meanings. By using node-cleanup, you're deciding that 2 means SIGINT and 99 means uncaught exception. If you decide that 2 can also mean something else, then you have chosen to make 2 ambiguous. Just don't do that, and you have no problem.

[huan]

Yes, I agree with you.

However, I can not control what my user do. Because I want to use node-cleanup in my framework and tell the user what happens.

If my user use process.exit(2) by some reason, I want to have the ability to output the right information.

So, how about let's put the signal name at the second param? It would like this: callback(code, signal) and I wish this could make you more comfortable because the signal param can be ignored.

It's ok if you still think this is no sense, then I have to embed the code instead of using the npm module.

[jtlapp]

If you're leaving it up to the user to decide what the exit codes mean, then what you really need is the ability to configure the exit codes.

Since javascript is interpreted, the interpreter can also choose to error out of the program. I found the standard node.js error codes here.

In particular, the SIGINT exit code is supposed to be 128+2 = 130, so we do need to change that.

[huan]

Sorry, What I only care about is whether a user pressed Ctrl+C, or just called process.exit(2) or process.exit(130).

I do not care about whether the user follows the node.js standard because not all users will.

That's the reason why I need the callback to have both exit code and signal name. If you do not want to extend the params of callback, then I will leave this issue here and find other ways to do that. ;-p

[jtlapp]

Yeah, I believe you can do what you need to do with just the exit code. This discussion has been helpful because it showed we need to make two changes:

(1) Pass the exit code to the cleanup handler. (2) Change the SIGINT exit code from 2 to 130.

If you want to make that patch, I'll be happy to accept it.

[huan]

Ok, I did a new PR that can fit both your 1 & 2.

And there's also an additional param 'signal`, which you can totally ignore it. (but I really need it, and you can never pay attention about that).

How about we use this version of solution, and make both of us happy? if so, I'll really appreciate.

[jtlapp]

Sorry, I just don't see the need for all the signal/reason text complexity. If someone uses exit code 130 (formerly 2) for anything but SIGINT, they're doing it wrong and get what they deserve.

I need to take care of other things. I'm happy to merge ONLY changes (1) and (2).

[huan]

Ok. PR sent. I believe you get what you want now. :)

[jtlapp]

Awesome! Thank you for these improvements!

[jtlapp]

Oops, sorry! I thought I had. Thanks!

[huan]

You are welcome. :)

[jtlapp]

Hey @zixia! Someone raised this issue about SIGINT handling, and I ended up having to rewrite everything. In the course of the rewrite I discovered that node does not employ the 128+signal_number scheme that it defines. Instead it uses a two-parameter scheme similar to the one you proposed, but not quite the same. Check out the child process exit event handler. Read the new README for a detailed explanation of the way things work now. You were on the right track!

[huan]

Hey @jtlapp

Great to hear that, thanks! :)