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

fix: 🐛 process.exitCode instead of calling process.exit to allow cleanup #483

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

Conversation

pcace
Copy link

@pcace pcace commented Dec 18, 2024

closes #429

see #429

The test sometimes failed, i guess because it now takes a bit longer to finish code and exit. after using 2000 it never failed on my machine. Hope thats ok.

worker.exit = process.exit.bind(process);
worker.exit = function (code) {
process.exitCode = code;
parentPort.postMessage('__workerpool-terminate__');
Copy link
Contributor

@joshLong145 joshLong145 Feb 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am confused on why we are seeing a message to the parent for shutdown when this message typically sent from parent to child, not child to parent. The parent message handler does not have implementations to deal with this message type.

Perhaps I am missing something, but to me this is not going to achieve the goals of providing the exit code on shutdown. Can't there be another binding for worker.exitCode like worker.exit was binded?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I have the same question indeed, this message is only send from the main process to the worker right now.

I would expect something like:

worker.exit = function (code) {
  process.exitCode = code
  process.exit()
}

@josdejong
Copy link
Owner

Thanks for your PR @pcace, and sorry for the late reply.

Two remarks:

  • Can you have a look at the inline comment of Bean?
  • How can we reproduce your issue, so we can test whether this PR actually solves the issue? Is it possible to write a unit test to validate the fix?

@pcace
Copy link
Author

pcace commented Mar 6, 2025

Hi there, I am very sorry, but I am not using it anymore, so my testcase is gone.
its totally ok if you close the pr.

@josdejong
Copy link
Owner

Okido, I'll close your PR. Thanks anyway for your efforts!

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.

worker.exit should set process.exitCode instead of calling process.exit to allow cleanup
3 participants