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

worker.exitedAfterDisconnect is set to false after worker.kill(), which doesn't comply with the documentation #51934

Closed
khaled4vokalz opened this issue Mar 1, 2024 · 4 comments

Comments

@khaled4vokalz
Copy link

khaled4vokalz commented Mar 1, 2024

Version

v18.16.0 (haven't check older version btw)

Platform

Linux 6.5.0-21-generic #21-Ubuntu SMP PREEMPT_DYNAMIC Wed Feb 7 14:17:40 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

No response

What steps will reproduce the bug?

  • Run the below code block using node index.js e.g.
const http = require("node:http");
const process = require("node:process");

if (cluster.isPrimary) {
  // Keep track of http requests
  let numReqs = 0;
  let inter = setInterval(() => {
    console.log(`numReqs = ${numReqs}`);
  }, 1000);

  // Count requests
  function messageHandler(msg) {
    if (msg.cmd && msg.cmd === "notifyRequest") {
      numReqs += 1;
    }
  }

  // Start workers and listen for messages containing notifyRequest
  const numCPUs = require("node:os").availableParallelism();
  for (let i = 0; i < 2; i++) {
    cluster.fork();
  }

  for (const id in cluster.workers) {
    cluster.workers[id].on("message", messageHandler);
  }
  cluster.on("exit", (worker, code, signal) => {
    console.log(
      `--------------EAD---------------${worker.exitedAfterDisconnect}-----`,
    );
    if (worker.exitedAfterDisconnect === true) {
      console.log("Oh, it was just voluntary – no need to worry");
    } else {
      console.log(`-----------EXIT------------------${worker.process.pid}`);
    }
  });
  process.on("SIGINT", async (signal) => {
    console.log("----------------SIGINT----------------------");
    await killChildProcesses(cluster);
    console.log("-----------------DONE------------------");
    process.exit(0);
  });
} else {
  // Worker processes have a http server.
  const server = http
    .Server((req, res) => {
      res.writeHead(200);
      res.end("hello world\n");

      // Notify primary about the request
      process.send({ cmd: "notifyRequest" });
    })
    .listen(4000);
  process.on("SIGTERM", (signal) => {
    console.log("----------------SIGTERM----------------------");
    server.close(() => {
      console.log("----------------------CLOSED---------------------");
      process.exit(0);
    });
  });
}

function killChildProcesses(cluster) {
  return new Promise((resolve) => {
    const aliveWorkers = Object.values(cluster.workers || {}).filter(
      (worker) => {
        return worker && !worker.isDead();
      },
    );

    if (aliveWorkers.length === 0) {
      resolve();
      return;
    }

    let workersAlive = aliveWorkers.length;

    const handleWorkerExit = () => {
      workersAlive--;
      if (workersAlive === 0) {
        resolve();
      }
    };

    aliveWorkers.forEach((worker) => {
      console.log("-----------KILLING-----------");
      worker.on("exit", handleWorkerExit);
      worker.kill();
    });
  });
}

  • find out the primary process number for this e.g. ps -ef | grep index.js
  • kill the process with SIGINT signal, i.e. kill -SIGINT <pid>
  • the resulting console output is as below
----------------SIGINT----------------------
-----------KILLING-----------
-----------KILLING-----------
----------------SIGTERM----------------------
----------------SIGTERM----------------------
----------------------CLOSED---------------------
----------------------CLOSED---------------------
--------------EAD---------------false--0---
-----------EXIT------------------<worker-1-pid>
--------------EAD---------------false--0---
-----------EXIT------------------<worker-2-pid>
-----------------DONE------------------

How often does it reproduce? Is there a required condition?

always

What is the expected behavior? Why is that the expected behavior?

The expected behavior should be

----------------SIGINT----------------------
-----------KILLING-----------
-----------KILLING-----------
----------------SIGTERM----------------------
----------------SIGTERM----------------------
----------------------CLOSED---------------------
----------------------CLOSED---------------------
--------------EAD---------------false--0---
Oh, it was just voluntary – no need to worry
--------------EAD---------------false--0---
Oh, it was just voluntary – no need to worry
-----------------DONE------------------

What do you see instead?

As you can see the worker.exitedAfterDisconnect is not true, but the documentation (https://nodejs.org/api/cluster.html#workerexitedafterdisconnect) states that it should be true if you kill() or disconnect() the worker.

image

As you can the worker process exited with code 0, I don't understand which other way it can be exited with code 0 other that what the code is doing...

Additional information

No response

@aduh95
Copy link
Contributor

aduh95 commented Mar 2, 2024

Here's the relevant documentation:

node/doc/api/cluster.md

Lines 447 to 457 in 84716d8

### `worker.exitedAfterDisconnect`
<!-- YAML
added: v6.0.0
-->
* {boolean}
This property is `true` if the worker exited due to `.disconnect()`.
If the worker exited any other way, it is `false`. If the
worker has not exited, it is `undefined`.

According to your repro steps, you never call worker.disconnect(), so according to the docs, worker.exitedAfterDisconnect should be false (the worker exited any other way). Am I missing something?

@khaled4vokalz
Copy link
Author

khaled4vokalz commented Mar 2, 2024

okay, I guess it's a types doc issue then? https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/node/v16/cluster.d.ts#L269

Was it previously supported on kill() api as well? or it's just a typo? that's the reason I shared the picture of the typings (I am using types/node version 17)

Also, the DOC says
image

But from code I don't see something like this... 🤔
https://github.com/nodejs/node/blob/main/lib/internal/cluster/primary.js#L360

Or I am looking into the wrong place? 🤔

@khaled4vokalz
Copy link
Author

closing this issue as calling worker#disconnect() works as expected....

@aduh95
Copy link
Contributor

aduh95 commented Mar 2, 2024

FYI @types/node is not maintained in this repo – but you probably already know that since you linked to the DefinitelyTyped repo.

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

No branches or pull requests

2 participants