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

Added inspect port overriding in cluster #13761

Closed
wants to merge 1 commit into from
Closed

Added inspect port overriding in cluster #13761

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 13 additions & 3 deletions lib/internal/cluster/master.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ cluster.SCHED_RR = SCHED_RR; // Master distributes connections.
var ids = 0;
var debugPortOffset = 1;
var initialized = false;
let debugSettingsOverriden = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

a global, augh...
 💡
🤓
create a Symbol and hang it on cluster.settings

const kDebugSettingsOverriden = Symbol('Debug Settings Overriden');
...
    var settings = {
      args: process.argv.slice(2),
      exec: process.argv[1],
      execArgv: process.execArgv,
      silent: false,
      [kDebugSettingsOverriden]: false
    };

What the heck, hang them all (maybe in the different PR though)


// XXX(bnoordhuis) Fold cluster.schedulingPolicy into cluster.settings?
var schedulingPolicy = {
Expand All @@ -41,15 +42,15 @@ if (schedulingPolicy === undefined) {

cluster.schedulingPolicy = schedulingPolicy;

cluster.setupMaster = function(options) {
cluster.setupMaster = function(options = {}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a silly one, but this might be a breaking change, but IMHO we should keep it unless someone objects.
This changes the .length of the method:

> require('cluster').setupMaster.length
1
> (function a(a1) {}).length
1
> (function b(b1={}) {}).length
0

Copy link
Member

Choose a reason for hiding this comment

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

This is definitely a breaking change, albeit a subtle one. Setting an argument default necessarily makes this semver-major

Copy link
Member

Choose a reason for hiding this comment

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

@jasnell Can you explain why? It’s not obvious to me how this would break code that isn’t designed specifically to be broken by this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why its backwards incompatible, but it is unnecessary, as is util._extend(settings, options || {}). _extend() ignores non-object second args. Default here should be removed, and the || {} that is removed below can stay removed.

var settings = {
args: process.argv.slice(2),
exec: process.argv[1],
execArgv: process.execArgv,
silent: false
};
util._extend(settings, cluster.settings);
util._extend(settings, options || {});
util._extend(settings, options);
Copy link
Contributor

Choose a reason for hiding this comment

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

+1, the || {} was unnecessary, _extend() already does the right thing.


// 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
Expand All @@ -60,6 +61,14 @@ cluster.setupMaster = function(options) {
settings.execArgv = settings.execArgv.concat(['--logfile=v8-%p.log']);
}

// This allows user to override inspect port for workers.
const debugPortArgsRegex = /--inspect(=|-port=)|--debug-port=/;
Copy link
Contributor

Choose a reason for hiding this comment

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

const debugOverrideInSettings = options.execArgv && options.execArgv.some((arg) => arg.match(debugPortArgsRegex))
settings.execArgv[kDebugSettingsInArgv] = settings.execArgv.some((arg) => arg.match(/--inspect(?:-brk|-port)?|--debug-port/)))

Or some variation of... Then reuse


if (!debugSettingsOverriden && options.execArgv &&
options.execArgv.some((arg) => arg.match(debugPortArgsRegex))) {
debugSettingsOverriden = true;
}

cluster.settings = settings;

if (initialized === true)
Expand Down Expand Up @@ -103,7 +112,8 @@ function createWorkerProcess(id, env) {
util._extend(workerEnv, env);
workerEnv.NODE_UNIQUE_ID = '' + id;

if (execArgv.some((arg) => arg.match(debugArgRegex))) {
if (!debugSettingsOverriden &&
execArgv.some((arg) => arg.match(debugArgRegex))) {
execArgv.push(`--inspect-port=${process.debugPort + debugPortOffset}`);
debugPortOffset++;
}
Expand Down
121 changes: 106 additions & 15 deletions test/inspector/test-inspector-port-cluster.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ function testRunnerMain() {
workers: [{expectedPort: 9230}]
});

let port = debuggerPort + offset++ * 10;
let port = debuggerPort + offset++ * 5;

spawnMaster({
execArgv: [`--inspect=${port}`],
Expand All @@ -34,81 +34,172 @@ function testRunnerMain() {
]
});

port = debuggerPort + offset++ * 10;
port = debuggerPort + offset++ * 5;

spawnMaster({
execArgv: ['--inspect', `--inspect-port=${port}`],
workers: [{expectedPort: port + 1}]
});

port = debuggerPort + offset++ * 10;
port = debuggerPort + offset++ * 5;

spawnMaster({
execArgv: ['--inspect', `--debug-port=${port}`],
workers: [{expectedPort: port + 1}]
});

port = debuggerPort + offset++ * 10;
port = debuggerPort + offset++ * 5;

spawnMaster({
execArgv: [`--inspect=0.0.0.0:${port}`],
workers: [{expectedPort: port + 1, expectedHost: '0.0.0.0'}]
});

port = debuggerPort + offset++ * 10;
port = debuggerPort + offset++ * 5;

spawnMaster({
execArgv: [`--inspect=127.0.0.1:${port}`],
workers: [{expectedPort: port + 1, expectedHost: '127.0.0.1'}]
});

if (common.hasIPv6) {
port = debuggerPort + offset++ * 10;
port = debuggerPort + offset++ * 5;

spawnMaster({
execArgv: [`--inspect=[::]:${port}`],
workers: [{expectedPort: port + 1, expectedHost: '::'}]
});

port = debuggerPort + offset++ * 10;
port = debuggerPort + offset++ * 5;

spawnMaster({
execArgv: [`--inspect=[::1]:${port}`],
workers: [{expectedPort: port + 1, expectedHost: '::1'}]
});
}

/*
* Following tests check that port should not increment
* if developer sets inspector port in cluster.setupMaster arguments.
*/

port = debuggerPort + offset++ * 5;

spawnMaster({
execArgv: [`--inspect=${port}`],
workers: [
{expectedPort: port + 2},
{expectedPort: port + 4},
{expectedPort: port + 6}
],
clusterExecArgv: [
[`--inspect=${port + 2}`],
[`--inspect-port=${port + 4}`],
[`--debug-port=${port + 6}`],
]
});

port = debuggerPort + offset++ * 5;

spawnMaster({
execArgv: [],
workers: [
{expectedPort: port}
],
clusterExecArgv: [
[`--inspect=${port}`]
]
});

// Next tests check that inspector port incrementing logic
// is disabled if zero port passed to workers.
// Even if supplied execArgv is equal to master's.

spawnMaster({
execArgv: [],
workers: [
{expectedInitialPort: 0},
{expectedInitialPort: 0},
{expectedInitialPort: 0}
],
clusterExecArgv: [
['--inspect=0'],
['--inspect=0'],
['--inspect=0']
]
});

spawnMaster({
execArgv: ['--inspect=0'],
workers: [
{expectedInitialPort: 0},
{expectedInitialPort: 0},
{expectedInitialPort: 0}
],
clusterExecArgv: [
['--inspect=0'],
['--inspect=0'],
['--inspect=0']
]
});

spawnMaster({
execArgv: ['--inspect=0'],
workers: [
{expectedInitialPort: 0},
{expectedInitialPort: 0},
{expectedInitialPort: 0}
],
clusterExecArgv: [
['--inspect', '--inspect-port=0'],
['--inspect', '--inspect-port=0'],
['--inspect', '--inspect-port=0']
]
});

}

function masterProcessMain() {
const workers = JSON.parse(process.env.workers);
const clusterExecArgv = JSON.parse(process.env.clusterExecArgv);

for (const [index, worker] of workers.entries()) {
if (clusterExecArgv[index]) {
cluster.setupMaster({execArgv: clusterExecArgv[index]});
}

for (const worker of workers) {
cluster.fork({
expectedPort: worker.expectedPort,
expectedInitialPort: worker.expectedInitialPort,
expectedHost: worker.expectedHost
}).on('exit', common.mustCall(checkExitCode));
}
}

function workerProcessMain() {
const {expectedPort, expectedHost} = process.env;
const {expectedPort, expectedInitialPort, expectedHost} = process.env;
const debugOptions = process.binding('config').debugOptions;

if (+expectedPort) {
assert.strictEqual(process.debugPort, +expectedPort);
}

assert.strictEqual(process.debugPort, +expectedPort);
if (+expectedInitialPort) {
assert.strictEqual(debugOptions.port, +expectedInitialPort);
}

if (expectedHost !== 'undefined') {
assert.strictEqual(
process.binding('config').debugOptions.host,
expectedHost
);
assert.strictEqual(debugOptions.host, expectedHost);
}

process.exit();
}

function spawnMaster({execArgv, workers}) {
function spawnMaster({execArgv, workers, clusterExecArgv = []}) {
childProcess.fork(__filename, {
env: {
workers: JSON.stringify(workers),
clusterExecArgv: JSON.stringify(clusterExecArgv),
testProcess: true
},
execArgv
Expand Down