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

cluster: wait for all servers closing before disconnect #1400

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions lib/cluster.js
Original file line number Diff line number Diff line change
Expand Up @@ -625,12 +625,26 @@ function workerInit() {

Worker.prototype.disconnect = function() {
this.suicide = true;
var waitingHandles = 0;

function checkRemainingHandles() {
waitingHandles--;
if (waitingHandles === 0) {
process.disconnect();
}
}

for (var key in handles) {
var handle = handles[key];
delete handles[key];
handle.close();
waitingHandles++;
handle.owner.close(checkRemainingHandles);
}
process.disconnect();

if (waitingHandles === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this block necessary? Won't checkRemainingHandles itself handle this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What if we have no any handles yet?

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool. Thanks for clarifying :-)

process.disconnect();
}

};

Worker.prototype.destroy = function() {
Expand Down
63 changes: 63 additions & 0 deletions test/parallel/test-cluster-worker-wait-server-close.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
var common = require('../common');
Copy link
Contributor

Choose a reason for hiding this comment

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

lint will want a 'use strict'; here.

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

if (cluster.isWorker) {
net.createServer(function(socket) {
// Wait for any data, then close connection
socket.on('data', socket.end.bind(socket));
}).listen(common.PORT, common.localhostIPv4);
} else if (cluster.isMaster) {

var connectionDone;
var checks = {
disconnectedOnClientsEnd: false,
workerDied: false
};

// helper function to check if a process is alive
var alive = function(pid) {
try {
process.kill(pid, 0);
return true;
} catch (e) {
return false;
}
};

// start worker
var worker = cluster.fork();

// Disconnect worker when it is ready
worker.once('listening', function() {
net.createConnection(common.PORT, common.localhostIPv4, function() {
var socket = this;
setTimeout(function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This outer setTimeout is unnecessary. Now that the tests run in parallel, perhaps it does no harm, but I would remove it.

worker.disconnect();
setTimeout(function() {
socket.write('.');
Copy link
Contributor

Choose a reason for hiding this comment

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

The test is generally nicely commented, thanks! I'd suggest changing '.' to 'END', to more clearly indicate the purpose - I puzzled over why writing to a socket would cause it to become closed until I remembered the client was doing the close on receive.

connectionDone = true;
}, 1000);
}, 1000);
});
});

// Check worker events and properties
worker.once('disconnect', function() {
checks.disconnectedOnClientsEnd = connectionDone;
});

// Check that the worker died
worker.once('exit', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test is a no-op. It confirms what the child_process exit event does not occur until the child process has exited. It could be that breaks some day, but it will be a bug in child_process, unrelated to cluster, and wholly unrelated to what this test is looking at.

checks.workerDied = !alive(worker.process.pid);
process.nextTick(function() {
process.exit(0);
});
});

process.once('exit', function() {
assert.ok(checks.disconnectedOnClientsEnd, 'The worker disconnected before all clients are ended');
Copy link
Contributor

Choose a reason for hiding this comment

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

run make lint, this line is too long.

assert.ok(checks.workerDied, 'The worker did not die');
});
}