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

Enhanced signal support #22

Closed

Conversation

wearhere
Copy link

@wearhere wearhere commented Oct 10, 2016

This PR enables throng to observe a shutdown signal beyond SIGINT and SIGTERM, for instance SIGUSR2. Two use cases that support the various changes in this PR:

  1. The process monitor we use for local development, nodemon, uses SIGUSR2 to restart the process. If the process traps the signal to enact some graceful shutdown, nodemon expects the process to re-kill itself with SIGUSR2—nodemon detects the corresponding exit code to resume control.
  2. We run our applications on AWS Elastic Beanstalk. We add support for graceful shutdown to EB simply by sending SIGUSR2 to all Node processes before EB enacts deploys, makes configuration changes, etc; and then waiting for a period of time for the processes to exit.

Why SIGUSR2 and not one of the default signals (SIGTERM or SIGINT)? In the case of nodemon, because it's not customizable; in the case of EB, because we send the signal to all processes and we don't want the worker processes to exit before the master has begun the shutdown process.

The EB use case is the more compelling since we don't really need to use clustering in local development. But, now throng supports that too. The only nodemon-specific change is having throng exit with the code corresponding to the shutdown signal. If you feel we shouldn't change that behavior I can take it out, but it seems like good practice for the process to respect the signal exit codes as documented by Node in “Signal Exits” here.

@wearhere wearhere force-pushed the jeff/enhanced_signal_support branch 2 times, most recently from 9c2f702 to 6f47104 Compare October 11, 2016 01:40
If a client kills the master process with a particular signal, it can reasonably
expect the process to exit with the corresponding exit code as defined in
“Signal Exits” here: https://nodejs.org/api/process.html#process_exit_codes.

We change to observing the shutdown signals `once` so that we can re-kill
with the shutdown signal after the child processes disconnect.
Beyond `SIGINT` and `SIGTERM`.
@tillkruss
Copy link

Is there a reason why this hasn't been merged, yet?

@hunterloftis
Copy link
Owner

Thanks for explaining the various scenarios you're running into. I believe v5 can handle them; please re-open if there's something I'm missing: https://github.com/hunterloftis/throng#all-options-with-defaults

@wearhere
Copy link
Author

Hey @hunterloftis, it looks like v5 can indeed observe additional shutdown signals, that's great. I think that it would still be useful to have throng exit with the code corresponding to the shutdown signal https://github.com/hunterloftis/throng/pull/22/files#diff-73e968726de16eeee0444f17bb9afc09R67 to respect the signal exit codes as documented by Node in “Signal Exits” here.

@hunterloftis
Copy link
Owner

>128 Signal Exits: If Node.js receives a fatal signal such as SIGKILL or SIGHUP, then its exit code will be 128 plus the value of the signal code. This is a standard POSIX practice, since exit codes are defined to be 7-bit integers, and signal exits set the high-order bit, and then contain the value of the signal code. For example, signal SIGABRT has value 6, so the expected exit code will be 128 + 6, or 134.

TIL! I agree, that sounds like standard behavior that other tools might depend on. Issue created.

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

Successfully merging this pull request may close these issues.

3 participants