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.

Sign up for GitHub

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

Jump to bottom

worker.exitedAfterDisconnect is false, after the worker process has exited using process.exit() #28837

Closed
nimit95 opened this issue Jul 24, 2019 · 7 comments
Closed

worker.exitedAfterDisconnect is false, after the worker process has exited using process.exit() #28837

nimit95 opened this issue Jul 24, 2019 · 7 comments

Comments

@nimit95
Copy link
Contributor

nimit95 commented Jul 24, 2019 •
edited
Loading

Version: 10.16.0
Platform: 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

Call process.exit() in worker/child, in master log the value after any worker exit
@nimit95 nimit95 changed the title worker.exitedAfterDisconnect is false, after the worker process has exited on its own worker.exitedAfterDisconnect is false, after the worker process has exited using process.exit() Jul 24, 2019
@sam-github
Copy link
Contributor

https://nodejs.org/api/cluster.html#cluster_worker_exitedafterdisconnect

Set by calling .kill() or .disconnect(). Until then, it is undefined.

The behaviour you describe is exactly what is documented. Neither kill() or disconnect() was called in your situation, the worker called exit(), so exitedAfterDisconnect is false.

@nimit95
Copy link
Contributor Author

nimit95 commented Jul 24, 2019

But shouldn't it be undefined as documented. .kill() or .disconnect() has not been called

@sam-github sam-github reopened this Jul 24, 2019
@nimit95
Copy link
Contributor Author

nimit95 commented Jul 24, 2019

Should we fix it at the documentation level or fix the code?
I can make PR for any of the above two case

@sam-github
Copy link
Contributor

It's repeatedly normalized from truthy/falsy to true/false: worker.exitedAfterDisconnect = !!worker.exitedAfterDisconnect. I'm not sure about the history of this. Can you troll the git history to see why this occurs?

I strongly suspect current behaviour is an improvement, but the docs need updating.

Also, I wonder, is the initial value false? If not, it probably should be.

@nimit95
Copy link
Contributor Author

nimit95 commented Jul 24, 2019

Okay I will see what I can find and get back

@nimit95
Copy link
Contributor Author

nimit95 commented Jul 24, 2019 •
edited
Loading

Yeah I checked out the git history to this PR #3743
Here .suicide was replaced by .exitedAfterDisconnect but the truthy/falsy to true/false was retained and documentation was updated too but not anything about false. truth/false seems suicide behaviour.

Yeah, it was an improvement over .suicide
IMO docs and the code both need an update, it should be true or false, and the change reflected in the docs. If you approve I can create a PR for the fix of this.

@sam-github
Copy link
Contributor

@nimit95 Please PR a fix, thanks.

nimit95 added a commit to nimit95/node that referenced this issue Jul 26, 2019
@nimit95
cluster: default value of exitedAfterDisconnect changed to false
e9a3db6 
The defaul value of worker.exitedAfterDisconnect is undefined. If the
worker dies on its own the value is changed to false and doesn't
remains undefined. While the documentation just mentions 'Set by
calling .kill() or .disconnect(). Until then, it is undefined.'.
Making the default value as false in worker.js

Fixes: nodejs#28837
Refs: nodejs#3743
nimit95 added a commit to nimit95/node that referenced this issue Jul 26, 2019
@nimit95
doc: changing the documentation to reflect the change
830e98a 
Fixed the documentaion to reflect the changes in the default value
of worker.exitedAfterDisconnect

Fixes: nodejs#28837
Refs: nodejs#3743
nimit95 added a commit to nimit95/node that referenced this issue Jul 26, 2019
@nimit95
test: test for the exitedAfterDisconnect default value
b2f60cb 
The test checka for the exit code of the worker. cluster on exit
must be called. And the value for exitedAfterDisconnect should be false

Fixes: nodejs#28837
Refs: nodejs#3743
nimit95 added a commit to nimit95/node that referenced this issue Jul 26, 2019
@nimit95
test: fixed test-cluster-worker-constructor.js for default value
7d11da2 
fixed the test that checked for default value undefined to false and renamed the test that checks for the value exitedAfterDisconnect if the worker
exits on its own.

Fixes: nodejs#28837
Refs: nodejs#3743
nimit95 added a commit to nimit95/node that referenced this issue Sep 2, 2019
@nimit95
doc: exitedAfterDisconnect value can be false
4ae3d75 
Fixed the documentation to reflect the changes in the default value
of worker.exitedAfterDisconnect

Fixes: nodejs#28837
Refs: nodejs#3743
@nimit95 nimit95 mentioned this issue Sep 2, 2019
Closed
4 tasks
@danbev danbev closed this as completed in 695e819 Sep 9, 2019
targos pushed a commit that referenced this issue Sep 20, 2019
@nimit95 @targos
doc: exitedAfterDisconnect value can be false
5d4655c 
Fixed the documentation to reflect the changes in the default value
of worker.exitedAfterDisconnect

PR-URL: #29404
Fixes: #28837
Refs: #3743
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
BridgeAR pushed a commit that referenced this issue Sep 25, 2019
@nimit95 @BridgeAR
doc: exitedAfterDisconnect value can be false
9e63f91 
Fixed the documentation to reflect the changes in the default value
of worker.exitedAfterDisconnect

PR-URL: #29404
Fixes: #28837
Refs: #3743
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

doc: exitedAfterDisconnect value can be false nimit95/node
2 participants
@sam-github @nimit95