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

Calling disconnect causes processes spawned by cluster module to exit too early #27679

Open
bimbiltu opened this issue May 13, 2019 · 6 comments
Labels
cluster Issues and PRs related to the cluster subsystem. help wanted Issues that need assistance from volunteers or PRs that need help to proceed.

Comments

@bimbiltu
Copy link

bimbiltu commented May 13, 2019

  • Version: 12.2.0 but could also reproduce on 10.15.3
  • Platform: Darwin 18.5.0 Darwin Kernel Version 18.5.0: Mon Mar 11 20:40:32 PDT 2019; root:xnu-4903.251.3~3/RELEASE_X86_64 x86_64
  • Subsystem:

Consider the following script:

const cluster = require('cluster');
const childProcess = require('child_process');

const useCluster = false;
const isMaster = useCluster ? cluster.isMaster : !process.argv.includes('worker');

if (isMaster) {
  if (useCluster) {
    cluster.fork(__filename);
  } else {
    childProcess.fork(__filename, ['worker']);
  }
} else {
  setTimeout(() => console.log('hi from worker'), 1000);
  process.disconnect();
}

When useCluster is true node exits immediately and nothing is printed to the console. When useCluster is false you see "hi from worker" logged after 1s and then node will exit which is the expected behavior. The documentation makes it sound like calling process.disconnect should only close the IPC channel between the master and worker process and should not cause either to exit early if there is still work to do.

@sam-github
Copy link
Contributor

Sorry, I ran out of time to look at this, but what I suspect is happening is that the master does not expect to see the IPC pipe closed directly, without the worker sending a message saying that it is going to do so, considers this to be unexpected, and kills the worker. I had trouble proving that, though, more research would be required.

Note that using process.worker.disconnect() instead of process.disconnect() gives the behaviour you expect. If you just want a workaround, doing (process.worker ? process.worker.disconnect : process.disconnect)() would allow more symetricality between cluster workers and forked child processes.

@oyyd oyyd added the cluster Issues and PRs related to the cluster subsystem. label May 14, 2019
@bimbiltu
Copy link
Author

@sam-github process.worker is undefined for me when using both cluster and child_process. I tried this with node 10.15.3 as well as 12.2.0 on macOS mojave if that makes a difference. I've just switched to using child_process for now in my code since we were really only using the cluster module for its automatic debug port incrementing

@sam-github
Copy link
Contributor

Apologies, I meant cluster.worker.disconnect.

The cluster modules is not very flexible, if its not being used for exactly the intended use-case (identical net or http servers), its features easily become misfeatures, so using child_process is probably a better idea for you.

@jasnell jasnell added the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label Jun 26, 2020
@ledbit
Copy link

ledbit commented Aug 13, 2020

Just ran into a variation of this issue, in our case the master process exits and we want to perform some proper graceful shutdown on the child processes. Happy to contribute, but want to validate that I have the correct approach first. I believe the root cause of both issues (master process going away and process.disconnect) is in this listener added during cluster's worker setup.

The approach that I think would solve this issue would be to add an option (similar to exitedOnDisconnect) that would control whether the child process should exit on disconnect. Can someone please chime in on whether this is the right approach?

@drewswanner
Copy link

Is anyone working on this I would be interested in looking into it. I may need some guidance but I am willing to give it a shot.

@SeanMcCord
Copy link

Based on the documentation I believe process.disconnect is working correctly and as expected. @sam-github makes a good point about the cluster module flexibility. Using the cluster module effectively requires the usage of worker.disconnect to handle graceful shutdown*. This has been surprising behavior to people since 2016[1][2]. I'm not convinced that a change to either the cluster or the child_process modules are needed.

The 2017 issue[2] suggested updating the documentation to make this behavior clear; however, the documentation does not appear to have been changed. I've created a PR for updating the documentation #38713

[1] #13671
[2] https://stackoverflow.com/a/40846737

  • You could hack your way around this by manually setting worker.exitedAfterDisconnect to true before calling process.disconnect, but obviously that is gross.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cluster Issues and PRs related to the cluster subsystem. help wanted Issues that need assistance from volunteers or PRs that need help to proceed.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants