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 test-cluster-worker-isdead #3954

Closed
wants to merge 1 commit into from

Conversation

santigimeno
Copy link
Member

It tries to fix #3886

@JungMinu
Copy link
Member

would you please squash the commits?

Check if the worker 'isDead' instead of 'isConnected' as the
'disconnect' event is not guaranteed to be received before the
'exit' event.
Remove the 'net' dependency as it is not used.
@santigimeno
Copy link
Member Author

Done

@targos
Copy link
Member

targos commented Nov 21, 2015

@JungMinu
Copy link
Member

CI is not green with node-test-binary-windows, but it's unrelated.

@Fishrock123
Copy link
Contributor

Better flakiness stress-test: https://ci.nodejs.org/job/node-stress-single-test/25/

Trying on freebsd102-64 since that's where I originally picked up the failure.

@Fishrock123 Fishrock123 added the test Issues and PRs related to the tests. label Nov 21, 2015
@JungMinu
Copy link
Member

@Fishrock123 CI is happy with new stress-test, LGTM

@mscdex mscdex added the cluster Issues and PRs related to the cluster subsystem. label Nov 21, 2015
@cjihrig
Copy link
Contributor

cjihrig commented Nov 23, 2015

The change LGTM, but I wonder why !worker.isConnected() is flaky and worker.isDead() is not. I'd think that isConnected() would always be false if the worker is dead.

@cjihrig
Copy link
Contributor

cjihrig commented Nov 23, 2015

Actually the flakiness is probably due the inability to determine the order between the disconnect and exit events. So, LGTM

@JungMinu
Copy link
Member

JungMinu commented Dec 5, 2015

@cjihrig @Fishrock123 May I land this? Thank you

@cjihrig
Copy link
Contributor

cjihrig commented Dec 5, 2015

@JungMinu yes, this should be good to go.

JungMinu pushed a commit that referenced this pull request Dec 5, 2015
Check if the worker 'isDead' instead of 'isConnected' as the
'disconnect' event is not guaranteed to be received before the
'exit' event.
Remove the 'net' dependency as it is not used.

PR-URL: #3954
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>
@JungMinu
Copy link
Member

JungMinu commented Dec 5, 2015

Thanks, landed in f8cf947

@JungMinu JungMinu closed this Dec 5, 2015
rvagg pushed a commit that referenced this pull request Dec 8, 2015
Check if the worker 'isDead' instead of 'isConnected' as the
'disconnect' event is not guaranteed to be received before the
'exit' event.
Remove the 'net' dependency as it is not used.

PR-URL: #3954
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 15, 2015
Check if the worker 'isDead' instead of 'isConnected' as the
'disconnect' event is not guaranteed to be received before the
'exit' event.
Remove the 'net' dependency as it is not used.

PR-URL: #3954
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>
@jasnell jasnell mentioned this pull request Dec 17, 2015
jasnell pushed a commit that referenced this pull request Dec 17, 2015
Check if the worker 'isDead' instead of 'isConnected' as the
'disconnect' event is not guaranteed to be received before the
'exit' event.
Remove the 'net' dependency as it is not used.

PR-URL: #3954
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>
jasnell pushed a commit that referenced this pull request Dec 23, 2015
Check if the worker 'isDead' instead of 'isConnected' as the
'disconnect' event is not guaranteed to be received before the
'exit' event.
Remove the 'net' dependency as it is not used.

PR-URL: #3954
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
Check if the worker 'isDead' instead of 'isConnected' as the
'disconnect' event is not guaranteed to be received before the
'exit' event.
Remove the 'net' dependency as it is not used.

PR-URL: nodejs#3954
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Minwoo Jung <jmwsoft@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. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate test-cluster-worker-isdead.js failure
8 participants