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

doc: specify cluster worker.kill() caveat #23165

Merged
merged 2 commits into from
Oct 4, 2018
Merged

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Sep 29, 2018

worker.kill() relies on a graceful disconnect, which might not always be possible. This commit calls this out in the docs, and specifies worker.process.kill() as a non-graceful alternative.

Fixes: #22703

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added cluster Issues and PRs related to the cluster subsystem. doc Issues and PRs related to the documentations. labels Sep 29, 2018
@thefourtheye
Copy link
Contributor

Do we need any tests which back this text?

@cjihrig
Copy link
Contributor Author

cjihrig commented Oct 4, 2018

@thefourtheye I added a test for worker.process.kill(). I'd rather not add a test for worker.kill() because I don't want to introduce a test that is expected to not terminate and need setTimeout()s that are prone to becoming flaky.

CI: https://ci.nodejs.org/job/node-test-pull-request/17616/

@thefourtheye
Copy link
Contributor

@cjihrig Fair enough 👍

worker.kill() relies on a graceful disconnect, which might not
always be possible. This commit calls this out in the docs, and
specifies worker.process.kill() as a non-graceful alternative.

PR-URL: nodejs#23165
Fixes: nodejs#22703
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Verify that worker.process.kill() can terminate a cluster worker
stuck in an infinite loop.

PR-URL: nodejs#23165
Fixes: nodejs#22703
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@cjihrig
Copy link
Contributor Author

cjihrig commented Oct 4, 2018

@cjihrig cjihrig merged commit 1f7b6a6 into nodejs:master Oct 4, 2018
@cjihrig cjihrig deleted the cluster-doc branch October 4, 2018 13:04
targos pushed a commit that referenced this pull request Oct 5, 2018
worker.kill() relies on a graceful disconnect, which might not
always be possible. This commit calls this out in the docs, and
specifies worker.process.kill() as a non-graceful alternative.

PR-URL: #23165
Fixes: #22703
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
targos pushed a commit that referenced this pull request Oct 5, 2018
Verify that worker.process.kill() can terminate a cluster worker
stuck in an infinite loop.

PR-URL: #23165
Fixes: #22703
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
jasnell pushed a commit that referenced this pull request Oct 17, 2018
worker.kill() relies on a graceful disconnect, which might not
always be possible. This commit calls this out in the docs, and
specifies worker.process.kill() as a non-graceful alternative.

PR-URL: #23165
Fixes: #22703
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
jasnell pushed a commit that referenced this pull request Oct 17, 2018
Verify that worker.process.kill() can terminate a cluster worker
stuck in an infinite loop.

PR-URL: #23165
Fixes: #22703
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
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. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants