cluster: refactor module into multiple files #10746

cjihrig · 2017-01-11T20:47:22Z

I've always found the cluster code unpleasant to deal with. Currently, one large file contains the Worker, SharedHandle, and RoundRobinHandle structures. The same file also contains the cluster master and worker implementations, each of which modifies Worker. This makes the code tricky, because when you jump around in the code, you need to determine what context you're working in. I'm hoping to make it more readable/maintainable.

This commit splits the existing cluster module into several internal modules. More specifically, the cluster master and worker implementations are separated, and the various data structures are separated.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

cluster

mscdex · 2017-01-11T21:21:39Z

I think the triple newlines could be changed to just double newlines (one empty line between function definitions, etc. instead of two).

jasnell

Generally LGTM. Couple comments tho (consider to be optional)

jasnell · 2017-01-11T22:44:56Z

lib/internal/cluster/RoundRobinHandle.js

+    this.handle.onconnection = (err, handle) => this.distribute(err, handle);
+    this.server._handle = null;
+    this.server = null;
+  });


While we're at it, perhaps consider eliminating the closure by moving these handlers into top level functions. Doing so should result in at least some performance improvement overall.

If it was a function, how would it get access to this? Wouldn't it have to be bound, which has perf negatives?

jasnell · 2017-01-11T22:46:00Z

lib/internal/cluster/Worker.js

+      'worker.suicide is deprecated. Please use worker.exitedAfterDisconnect.'),
+    set: internalUtil.deprecate(
+      (val) => { this.exitedAfterDisconnect = val; },
+      'worker.suicide is deprecated. Please use worker.exitedAfterDisconnect.'),


The deprecation message could be made into a const and reused.

Trott · 2017-01-11T23:30:15Z

lib/internal/cluster/master.js

+};
+
+
+function onmessage(message, handle) {


Nit: While we're refactoring all this code, maybe remove handle as an argument here? It's not used in the function.

cjihrig · 2017-01-12T17:07:18Z

Switched from two blank lines to one blank line between functions.

@mscdex I have two questions for you:

Are we in the clear to use bind() for things like cluster: refactor module into multiple files #10746 (comment)? If so, it might be good to move to a separate commit, so it doesn't get backported.
Is there a performance hit for removing functions arguments that may be provided by the caller, but not used, such as in cluster: refactor module into multiple files #10746 (comment).

mscdex · 2017-01-12T17:14:05Z

@cjihrig

I believe so, but I would do it as a separate commit/PR for easier backporting of this PR.
I haven't tested that, so I don't know offhand. I'd probably err on the side caution and leave it the way it was until we know for sure.

cjihrig · 2017-01-12T17:17:39Z

OK, thanks! That's what I was thinking on both points, but wanted to check.

CI: https://ci.nodejs.org/job/node-test-pull-request/5824/

sam-github

Couple small suggestions, and I don't think we should be using mixed case filenames, but basically LGTM.

sam-github · 2017-01-12T17:15:54Z

lib/internal/cluster/RoundRobinHandle.js

+    this.handle.onconnection = (err, handle) => this.distribute(err, handle);
+    this.server._handle = null;
+    this.server = null;
+  });


If it was a function, how would it get access to this? Wouldn't it have to be bound, which has perf negatives?

sam-github · 2017-01-12T17:18:46Z

lib/internal/cluster/RoundRobinHandle.js

+  if (getOwnPropertyNames(this.all).length !== 0)
+    return false;
+
+  for (var handle; handle = this.handles.shift(); handle.close());


I find this kindof for construction hard to read, could you put the empty statement, the ;, on the next line?

sam-github · 2017-01-12T17:22:18Z

lib/internal/cluster/RoundRobinHandle.js

@@ -0,0 +1,113 @@
+'use strict';


file-naming convention in lib/ is is snake_case.js, AFAICT.

I used https://github.com/nodejs/node/blob/master/lib/internal/streams/BufferList.js as a reference for naming files that hold a single data type.

I think that BufferList is an unfortunate outlier. lib/internal/streams/lazy_transform.js is a better model to follow.

sam-github · 2017-01-12T17:23:20Z

lib/internal/cluster/RoundRobinHandle.js

+'use strict';
+const assert = require('assert');
+const net = require('net');
+const { sendHelper } = require('internal/cluster/utils');


sort requires?

I'm all for sorting requires. Do we sort by the variable name on the left hand side, or the string inside of require()? What about internal modules? Should they be grouped differently?

Lexical sort (in ASCII order/posix "C" locale) on the statements (which ends up being by name of variable, but variable name usually maps 1:1 to the require name, es6 requires aside) (:sort in vim, pipe block through !sort, whatever your editor likes). No grouping, too complicated to document and reproduce.

sam-github · 2017-01-12T17:26:10Z

lib/internal/cluster/RoundRobinHandle.js

+}
+
+RoundRobinHandle.prototype.add = function(worker, send) {
+  assert(worker.id in this.all === false);


Why not use ! operator instead of comparison against false? As is done in

+ if (!(this instanceof Worker)) + return new Worker(options);

sam-github · 2017-01-12T17:29:29Z

lib/internal/cluster/child.js

@@ -0,0 +1,224 @@
+'use strict';
+const assert = require('assert');


sort requires?

sam-github · 2017-01-12T17:32:49Z

lib/internal/cluster/master.js

+
+  cluster.settings = settings;
+
+  if (initialized === true)


if (initialized) ?

I think a strict comparison against a boolean is faster. This was also the existing code.

Some of the asserts don't do direct comparisons. but its existing, so NBD. I more noticed the inconsistent usage, sometimes direct comparisons, sometimes not. But again, pre-existing, so its OK.

cjihrig · 2017-01-13T16:03:16Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/5848/

Looks like an unrelated failure, so trying again: https://ci.nodejs.org/job/node-test-pull-request/5850/

This commit splits the existing cluster module into several internal modules. More specifically, the cluster master and worker implementations are separated, and the various data structures are separated. PR-URL: nodejs#10746 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sam Roberts <[email protected]>

This commit splits the existing cluster module into several internal modules. More specifically, the cluster master and worker implementations are separated, and the various data structures are separated. PR-URL: #10746 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sam Roberts <[email protected]>

This commit splits the existing cluster module into several internal modules. More specifically, the cluster master and worker implementations are separated, and the various data structures are separated. PR-URL: nodejs#10746 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sam Roberts <[email protected]>

jasnell · 2017-03-08T21:01:01Z

@nodejs/lts @cjihrig ... trying to decide if we should backport this to v6 or not. Recommending that we skip backporting to v4

jasnell · 2017-03-08T21:01:22Z

If we do backport to v6, a backport PR will be required.

nodejs-github-bot added build Issues and PRs related to build files or the CI. dont-land-on-v7.x cluster Issues and PRs related to the cluster subsystem. labels Jan 11, 2017

mscdex removed the dont-land-on-v7.x label Jan 11, 2017

jasnell approved these changes Jan 11, 2017

View reviewed changes

Trott reviewed Jan 11, 2017

View reviewed changes

cjihrig force-pushed the cluster_refactor branch from 6b9bd1a to 425162e Compare January 12, 2017 17:04

nodejs-github-bot added the dont-land-on-v7.x label Jan 12, 2017

sam-github approved these changes Jan 12, 2017

View reviewed changes

cjihrig force-pushed the cluster_refactor branch from 425162e to b032fef Compare January 13, 2017 16:01

nodejs-github-bot added the dont-land-on-v7.x label Jan 13, 2017

cjihrig force-pushed the cluster_refactor branch from b032fef to 2f885e7 Compare January 13, 2017 22:06

nodejs-github-bot added the dont-land-on-v7.x label Jan 13, 2017

cjihrig merged commit 2f885e7 into nodejs:master Jan 13, 2017

cjihrig deleted the cluster_refactor branch January 13, 2017 22:08

targos removed the dont-land-on-v7.x label Jan 28, 2017

italoacasas mentioned this pull request Jan 29, 2017

v7.5.0 proposal #11062

Merged

jasnell added the dont-land-on-v4.x label Mar 8, 2017

MylesBorins added the backport-to-v6.x label Apr 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cluster: refactor module into multiple files #10746

cluster: refactor module into multiple files #10746

cjihrig commented Jan 11, 2017

mscdex commented Jan 11, 2017

jasnell left a comment

jasnell Jan 11, 2017

sam-github Jan 12, 2017

jasnell Jan 11, 2017

Trott Jan 11, 2017

cjihrig commented Jan 12, 2017

mscdex commented Jan 12, 2017 •

edited

Loading

cjihrig commented Jan 12, 2017

sam-github left a comment

sam-github Jan 12, 2017

sam-github Jan 12, 2017

sam-github Jan 12, 2017

cjihrig Jan 12, 2017

sam-github Jan 12, 2017

sam-github Jan 12, 2017

cjihrig Jan 12, 2017

sam-github Jan 12, 2017

sam-github Jan 12, 2017

sam-github Jan 12, 2017

sam-github Jan 12, 2017

cjihrig Jan 12, 2017

sam-github Jan 12, 2017

cjihrig commented Jan 13, 2017 •

edited

Loading

jasnell commented Mar 8, 2017

jasnell commented Mar 8, 2017

		@@ -0,0 +1,224 @@
		'use strict';
		const assert = require('assert');

cluster: refactor module into multiple files #10746

cluster: refactor module into multiple files #10746

Conversation

cjihrig commented Jan 11, 2017

Checklist

Affected core subsystem(s)

mscdex commented Jan 11, 2017

jasnell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjihrig commented Jan 12, 2017

mscdex commented Jan 12, 2017 • edited Loading

cjihrig commented Jan 12, 2017

sam-github left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjihrig commented Jan 13, 2017 • edited Loading

jasnell commented Mar 8, 2017

jasnell commented Mar 8, 2017

mscdex commented Jan 12, 2017 •

edited

Loading

cjihrig commented Jan 13, 2017 •

edited

Loading