threads.js icon indicating copy to clipboard operation
threads.js copied to clipboard

Signal handling improvements

Open rhansen opened this issue 4 years ago • 6 comments

Multiple commits (intentionally separate, please do not squash):

  • Signal handling: Factor out common signal handling code
  • Signal handling: Always exit non-zero
  • Signal handling: Allow worker.terminate() to be synchronous
  • Signal handling: Wait for all workers to terminate
  • Signal handling: Don't exit if there are other signal listeners

This is intended as an alternative to pull request #324.

rhansen avatar Dec 21 '20 22:12 rhansen

Great stuff, @rhansen! Thanks so much.

@mjgp2 Can you check if this fix solves the issue for you, too? I don't see any reason why not, but an additional test would still be nice.

andywer avatar Dec 22 '20 15:12 andywer

Hi, @andywer any reason not to merge this? This fixes our issue as well.

We've temporarily addressed this by doing :

    process.removeAllListeners('SIGTERM');
    process.on('SIGTERM', () => {
      ...
    }

But it is not really satisfying.

JeremieDoctrine avatar Sep 21 '21 07:09 JeremieDoctrine

I dont think you should install any signal listeners it's dangerous and out of scope of the common library..

Huararanga avatar Oct 14 '21 17:10 Huararanga

I dont think you should install any signal listeners it's dangerous and out of scope of the common library..

Note that this PR doesn't add signal handling, it only attempts to improve the existing signal handling. Removing the signal handlers entirely is a topic that deserves discussion in a dedicated bug report. The scope of this PR is intentionally narrow to avoid breaking existing users.

rhansen avatar Oct 15 '21 05:10 rhansen

Can signal handling be provided by as an additional utility function that users can install? It's quite dangerous for the signal handlers to be installed upon import.

CMCDragonkai avatar Nov 30 '21 12:11 CMCDragonkai

Can signal handling be provided by as an additional utility function that users can install? It's quite dangerous for the signal handlers to be installed upon import.

I'm all for that, but it would require a major version bump. This PR attempts to preserve compatibility as much as possible so that it can be merged without delay (ping @andywer). My preference would be to merge this as-is, then open another PR that overhauls cleanup.

Note that Node.js currently does not provide a robust way to perform cleanup actions, so there may never be a completely satisfactory solution. See https://github.com/nodejs/node/discussions/29480.

rhansen avatar Dec 01 '21 04:12 rhansen