cluster: allow multiple calls to setupMaster()

Only attributes of 'cluster.settings' will be modified after the first
call, leaving all other cluster initialization alone. Each call that
includes a 'settings' argument triggers a 'setup' event to be emitted.

Instead of each call resetting all values to their defaults, use the
current settings (if any) as the default. This retains setupMaster's
support how cluster.fork() uses setupMaster() to ensure
cluster.settings has been populated.

Update example in docs to use current node coding style and include
an example of progressive configuration.

Signed-off-by: Fedor Indutny <fedor@indutny.com>

diff --git a/doc/api/cluster.markdown b/doc/api/cluster.markdown
index 9a6e51f..e4c2111 100644 (file)
--- a/doc/api/cluster.markdown
+++ b/doc/api/cluster.markdown
@@ -250,7 +250,7 @@ See [child_process event: 'exit'](child_process.html#child_process_event_exit).
 
 ## Event: 'setup'
 
-Emitted the first time that `.setupMaster()` is called.
+Emitted every time `.setupMaster()` is called with a `settings` argument.
 
 ## cluster.setupMaster([settings])
 
@@ -266,23 +266,26 @@ the settings will be present in `cluster.settings`.
 
 Note that:
 
-* Only the first call to `.setupMaster()` has any effect, subsequent calls are
-  ignored
-* That because of the above, the *only* attribute of a worker that may be
-  customized per-worker is the `env` passed to `.fork()`
-* `.fork()` calls `.setupMaster()` internally to establish the defaults, so to
-  have any effect, `.setupMaster()` must be called *before* any calls to
-  `.fork()`
+* any settings changes only affect future calls to `.fork()` and have no
+  effect on workers that are already running
+* The *only* attribute of a worker that cannot be set via `.setupMaster()` is
+  the `env` passed to `.fork()`
+* the defaults above apply to the first call only, the defaults for later
+  calls is the current value at the time of `cluster.setupMaster()` is called
 
 Example:
 
-    var cluster = require("cluster");
+    var cluster = require('cluster');
+    cluster.setupMaster({
+      exec: 'worker.js',
+      args: ['--use', 'https'],
+      silent: true
+    });
+    cluster.fork(); // https worker
     cluster.setupMaster({
-      exec : "worker.js",
-      args : ["--use", "https"],
-      silent : true
+      args: ['--use', 'http']
     });
-    cluster.fork();
+    cluster.fork(); // http worker
 
 This can only be called from the master process.
 

diff --git a/lib/cluster.js b/lib/cluster.js
index 269040a..e5d3533 100644 (file)
--- a/lib/cluster.js
+++ b/lib/cluster.js
@@ -239,14 +239,13 @@ function masterInit() {
 
   var initialized = false;
   cluster.setupMaster = function(options) {
-    if (initialized === true) return;
-    initialized = true;
     var settings = {
       args: process.argv.slice(2),
       exec: process.argv[1],
       execArgv: process.execArgv,
       silent: false
     };
+    settings = util._extend(settings, cluster.settings);
     settings = util._extend(settings, options || {});
     // Tell V8 to write profile data for each process to a separate file.
     // Without --logfile=v8-%p.log, everything ends up in a single, unusable
@@ -257,10 +256,15 @@ function masterInit() {
     {
       settings.execArgv = settings.execArgv.concat(['--logfile=v8-%p.log']);
     }
+    cluster.settings = settings;
+    if (initialized === true)
+      return options && process.nextTick(function() {
+        cluster.emit('setup');
+      });
+    initialized = true;
     schedulingPolicy = cluster.schedulingPolicy;  // Freeze policy.
     assert(schedulingPolicy === SCHED_NONE || schedulingPolicy === SCHED_RR,
            'Bad cluster.schedulingPolicy: ' + schedulingPolicy);
-    cluster.settings = settings;
 
     process.on('internalMessage', function(message) {
       if (message.cmd !== 'NODE_DEBUG_ENABLED') return;

diff --git a/test/simple/test-cluster-setup-master-cumulative.js b/test/simple/test-cluster-setup-master-cumulative.js
new file mode 100644 (file)
index 0000000..2f69cd3
--- /dev/null
+++ b/test/simple/test-cluster-setup-master-cumulative.js
@@ -0,0 +1,62 @@
+// Copyright Joyent, Inc. and other Node contributors.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a
+// copy of this software and associated documentation files (the
+// "Software"), to deal in the Software without restriction, including
+// without limitation the rights to use, copy, modify, merge, publish,
+// distribute, sublicense, and/or sell copies of the Software, and to permit
+// persons to whom the Software is furnished to do so, subject to the
+// following conditions:
+//
+// The above copyright notice and this permission notice shall be included
+// in all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
+// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
+// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+// USE OR OTHER DEALINGS IN THE SOFTWARE.
+
+
+var common = require('../common');
+var assert = require('assert');
+var cluster = require('cluster');
+
+assert(cluster.isMaster);
+
+assert.deepEqual(cluster.settings, {},
+                 'cluster.settings should not be initialized until needed');
+
+cluster.setupMaster();
+assert.deepEqual(cluster.settings, {
+  args: process.argv.slice(2),
+  exec: process.argv[1],
+  execArgv: process.execArgv,
+  silent: false,
+});
+console.log('ok sets defaults');
+
+cluster.setupMaster({ exec: 'overridden' });
+assert.strictEqual(cluster.settings.exec, 'overridden');
+console.log('ok overrids defaults');
+
+cluster.setupMaster({ args: ['foo', 'bar'] });
+assert.strictEqual(cluster.settings.exec, 'overridden');
+assert.deepEqual(cluster.settings.args, ['foo', 'bar']);
+
+cluster.setupMaster({ execArgv: ['baz', 'bang'] });
+assert.strictEqual(cluster.settings.exec, 'overridden');
+assert.deepEqual(cluster.settings.args, ['foo', 'bar']);
+assert.deepEqual(cluster.settings.execArgv, ['baz', 'bang']);
+console.log('ok preserves unchanged settings on repeated calls');
+
+cluster.setupMaster();
+assert.deepEqual(cluster.settings, {
+  args: ['foo', 'bar'],
+  exec: 'overridden',
+  execArgv: ['baz', 'bang'],
+  silent: false,
+});
+console.log('ok preserves current settings');

diff --git a/test/simple/test-cluster-setup-master-multiple.js b/test/simple/test-cluster-setup-master-multiple.js
new file mode 100644 (file)
index 0000000..21756b9
--- /dev/null
+++ b/test/simple/test-cluster-setup-master-multiple.js
@@ -0,0 +1,71 @@
+// Copyright Joyent, Inc. and other Node contributors.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a
+// copy of this software and associated documentation files (the
+// "Software"), to deal in the Software without restriction, including
+// without limitation the rights to use, copy, modify, merge, publish,
+// distribute, sublicense, and/or sell copies of the Software, and to permit
+// persons to whom the Software is furnished to do so, subject to the
+// following conditions:
+//
+// The above copyright notice and this permission notice shall be included
+// in all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
+// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
+// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+// USE OR OTHER DEALINGS IN THE SOFTWARE.
+
+
+var common = require('../common');
+var assert = require('assert');
+var cluster = require('cluster');
+
+assert(cluster.isMaster);
+
+// The cluster.settings object is cloned even though the current implementation
+// makes that unecessary. This is to make the test less fragile if the
+// implementation ever changes such that cluster.settings is mutated instead of
+// replaced.
+function cheapClone(obj) {
+  return JSON.parse(JSON.stringify(obj));
+}
+
+var configs = [];
+
+// Capture changes
+cluster.on('setup', function() {
+  console.log('"setup" emitted', cluster.settings);
+  configs.push(cheapClone(cluster.settings));
+});
+
+var execs = [
+  'node-next',
+  'node-next-2',
+  'node-next-3',
+];
+
+process.on('exit', function assertTests() {
+  // Tests that "setup" is emitted for every call to setupMaster
+  assert.strictEqual(configs.length, execs.length);
+
+  assert.strictEqual(configs[0].exec, execs[0]);
+  assert.strictEqual(configs[1].exec, execs[1]);
+  assert.strictEqual(configs[2].exec, execs[2]);
+});
+
+// Make changes to cluster settings
+execs.forEach(function(v, i) {
+  setTimeout(function() {
+    cluster.setupMaster({ exec: v });
+  }, i * 100);
+});
+
+// cluster emits 'setup' asynchronously, so we must stay alive long
+// enough for that to happen
+setTimeout(function() {
+  console.log('cluster setup complete');
+}, (execs.length + 1) * 100);