inspector,cluster: fix inspect port assignment

* Adding --inspect-port with debug port, instead of parsing `execArgv`

* Export CLI debug options to `process.binding('config').debugOptions`
  (currently used only in tests)

PR-URL: https://github.com/nodejs/node/pull/13619
Refs: https://github.com/nodejs/node/pull/9659
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

6 files changed, 166 insertions, 58 deletions

diff --git a/lib/internal/cluster/master.js b/lib/internal/cluster/master.js
index 05bba01c4e..2d1e2d3097 100644
--- a/lib/internal/cluster/master.js
+++ b/lib/internal/cluster/master.js
@@ -96,26 +96,16 @@ function setupSettingsNT(settings) {
 }
 
 function createWorkerProcess(id, env) {
-  var workerEnv = util._extend({}, process.env);
-  var execArgv = cluster.settings.execArgv.slice();
-  var debugPort = 0;
-  var debugArgvRE =
-    /^(--inspect|--inspect-(brk|port)|--debug|--debug-(brk|port))(=\d+)?$/;
+  const workerEnv = util._extend({}, process.env);
+  const execArgv = cluster.settings.execArgv.slice();
+  const debugArgRegex = /--inspect(?:-brk|-port)?|--debug-port/;
 
   util._extend(workerEnv, env);
   workerEnv.NODE_UNIQUE_ID = '' + id;
 
-  for (var i = 0; i < execArgv.length; i++) {
-    const match = execArgv[i].match(debugArgvRE);
-
-    if (match) {
-      if (debugPort === 0) {
-        debugPort = process.debugPort + debugPortOffset;
-        ++debugPortOffset;
-      }
-
-      execArgv[i] = match[1] + '=' + debugPort;
-    }
+  if (execArgv.some((arg) => arg.match(debugArgRegex))) {
+    execArgv.push(`--inspect-port=${process.debugPort + debugPortOffset}`);
+    debugPortOffset++;
   }
 
   return fork(cluster.settings.exec, cluster.settings.args, {
diff --git a/src/node.cc b/src/node.cc
index bbce10220f..25a5f2c275 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -242,7 +242,7 @@ static double prog_start_time;
 static Mutex node_isolate_mutex;
 static v8::Isolate* node_isolate;
 
-static node::DebugOptions debug_options;
+node::DebugOptions debug_options;
 
 static struct {
 #if NODE_USE_V8_PLATFORM
diff --git a/src/node_config.cc b/src/node_config.cc
index f4729a64fe..b309171282 100644
--- a/src/node_config.cc
+++ b/src/node_config.cc
@@ -4,11 +4,14 @@
 #include "env-inl.h"
 #include "util.h"
 #include "util-inl.h"
+#include "node_debug_options.h"
 
 
 namespace node {
 
+using v8::Boolean;
 using v8::Context;
+using v8::Integer;
 using v8::Local;
 using v8::Object;
 using v8::ReadOnly;
@@ -62,6 +65,27 @@ static void InitConfig(Local<Object> target,
     target->DefineOwnProperty(env->context(), name, value).FromJust();
   }
 
+  Local<Object> debugOptions = Object::New(env->isolate());
+
+  target->DefineOwnProperty(env->context(),
+                            OneByteString(env->isolate(), "debugOptions"),
+                            debugOptions).FromJust();
+
+  debugOptions->DefineOwnProperty(env->context(),
+                            OneByteString(env->isolate(), "host"),
+                            String::NewFromUtf8(env->isolate(),
+                            debug_options.host_name().c_str())).FromJust();
+
+  debugOptions->DefineOwnProperty(env->context(),
+                            OneByteString(env->isolate(), "port"),
+                            Integer::New(env->isolate(),
+                            debug_options.port())).FromJust();
+
+  debugOptions->DefineOwnProperty(env->context(),
+                            OneByteString(env->isolate(), "inspectorEnabled"),
+                            Boolean::New(env->isolate(),
+                            debug_options.inspector_enabled())).FromJust();
+
   if (config_expose_internals)
     READONLY_BOOLEAN_PROPERTY("exposeInternals");
 }  // InitConfig
diff --git a/src/node_internals.h b/src/node_internals.h
index d857f3d4a3..d6bdf9b5ba 100644
--- a/src/node_internals.h
+++ b/src/node_internals.h
@@ -30,6 +30,7 @@
 #include "uv.h"
 #include "v8.h"
 #include "tracing/trace_event.h"
+#include "node_debug_options.h"
 
 #include <stdint.h>
 #include <stdlib.h>
@@ -100,6 +101,11 @@ extern bool config_pending_deprecation;
 // Tells whether it is safe to call v8::Isolate::GetCurrent().
 extern bool v8_initialized;
 
+// Contains initial debug options.
+// Set in node.cc.
+// Used in node_config.cc.
+extern node::DebugOptions debug_options;
+
 // Forward declaration
 class Environment;
 
diff --git a/test/inspector/test-inspector-port-cluster.js b/test/inspector/test-inspector-port-cluster.js
new file mode 100644
index 0000000000..b2a53f87ea
--- /dev/null
+++ b/test/inspector/test-inspector-port-cluster.js
@@ -0,0 +1,129 @@
+'use strict';
+
+const common = require('../common');
+
+common.skipIfInspectorDisabled();
+
+const assert = require('assert');
+const cluster = require('cluster');
+
+const debuggerPort = common.PORT;
+const childProcess = require('child_process');
+
+let offset = 0;
+
+/*
+ * This test suite checks that inspector port in cluster is incremented
+ * for different execArgv combinations
+ */
+
+function testRunnerMain() {
+  spawnMaster({
+    execArgv: ['--inspect'],
+    workers: [{expectedPort: 9230}]
+  });
+
+  let port = debuggerPort + offset++ * 10;
+
+  spawnMaster({
+    execArgv: [`--inspect=${port}`],
+    workers: [
+      {expectedPort: port + 1},
+      {expectedPort: port + 2},
+      {expectedPort: port + 3}
+    ]
+  });
+
+  port = debuggerPort + offset++ * 10;
+
+  spawnMaster({
+    execArgv: ['--inspect', `--inspect-port=${port}`],
+    workers: [{expectedPort: port + 1}]
+  });
+
+  port = debuggerPort + offset++ * 10;
+
+  spawnMaster({
+    execArgv: ['--inspect', `--debug-port=${port}`],
+    workers: [{expectedPort: port + 1}]
+  });
+
+  port = debuggerPort + offset++ * 10;
+
+  spawnMaster({
+    execArgv: [`--inspect=0.0.0.0:${port}`],
+    workers: [{expectedPort: port + 1, expectedHost: '0.0.0.0'}]
+  });
+
+  port = debuggerPort + offset++ * 10;
+
+  spawnMaster({
+    execArgv: [`--inspect=127.0.0.1:${port}`],
+    workers: [{expectedPort: port + 1, expectedHost: '127.0.0.1'}]
+  });
+
+  if (common.hasIPv6) {
+    port = debuggerPort + offset++ * 10;
+
+    spawnMaster({
+      execArgv: [`--inspect=[::]:${port}`],
+      workers: [{expectedPort: port + 1, expectedHost: '::'}]
+    });
+
+    port = debuggerPort + offset++ * 10;
+
+    spawnMaster({
+      execArgv: [`--inspect=[::1]:${port}`],
+      workers: [{expectedPort: port + 1, expectedHost: '::1'}]
+    });
+  }
+}
+
+function masterProcessMain() {
+  const workers = JSON.parse(process.env.workers);
+
+  for (const worker of workers) {
+    cluster.fork({
+      expectedPort: worker.expectedPort,
+      expectedHost: worker.expectedHost
+    }).on('exit', common.mustCall(checkExitCode));
+  }
+}
+
+function workerProcessMain() {
+  const {expectedPort, expectedHost} = process.env;
+
+  assert.strictEqual(process.debugPort, +expectedPort);
+
+  if (expectedHost !== 'undefined') {
+    assert.strictEqual(
+      process.binding('config').debugOptions.host,
+      expectedHost
+    );
+  }
+
+  process.exit();
+}
+
+function spawnMaster({execArgv, workers}) {
+  childProcess.fork(__filename, {
+    env: {
+      workers: JSON.stringify(workers),
+      testProcess: true
+    },
+    execArgv
+  }).on('exit', common.mustCall(checkExitCode));
+}
+
+function checkExitCode(code, signal) {
+  assert.strictEqual(code, 0);
+  assert.strictEqual(signal, null);
+}
+
+if (!process.env.testProcess) {
+  testRunnerMain();
+} else if (cluster.isMaster) {
+  masterProcessMain();
+} else {
+  workerProcessMain();
+}
diff --git a/test/parallel/test-cluster-inspector-debug-port.js b/test/parallel/test-cluster-inspector-debug-port.js
deleted file mode 100644
index a049da78be..0000000000
--- a/test/parallel/test-cluster-inspector-debug-port.js
+++ /dev/null
@@ -1,41 +0,0 @@
-'use strict';
-// Flags: --inspect={PORT}
-const common = require('../common');
-
-common.skipIfInspectorDisabled();
-
-const assert = require('assert');
-const cluster = require('cluster');
-const debuggerPort = common.PORT;
-
-if (cluster.isMaster) {
-  function checkExitCode(code, signal) {
-    assert.strictEqual(code, 0);
-    assert.strictEqual(signal, null);
-  }
-
-  function fork(offset, execArgv) {
-    if (execArgv)
-      cluster.setupMaster({execArgv});
-
-    const check = common.mustCall(checkExitCode);
-    cluster.fork({portSet: debuggerPort + offset}).on('exit', check);
-  }
-
-  assert.strictEqual(process.debugPort, debuggerPort);
-
-  fork(1);
-  fork(2, ['--inspect']);
-  fork(3, [`--inspect=${debuggerPort}`]);
-  fork(4, ['--inspect', `--debug-port=${debuggerPort}`]);
-  fork(5, [`--inspect-port=${debuggerPort}`]);
-  fork(6, ['--inspect', `--inspect-port=${debuggerPort}`]);
-} else {
-  const hasDebugArg = process.execArgv.some(function(arg) {
-    return /inspect/.test(arg);
-  });
-
-  assert.strictEqual(hasDebugArg, true);
-  assert.strictEqual(process.debugPort, +process.env.portSet);
-  process.exit();
-}