test: fix flaky cluster-disconnect-race #4457

mscdex · 2015-12-28T22:00:40Z

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

Trott · 2015-12-28T23:19:08Z

test/parallel/test-cluster-disconnect-race.js

@@ -16,12 +16,14 @@ if (cluster.isMaster) {
  worker1.on('message', common.mustCall(function() {
    worker2 = cluster.fork();
    worker1.disconnect();
-    worker2.on('online', common.mustCall(worker2.disconnect));
+    worker2.on('online', common.mustCall(function() {


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

Trott · 2015-12-28T23:32:39Z

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 · 2015-12-28T23:38:35Z

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

mscdex · 2015-12-29T00:54:20Z

Ok that should be fixed now.

mscdex · 2015-12-29T01:02:00Z

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

mscdex · 2015-12-29T02:55:05Z

Stress test is green! \o/

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

Trott · 2015-12-29T04:12:55Z

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 · 2015-12-29T04:17:09Z

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 · 2015-12-29T04:25:52Z

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 · 2015-12-29T04:37:40Z

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 · 2015-12-29T04:51:35Z

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

Trott · 2015-12-29T04:59:22Z

test/parallel/test-cluster-disconnect-race.js

@@ -7,6 +7,12 @@ const common = require('../common');
 const assert = require('assert');
 const net = require('net');
 const cluster = require('cluster');
+
+if (common.isWindows) {
+  console.log('1..0 # Skipped: This test is disabled on windows.');


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.

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

Eh. Either way. It's a nit.

Trott · 2015-12-29T05:00:54Z

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.

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 · 2015-12-29T05:12:59Z

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

Trott · 2015-12-29T05:27:01Z

Behaves as expected with Node 5.3.0 on Ubuntu 14.04.

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 · 2015-12-29T05:48:24Z

Landed in 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: nodejs#4450 PR-URL: nodejs#4457 Reviewed-By: Rich Trott <rtrott@gmail.com>

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>

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>

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

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

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

Trott reviewed Dec 29, 2015
View reviewed changes

mscdex force-pushed the fix-win-flaky-test-cluster-disconnect-race branch from 192c097 to d2d7c94 Compare December 29, 2015 05:11

mscdex closed this Dec 29, 2015

mscdex deleted the fix-win-flaky-test-cluster-disconnect-race branch December 29, 2015 05:48

Trott mentioned this pull request Jan 14, 2016

LTS: backport no-unused-vars to v4.x-staging #4688

Merged

MylesBorins added the lts-watch-v4.x label Jan 14, 2016

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Jan 14, 2016

MylesBorins mentioned this pull request Jan 15, 2016

Audit commits not found on v4.x-staging #4698

Closed

MylesBorins mentioned this pull request Jan 19, 2016

V4.2.5 proposal #4768

Merged

This was referenced Mar 8, 2016

v0.12.11 cluster-disconnect-race failure on Win7 #5603

Closed

test: skip cluster-disconnect-race on Windows #5621

Closed

This was referenced Oct 16, 2020

[Snyk] Security upgrade mocha from 7.2.0 to 8.2.0 turkdevops/node#22

Merged

[Snyk] Security upgrade mocha from 6.2.3 to 8.2.0 adamlaska/node#40

Open

[Snyk] Security upgrade mocha from 6.2.3 to 8.2.0 turkdevops/node#23

Open

This was referenced Nov 18, 2020

[Snyk] Security upgrade mocha from 7.2.0 to 8.2.0 turkdevops/node#32

Merged

[Snyk] Security upgrade mocha from 6.2.3 to 8.2.0 adamlaska/node#45

Open

[Snyk] Security upgrade mocha from 6.2.3 to 8.2.0 turkdevops/node#33

Open

leonardoadame mentioned this pull request May 16, 2024

[Snyk] Fix for 4 vulnerabilities leonardoadame/node#966

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: fix flaky cluster-disconnect-race #4457

test: fix flaky cluster-disconnect-race #4457

mscdex commented Dec 28, 2015

Trott Dec 28, 2015

Trott commented Dec 28, 2015

mscdex commented Dec 28, 2015

mscdex commented Dec 29, 2015

mscdex commented Dec 29, 2015

mscdex commented Dec 29, 2015

Trott commented Dec 29, 2015

Trott commented Dec 29, 2015

mscdex commented Dec 29, 2015

Trott commented Dec 29, 2015

mscdex commented Dec 29, 2015

Trott Dec 29, 2015

mscdex Dec 29, 2015

Trott Dec 29, 2015

Trott commented Dec 29, 2015

mscdex commented Dec 29, 2015

Trott commented Dec 29, 2015

mscdex commented Dec 29, 2015

test: fix flaky cluster-disconnect-race #4457

test: fix flaky cluster-disconnect-race #4457

Conversation

mscdex commented Dec 28, 2015

Trott Dec 28, 2015

Choose a reason for hiding this comment

Trott commented Dec 28, 2015

mscdex commented Dec 28, 2015

mscdex commented Dec 29, 2015

mscdex commented Dec 29, 2015

mscdex commented Dec 29, 2015

Trott commented Dec 29, 2015

Trott commented Dec 29, 2015

mscdex commented Dec 29, 2015

Trott commented Dec 29, 2015

mscdex commented Dec 29, 2015

Trott Dec 29, 2015

Choose a reason for hiding this comment

mscdex Dec 29, 2015

Choose a reason for hiding this comment

Trott Dec 29, 2015

Choose a reason for hiding this comment

Trott commented Dec 29, 2015

mscdex commented Dec 29, 2015

Trott commented Dec 29, 2015

mscdex commented Dec 29, 2015