cluster: don't assert if worker has no handles #8642

sam-github · 2014-10-29T04:50:34Z

As it says.

sam-github · 2014-11-14T19:33:19Z

@misterdjules this is a regression introduced in 0.11, trivial to repro and tiny to fix, do you have ability to get some motion on it?

sam-github · 2014-11-14T19:35:50Z

At the very least it could be tagged as a bug.

misterdjules · 2014-11-17T06:41:50Z

Thank you @sam-github for the PR! A few observations:

The dgram module is not loaded by the test, so it fails all the time.
The master also doesn't know if a worker fails (failure to listen or to get online), and in this case the test times out.
When running on Windows with the fix for 1), the test still fails with ENOTSUP:

C:\Users\jgilli\dev\node>.\Release\node.exe test\simple\test-cluster-disconnect-
unshared.js
events.js:85
      throw er; // Unhandled 'error' event
            ^
Error: write ENOTSUP
    at exports._errnoException (util.js:746:11)
    at ChildProcess.target._send (child_process.js:482:28)
    at ChildProcess.target.send (child_process.js:415:12)
    at sendHelper (cluster.js:675:8)
    at send (cluster.js:511:5)
    at cluster.js:487:7
    at SharedHandle.add (cluster.js:99:3)
    at queryServer (cluster.js:479:12)
    at Worker.onmessage (cluster.js:437:7)
    at ChildProcess.<anonymous> (cluster.js:691:8)

C:\Users\jgilli\dev\node>

Other minor suggestions:

There's a typo in the commit message: listenes -> listens.
It could be nice to add a comment in SharedHandle.prototype.remove to make why we allow for workers not to share a given handle a bit clearer. The cluster module is quite complicated to follow, and in my opinion a few more comments here and there could not hurt.

sam-github · 2014-11-17T17:58:11Z

Thanks @misterdjules, I will fix this up.

sam-github · 2014-11-18T06:08:50Z

I added a possible comment, but honestly, I think its the original assert that needed the comment, the code WITHOUT the comment is pretty clear. A worker is being removed from a handle, but what reason is there to expect every worker to listen on EVERY handle? For handles that a worker didn't listen on, no need to remove it, return.

I patched up the other small things.

Alas, as I don't have a windows node build env ATM, it'll take me some time tomorrow to get my dog-slow windows vm to grind out a build.

misterdjules · 2014-11-18T06:38:13Z

@sam-github Thanks!

Regarding the comment, I agree with you that it's actually overkill. My initial suggestion mostly came from the fact that I revisit the cluster's implementation only once in a while, and every time It takes me some time to remember how it all fits together. Sorry for that.

As for the Windows build, please let me know if you need any help with that, I have a pretty good Windows 7 VM running on my laptop.

sam-github · 2014-11-18T06:54:11Z

Actually, I think I know what the deal is. dgram isn't supported by cluster on windows, and nothing but dgram uses the shared handle on linux.... so by default there isn't a single socket type that can be used to make a portable test, though the fix applies to any use of shared handle.

I'm going to force everything to be shared and use tcp, which should reliably show both the shared handle bug,and the fix, on windows and linux.

sam-github · 2014-11-26T20:46:02Z

I spent about an hour trying to find a way to make this work on Windows, since dgram isn't supported on windows, and just made a mess. I'll try to get time to look at it again tomorrow, hopefully with all the Americans on vacation it will be quiet.

sam-github · 2014-12-04T01:30:38Z

OK, I think I found my confusion. I thought I could trigger the SharedHandle to be used on linux and Windows by disabling round-robin, but that didn't work. It appears that all socket types other than dgram are not effected because there is some extra handle close messages that occur, but dgram doesn't do that.

So, it only effects dgram, and dgram isn't supported on Windows with cluster... so I'm comfortable saying this is a non-Windows test.

sam-github · 2014-12-04T01:32:19Z

@misterdjules PTAL

misterdjules · 2014-12-05T02:53:00Z

While taking another look at it, I found that running the newly added test (test/simple/test-cluster-disconnect-unshared.js) doesn't fail (it doesn't assert) without the fix in lib/cluster.js. Shouldn't it assert without the fix?

If understand correctly, I think that we want the test to behave such as the worker that doesn't share the handle disconnects first, so that the master attempts to remove it from the handle. If the non-sharing worker disconnects after the sharing worker, then the test passes even without the fix.

I suggest maybe rewriting the fix like following:

if (cluster.isMaster) {
  var nonSharing = cluster.fork();
  nonSharing.on('online', bind);

  function bind() {
    var sharing = cluster.fork({BOUND: 'y'});
    sharing.on('listening', disconnect);
  }

  function disconnect() {
    /*
     * Make sure to disconnect the non-sharing worker first,
     * and only when it has disconnected, disconnect the rest
     * of the workers. Otherwise, the problematic behavior is
     * not triggered.
     */
    nonSharing.on('disconnect', function onNonSharedDisconnect() {
      cluster.disconnect();
    });

    nonSharing.disconnect();
  }
} else {
  if (process.env.BOUND === 'y') {
    var source = dgram.createSocket('udp4');

    source.bind(0);
  }
}

@sam-github What do you think?

misterdjules · 2014-12-05T02:53:27Z

test/simple/test-cluster-disconnect-unshared.js

+
+if (cluster.isMaster) {
+  var messages = 0;
+  var ports = {};


Are messages and ports used in this test?

misterdjules · 2014-12-05T06:45:53Z

@sam-github Also, running the test modified as mentioned above but with the fix that removes the assert in SharedHandle.prototype.remove, I get the following error:

$ ./node test/simple/test-cluster-disconnect-unshared.js 
events.js:85
      throw er; // Unhandled 'error' event
            ^
Error: channel closed
    at ChildProcess.target.send (child_process.js:413:26)
    at sendHelper (cluster.js:676:8)
    at send (cluster.js:512:5)
    at Worker.disconnect (cluster.js:419:5)
    at EventEmitter.cluster.disconnect (cluster.js:411:30)
    at Worker.onNonSharedDisconnect (/Users/JulienGilli/dev/node/test/simple/test-cluster-disconnect-unshared.js:48:15)
    at Worker.emit (events.js:104:17)
    at ChildProcess.<anonymous> (cluster.js:392:14)
    at ChildProcess.g (events.js:199:16)
    at ChildProcess.emit (events.js:104:17)
$

Here's how I fixed it locally:

diff --git a/lib/cluster.js b/lib/cluster.js
index 6f355d0..5e681f4 100644
--- a/lib/cluster.js
+++ b/lib/cluster.js
@@ -407,7 +407,8 @@ function masterInit() {
     } else {
       for (var key in workers) {
         key = workers[key];
-        cluster.workers[key].disconnect();
+        if (cluster.workers[key].isConnected())
+          cluster.workers[key].disconnect();
       }
     }
     if (cb) intercom.once('disconnect', cb);

However, I want to validate with you that the test as currently written in this PR needs to be fixed before discussing about that error.

Thank you!

sam-github · 2014-12-10T04:39:34Z

I agree, the test had a scheduling order dependency. It was always working (failing without my fix) on my machine, but I can see why that would be fluke.

I added a stripped down test for the bug exposed by disconnect before exit.

sam-github · 2014-12-10T04:49:30Z

PTAL @misterdjules

misterdjules · 2014-12-11T18:17:58Z

test/simple/test-cluster-disconnect-before-exit.js

+    // disconnected worker would error.
+    worker.on('disconnect', cluster.disconnect);
+  }
+} else {


Do we need the else block here?

misterdjules · 2014-12-11T18:21:18Z

Thanks again @sam-github! It does look good to me. One last thing, could you please elaborate on:

It appears that all socket types other than dgram are not effected because there is some extra
handle close messages that occur

? I'm not sure I understand exactly how and when the "extra handle close close messages" happen for non-dgram.

Thank you!

sam-github · 2014-12-23T21:28:34Z

@misterdjules I'm sorry, I was confused. The ordering dependency of the tests, wherein they always passed for dgram, but always failed for tcp, was confusing me. I wrote a tcp version that forces use of SharedHandle (non-round robin) and renamed the existing tests. I've confirmed the tests pass on linux and windows, and fail without my changes, on linux and windows (dgram is skipped on windows, though, because node cluster doesn't support dgram on windows).

misterdjules · 2014-12-24T02:58:28Z

@sam-github No worries, and thanks for the update. I don't know if you got that on IRC, but I'm off until January 5th, so I probably won't have time to review it until then.

misterdjules · 2015-01-09T22:18:03Z

@sam-github Did you have a chance to look at my comment? Otherwise LGTM. Depending on where we are with the merge from v0.10 to v0.12, we'll land it before or after the merge is done. I'll keep you posted.

Thanks!

misterdjules · 2015-01-09T22:48:02Z

Adding it to the 0.11.15 milestone because it's been reviewed and tested, so it should be quick and easy to land.

sam-github · 2015-01-10T01:01:36Z

Didn't see your comment, I'll drop the else.

For shared handles that do not get connection close messages (UDP/dgram is the only example of this), cluster must not assume that a port listened on by one worker is listened on by all workers. PR-URL: nodejs/node-v0.x-archive#8642 Reviewed-by: Bert Belder <bertbelder@gmail.com>

Workers that are already disconnected but not yet exited should not be disconnected, trying to do so raises exceptions. PR-URL: nodejs/node-v0.x-archive#8642 Reviewed-by: Bert Belder <bertbelder@gmail.com>

piscisaureus · 2015-01-10T03:53:16Z

lgtm, landed in io.js: nodejs/node@a80b977...092c224
Thanks Sam!

For shared handles that do not get connection close messages (UDP/dgram is the only example of this), cluster must not assume that a port listened on by one worker is listened on by all workers.

Workers that are already disconnected but not yet exited should not be disconnected, trying to do so raises exceptions.

misterdjules · 2015-01-15T00:36:00Z

@sam-github Thanks, landed in f260ef8 and 85360f0 (I switched the order of the commits as 85360f0 depends on f260ef8). I also slightly reworded the commit message for 85360f0, which in my opinion now better reflects what the change does.

Thank you very much again!

Nodejs-Jenkins added pr v0.12 labels Oct 29, 2014

sam-github mentioned this pull request Oct 29, 2014

Don't share dgram sockets that are implicitly bound #8643

Closed

misterdjules added the S-confirmed-bug label Nov 17, 2014

sam-github mentioned this pull request Nov 17, 2014

UDP datagram sockets don't seem to close properly when on a cluster.Worker #8195

Closed

misterdjules reviewed Dec 5, 2014
View reviewed changes

misterdjules reviewed Dec 11, 2014
View reviewed changes

misterdjules added the cluster label Jan 9, 2015

misterdjules added this to the 0.11.15 milestone Jan 9, 2015

sam-github added 2 commits January 12, 2015 12:07

cluster: don't assume all workers bind same ports

9305d38

For shared handles that do not get connection close messages (UDP/dgram is the only example of this), cluster must not assume that a port listened on by one worker is listened on by all workers.

cluster: cluster.disconnect() should check status

ae037ee

Workers that are already disconnected but not yet exited should not be disconnected, trying to do so raises exceptions.

misterdjules closed this Jan 15, 2015

mhart mentioned this pull request May 7, 2016

workaround for node bug joyent/node#8195 mhart/gelfling#3

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cluster: don't assert if worker has no handles #8642

cluster: don't assert if worker has no handles #8642

sam-github commented Oct 29, 2014

sam-github commented Nov 14, 2014

sam-github commented Nov 14, 2014

misterdjules commented Nov 17, 2014

sam-github commented Nov 17, 2014

sam-github commented Nov 18, 2014

misterdjules commented Nov 18, 2014

sam-github commented Nov 18, 2014

sam-github commented Nov 26, 2014

sam-github commented Dec 4, 2014

sam-github commented Dec 4, 2014

misterdjules commented Dec 5, 2014

misterdjules Dec 5, 2014

misterdjules commented Dec 5, 2014

sam-github commented Dec 10, 2014

sam-github commented Dec 10, 2014

misterdjules Dec 11, 2014

misterdjules commented Dec 11, 2014

sam-github commented Dec 23, 2014

misterdjules commented Dec 24, 2014

misterdjules commented Jan 9, 2015

misterdjules commented Jan 9, 2015

sam-github commented Jan 10, 2015

piscisaureus commented Jan 10, 2015

misterdjules commented Jan 15, 2015

cluster: don't assert if worker has no handles #8642

cluster: don't assert if worker has no handles #8642

Conversation

sam-github commented Oct 29, 2014

sam-github commented Nov 14, 2014

sam-github commented Nov 14, 2014

misterdjules commented Nov 17, 2014

sam-github commented Nov 17, 2014

sam-github commented Nov 18, 2014

misterdjules commented Nov 18, 2014

sam-github commented Nov 18, 2014

sam-github commented Nov 26, 2014

sam-github commented Dec 4, 2014

sam-github commented Dec 4, 2014

misterdjules commented Dec 5, 2014

misterdjules Dec 5, 2014

Choose a reason for hiding this comment

misterdjules commented Dec 5, 2014

sam-github commented Dec 10, 2014

sam-github commented Dec 10, 2014

misterdjules Dec 11, 2014

Choose a reason for hiding this comment

misterdjules commented Dec 11, 2014

sam-github commented Dec 23, 2014

misterdjules commented Dec 24, 2014

misterdjules commented Jan 9, 2015

misterdjules commented Jan 9, 2015

sam-github commented Jan 10, 2015

piscisaureus commented Jan 10, 2015

misterdjules commented Jan 15, 2015