FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: fix flaky cluster-disconnect-race by mscdex · Pull Request #4457 · nodejs/node · GitHub

/ node Public

test: fix flaky cluster-disconnect-race - #4457

Closed
mscdex wants to merge 1 commit into
nodejs:masterfrom
mscdex:fix-win-flaky-test-cluster-disconnect-race
ConversationCommits (1)ChecksFiles changed
Closed

test: fix flaky cluster-disconnect-race#4457
mscdex wants to merge 1 commit into
nodejs:masterfrom
mscdex:fix-win-flaky-test-cluster-disconnect-race

Conversation

mscdex commented Dec 28, 2015

Copy link
Copy Markdown
Contributor

Before this commit, on single core Windows systems process.send() would cause an EPIPE because of the ordering of the IPC channel disconnect and the process.send().

Disconnecting the second worker once it receives the sent message vs. when the worker merely comes online fixes this race condition.

Fixes: #4450

mscdex added cluster Issues and PRs related to the cluster subsystem. windows Issues and PRs related to the Windows platform. test Issues and PRs related to the tests. labels Dec 28, 2015
Trott reviewed Dec 28, 2015
View reviewed changes
Comment thread test/parallel/test-cluster-disconnect-race.js Outdated

Trott Dec 28, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Maybe just worker2.on('message', ...) by itself rather than wrapped inside a worker2.on('online', ...) ?

Trott commented Dec 28, 2015

Copy link
Copy Markdown
Member

Seems to blow up a bit on ubuntu more often than not...

Mismatched <anonymous> function calls. Expected 1, actual 2.
    at Object.<anonymous> (/home/vagrant/node/test/parallel/trott.js:24:29)
    at Module._compile (module.js:408:26)
    at Object.Module._extensions..js (module.js:415:10)
    at Module.load (module.js:354:32)
    at Function.Module._load (module.js:311:12)
    at Function.Module.runMain (module.js:440:10)
    at startup (node.js:141:18)
    at node.js:997:3

mscdex commented Dec 28, 2015

Copy link
Copy Markdown
Contributor Author

Yeah I'm seeing that now. Looking into it...

mscdex force-pushed the fix-win-flaky-test-cluster-disconnect-race branch from a6f4f4f to dd5f204 Compare December 29, 2015 00:53

mscdex commented Dec 29, 2015

Copy link
Copy Markdown
Contributor Author

Ok that should be fixed now.

mscdex commented Dec 29, 2015

Copy link
Copy Markdown
Contributor Author

Stress test: https://ci.nodejs.org/job/node-stress-single-test/250/nodes=win2012r2/

mscdex commented Dec 29, 2015

Copy link
Copy Markdown
Contributor Author

Stress test is green! \o/

General CI run: https://ci.nodejs.org/job/node-test-pull-request/1101/

Trott commented Dec 29, 2015

Copy link
Copy Markdown
Member

Unfortunately, this change means that the test no longer fails in Node 5.3.0, which means that it doesn't test the issue it was written to test anymore. (Tested on ubuntu 14.04.)

Trott commented Dec 29, 2015

Copy link
Copy Markdown
Member

I think the issue in the test that you've fixed is the source of the race condition that's supposed to be tested here. It's a feature and not a bug. Maybe the thing to do is swallow EPIPE on Windows.

Another perhaps better option is to skip the test on anything that isn't Linux, since that's the only place (that I know of, anyway) where the thing (that this test detects) ever happened.

mscdex commented Dec 29, 2015

Copy link
Copy Markdown
Contributor Author

I thought about that, but swallowing EPIPE seems like a bad idea since then it would appear as though the send() was successful but it actually wasn't.

Trott commented Dec 29, 2015

Copy link
Copy Markdown
Member

Maybe just skipping the test on Windows since the test does not test the issue it is supposed to test on Windows anyway (perhaps because the issue never existed there). In Node 5.3.0, it blows up on Linux but not on Windows. It's supposed to blow up in Node 5.3.0. Either this issue never manifested itself on Windows or else it did but this test never caught it anyway. (I'm guessing the former.)

mscdex force-pushed the fix-win-flaky-test-cluster-disconnect-race branch from dd5f204 to 192c097 Compare December 29, 2015 04:50

mscdex commented Dec 29, 2015

Copy link
Copy Markdown
Contributor Author

@Trott Ok, I've modified it to just skip on Windows and left in a couple extra mustCall()s. LGTY?

Trott reviewed Dec 29, 2015
View reviewed changes
Comment thread test/parallel/test-cluster-disconnect-race.js Outdated

Trott Dec 29, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Nit: Windows rather than windows

Nit: is disabled on Windows makes it sound like we just gave up and hope to enable it one day. I'd rather convey a message that it just doesn't apply to Windows. Maybe: Skipped: This test does not apply to Windows.

mscdex Dec 29, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yeah I just copied this directly from another test that presumably was doing a check for the same reason.

Trott Dec 29, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Eh. Either way. It's a nit.

Trott commented Dec 29, 2015

Copy link
Copy Markdown
Member

A couple of nits that you can take or leave (because, hey, that's what a nit is). Beyond that, yeah, totally LGTM if CI is happy and as long as it still fails on Linux on Node 5.3.0.

test: fix flaky cluster-disconnect-race
d2d7c94 
On single core Windows systems, process.send() would cause an
EPIPE because of the ordering of the IPC channel disconnect and
the process.send().

The test was originally only relevant for non-Windows platforms,
so this commit merely skips the test on Windows.

Fixes: nodejs#4450
PR-URL: nodejs#4457
mscdex force-pushed the fix-win-flaky-test-cluster-disconnect-race branch from 192c097 to d2d7c94 Compare December 29, 2015 05:11

mscdex commented Dec 29, 2015

Copy link
Copy Markdown
Contributor Author

Wording fixed. One last CI run: https://ci.nodejs.org/job/node-test-pull-request/1103/

Trott commented Dec 29, 2015

Copy link
Copy Markdown
Member

Behaves as expected with Node 5.3.0 on Ubuntu 14.04.

mscdex added a commit that referenced this pull request Dec 29, 2015
test: fix flaky cluster-disconnect-race
fd551c3 
On single core Windows systems, process.send() would cause an
EPIPE because of the ordering of the IPC channel disconnect and
the process.send().

The test was originally only relevant for non-Windows platforms,
so this commit merely skips the test on Windows.

Fixes: #4450
PR-URL: #4457
Reviewed-By: Rich Trott <rtrott@gmail.com>

mscdex commented Dec 29, 2015

Copy link
Copy Markdown
Contributor Author

Landed in fd551c3.

mscdex closed this Dec 29, 2015
mscdex deleted the fix-win-flaky-test-cluster-disconnect-race branch December 29, 2015 05:48
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Jan 6, 2016
test: fix flaky cluster-disconnect-race
e72112f 
On single core Windows systems, process.send() would cause an
EPIPE because of the ordering of the IPC channel disconnect and
the process.send().

The test was originally only relevant for non-Windows platforms,
so this commit merely skips the test on Windows.

Fixes: nodejs#4450
PR-URL: nodejs#4457
Reviewed-By: Rich Trott <rtrott@gmail.com>
Trott mentioned this pull request Jan 14, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Jan 14, 2016
test: fix flaky cluster-disconnect-race
cf7ebd3 
On single core Windows systems, process.send() would cause an
EPIPE because of the ordering of the IPC channel disconnect and
the process.send().

The test was originally only relevant for non-Windows platforms,
so this commit merely skips the test on Windows.

Fixes: #4450
PR-URL: #4457
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins mentioned this pull request Jan 15, 2016
Closed
MylesBorins pushed a commit that referenced this pull request Jan 19, 2016
test: fix flaky cluster-disconnect-race
c4eeb88 
On single core Windows systems, process.send() would cause an
EPIPE because of the ordering of the IPC channel disconnect and
the process.send().

The test was originally only relevant for non-Windows platforms,
so this commit merely skips the test on Windows.

Fixes: #4450
PR-URL: #4457
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins mentioned this pull request Jan 19, 2016
Merged
This was referenced Mar 8, 2016
Closed
Closed
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
test: fix flaky cluster-disconnect-race
6f0fe6a 
On single core Windows systems, process.send() would cause an
EPIPE because of the ordering of the IPC channel disconnect and
the process.send().

The test was originally only relevant for non-Windows platforms,
so this commit merely skips the test on Windows.

Fixes: nodejs#4450
PR-URL: nodejs#4457
Reviewed-By: Rich Trott <rtrott@gmail.com>
This was referenced Oct 16, 2020
Merged
Open
Open
This was referenced Nov 18, 2020
Merged
Open
Open
leonardoadame mentioned this pull request May 16, 2024
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

cluster Issues and PRs related to the cluster subsystem. test Issues and PRs related to the tests. windows Issues and PRs related to the Windows platform.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Investigate flaky test-cluster-disconnect-race

3 participants

Back | FazBrowse Home | New Git URL