From 2777a7e04f9a116937296ecd1a7ebd4a32766df6 Mon Sep 17 00:00:00 2001 From: cornholio <0@mcornholio.ru> Date: Sun, 11 Jun 2017 22:01:27 +0300 Subject: [PATCH] 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 Reviewed-By: James M Snell Reviewed-By: Sam Roberts Reviewed-By: Matteo Collina Reviewed-By: Colin Ihrig --- lib/internal/cluster/master.js | 22 +-- src/node.cc | 2 +- src/node_config.cc | 24 ++++ src/node_internals.h | 6 + test/inspector/test-inspector-port-cluster.js | 129 ++++++++++++++++++ .../test-cluster-inspector-debug-port.js | 41 ------ 6 files changed, 166 insertions(+), 58 deletions(-) create mode 100644 test/inspector/test-inspector-port-cluster.js delete mode 100644 test/parallel/test-cluster-inspector-debug-port.js diff --git a/lib/internal/cluster/master.js b/lib/internal/cluster/master.js index 05bba01c4ed6e8..2d1e2d3097f75b 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 bbce10220fe175..25a5f2c2754cbc 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 f4729a64fe6bcb..b309171282182a 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 target, target->DefineOwnProperty(env->context(), name, value).FromJust(); } + Local 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 d857f3d4a3d5af..d6bdf9b5ba4399 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 #include @@ -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 00000000000000..b2a53f87ea172e --- /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 a049da78be0f70..00000000000000 --- 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(); -}