Skip to content

Signal handling improvements#329

Open
rhansen wants to merge 5 commits into
andywer:masterfrom
rhansen:signal-handling
Open

Signal handling improvements#329
rhansen wants to merge 5 commits into
andywer:masterfrom
rhansen:signal-handling

Conversation

@rhansen

@rhansen rhansen commented Dec 21, 2020

Copy link
Copy Markdown

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.

@andywer

andywer commented Dec 22, 2020

Copy link
Copy Markdown
Owner

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.

@JeremieDoctrine

JeremieDoctrine commented Sep 21, 2021

Copy link
Copy Markdown

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.

@Huararanga

Copy link
Copy Markdown

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

@rhansen

rhansen commented Oct 15, 2021

Copy link
Copy Markdown
Author

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.

@CMCDragonkai

Copy link
Copy Markdown

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.

Comment thread src/master/implementation.node.ts
@rhansen

rhansen commented Dec 1, 2021

Copy link
Copy Markdown
Author

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 added 5 commits April 16, 2022 01:48
The default signal handler always exits non-zero so the signal
listener installed by threads.js should too.
This also suppresses any synchronous exceptions thrown by
`worker.terminate()`.
The promise returned from `Promise.all()` immediately rejects if any
of the underlying promises reject, even if some of the other promises
have not yet settled. Catch each rejection to give the other promises
an opportunity to resolve.

`Promise.allSettled()` could be used instead, but that's a relatively
new function that was added in Node.js v12.9.0.
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.

5 participants