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

Gracefully shutdown process on SIGTERM #198

Merged
merged 2 commits into from
Jan 20, 2020

Conversation

doochik
Copy link
Contributor

@doochik doochik commented Jan 9, 2020

We should manually shutdown process if SIGTERM/SIGINT listeners are installed

nodejs docs (https://nodejs.org/dist/latest-v12.x/docs/api/process.html#process_signal_events)

'SIGTERM' and 'SIGINT' have default handlers on non-Windows platforms that reset the terminal mode before exiting with code 128 + signal number. If one of these signals has a listener installed, its default behavior will be removed (Node.js will no longer exit).

tiny-worker docs (https://github.com/avoidwork/tiny-worker#faq)

In your core script register a listener for SIGTERM or SIGINT via process.on() which terminates (all) worker process(es) and then gracefully shutdowns via process.exit(0);

Example:
Problem: cluster workers don't shutdown because theads.js has invalid SIGTERM listener.

// cluster.js

const cluster = require('cluster');

if (cluster.isMaster) {
    console.log(`Master ${ process.pid } is running`);

    cluster.fork();

    cluster.on('exit', (worker, code, signal) => {
        console.log(`worker ${ worker.process.pid } died`);
    });
} else {
    const { spawn, Worker } = require('threads');

    spawn(new Worker('./worker.js'));
    console.log(`Worker ${ process.pid } started`);
}
// worker.js

const { expose } = require('threads/worker');

expose(() => 'hello from worker');

Run

$ node --version
v12.14.1
$ node cluster.js
Master 64701 is running
Worker 64702 started
$ kill 64702
# nothing happens... But we should see "worker 64702 died"
# PR fixes this problem

@andywer
Copy link
Owner

andywer commented Jan 20, 2020

Sorry, have been sick for the last two weeks. Good point and thanks for the PR!

I just changed the name of terminateAll to reflect its broadened purpose.

@doochik
Copy link
Contributor Author

doochik commented Jan 20, 2020

Thanks!

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.

2 participants