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.

Sign up for GitHub

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

Jump to bottom

cluster: return worker reference from disconnect() #10019

Closed
wants to merge 3 commits into from
Closed

cluster: return worker reference from disconnect() #10019

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7ef5107
cluster: return worker reference from disconnect()
stv8 Dec 1, 2016
a0af33c
cluster: Update markdown
stv8 Dec 5, 2016
14f58fd
cluster: add test for worker
stv8 Dec 15, 2016
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
2 changes: 2 additions & 0 deletions doc/api/cluster.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,8 @@ It is not emitted in the worker.
added: v0.7.7
-->

* Returns: {Worker} A reference to `worker`.

In a worker, this function will close all servers, wait for the `'close'` event on
those servers, and then disconnect the IPC channel.

Expand Down
2 changes: 2 additions & 0 deletions lib/cluster.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,6 +430,7 @@ function masterInit() {
send(this, { act: 'disconnect' });
removeHandlesForWorker(this);
removeWorker(this);
return this;
};

Worker.prototype.destroy = function(signo) {
Expand Down Expand Up @@ -687,6 +688,7 @@ function workerInit() {

Worker.prototype.disconnect = function() {
_disconnect.call(this);
return this;
};

Worker.prototype.destroy = function() {
Expand Down
4 changes: 3 additions & 1 deletion test/parallel/test-cluster-worker-destroy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
*/

const common = require('../common');
const assert = require('assert');
var cluster = require('cluster');
var worker1, worker2;

Expand All @@ -26,7 +27,8 @@ if (cluster.isMaster) {
cluster.worker.destroy();
});

cluster.worker.disconnect();
const w = cluster.worker.disconnect();
assert.strictEqual(w, cluster.worker, 'did not return a reference');
} else {
// Call destroy when worker is not disconnected yet
cluster.worker.destroy();
Expand Down
3 changes: 2 additions & 1 deletion test/parallel/test-cluster-worker-disconnect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ if (cluster.isWorker) {

// Disconnect worker when it is ready
worker.once('listening', common.mustCall(() => {
worker.disconnect();
const w = worker.disconnect();
assert.strictEqual(worker, w, 'did not return a reference');
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is needed here, I think it's best left to actual tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mscdex are you suggesting that it doesn't need to be tested or to pull it out into its own test?

Copy link
Contributor

Choose a reason for hiding this comment

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

You've already made changes to the test file, that's good enough IMHO. I believe these changes here can be removed.

Copy link
Contributor

@mscdex mscdex Dec 1, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Oops, sorry, disregard my comments for this file. I did not see it is a test file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries :) Do you think the assertion is only needed in one test file instead of two?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it matters either way, as long as there's at least one check for it.

}));

// Check cluster events
Expand Down
3 changes: 2 additions & 1 deletion test/parallel/test-cluster-worker-init.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ if (cluster.isMaster) {

worker.on('message', common.mustCall((message) => {
assert.strictEqual(message, true, 'did not receive expected message');
worker.disconnect();
const w = worker.disconnect();
assert.strictEqual(worker, w, 'did not return a reference');
}));

worker.on('online', () => {
Expand Down