cluster: resource leak when using shared handles and worker exits before it disconnects

[misterdjules]

The following code:

var cluster = require('cluster');
var net = require('net');

if (cluster.isMaster) {
  var worker = cluster.fork().on('online', function workerOnline() {
    worker.disconnect();
  });
} else {
  net.createServer(function(client) {}).listen(8000);
}

When run like following:

NODE_CLUSTER_SCHED_POLICY=none ./node ~/dev/test-cluster-tcp-close.js

asserts with the following error:

assert.js:98
  throw new assert.AssertionError({
        ^
AssertionError: Resource leak detected.
    at removeWorker (cluster.js:346:9)
    at ChildProcess.<anonymous> (cluster.js:366:34)
    at ChildProcess.g (events.js:199:16)
    at ChildProcess.emit (events.js:110:17)
    at Process.ChildProcess._handle.onexit (child_process.js:1057:12)

Tested with node built from the tip of v0.12 on MacOS X.

The problem is that the exit event's handler is called before the disconnect event handler, but the Worker.prototype.isConnected method returns false since worker.process.connected has already been set to false.

So we haven't had the time to remove the handle before removing the worker, and thus we assert.

My recommendation would be to change the Worker.prototype.isConnected method to an internal hasDisconnected method that returns true only after the disconnect event handler has run.

EDIT: We could maybe just add an internal hasDisconnected method instead of replacing the Worker.prototype.isConnected method.

[misterdjules]

@sam-github Putting you in the loop, since you know the cluster module very well. This bug is due to one of my changes (90d1147), but if you have some time to fix it, please feel free to do it. Otherwise I'll get to that later.

[misterdjules]

@sam-github Also, I think that currently our tests for the cluster module always run with the default scheduling policy. I would like to add the ability to run all cluster tests once for each policy that we have (currently only two, shared and round-robin). If that makes sense, I suggest creating a separate issue for that. What do you think?

[sam-github]

very well

"a little". And I haven't found time to fix bugs I already know about, much less new ones. :-(

For tests, your proposal seems very reasonable.

[jshkurti]

#9409

[cjihrig]

Closing. Appears to be fixed in nodejs/node.