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

Do not kill the master process #324

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

mjgp2
Copy link
Contributor

@mjgp2 mjgp2 commented Dec 1, 2020

There may be graceful shutdown happening elsewhere - other processes can be orphaned by doing this.

@rhansen
Copy link

rhansen commented Dec 15, 2020

I too am affected by this: the SIGINT and SIGTERM listeners installed by threads.js terminate Node.js before my cleanup code finishes running.

I don't think this pull request is the correct fix. Installing any signal handler inhibits the default behavior (which is to terminate node). See #198 for the reason why process.exit() was added in the first place.

What is needed is the ability to terminate workers without inhibiting the default signal handling behavior. Unfortunately there is no way to do this. Maybe this hacky approach is good enough:

  const terminateWorkersAndMaster = (signal) => {
    Promise.all(allWorkers.map(worker => worker.terminate().catch(() => {}))).then(() => {
      if (process.listenerCount(signal) > 1) {
        // Assume that one of the other signal listeners will take care of calling process.exit().
        // This breaks down if all of the other listeners are making the same assumption.
        // Unfortunately Node.js does not provide a way to run code when a signal arrives without
        // inhibiting the default signal handler. (The 'exit' event isn't usable for this because it
        // is not emitted when the default signal handler terminates the process.)
        return
      }
      // Right now this is the only signal listener, so assume that this listener is to blame for
      // inhibiting the default signal handler. (This assumption fails if the number of listeners
      // changes during signal handling. This can happen if a listener was added by process.once().)
      // Mimic the default behavior, which is to exit with a non-0 code.
      process.exit(1)
    })
    allWorkers = []
  }

@andywer
Copy link
Owner

andywer commented Dec 16, 2020

@rhansen Thank you for sharing that insight!
@mjgp2 Any thoughts?

Good thing I was so slow to merge this PR 😜

@rhansen
Copy link

rhansen commented Dec 21, 2020

I opened pull request #329 with my suggested approach.

@Huararanga
Copy link

I agree you should not install any signal listeners

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.

4 participants