Skip to content

Fixed cluster inspect port logic #13619

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

Closed
mutantcornholio wants to merge 1 commit into from
Closed

Fixed cluster inspect port logic #13619

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
22 changes: 6 additions & 16 deletions lib/internal/cluster/master.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, {
Expand Down
2 changes: 1 addition & 1 deletion src/node.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
24 changes: 24 additions & 0 deletions src/node_config.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
6 changes: 6 additions & 0 deletions src/node_internals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -83,6 +84,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;
Copy link
Copy Markdown
Contributor

@refack refack Jun 13, 2017

Choose a reason for hiding this comment

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

Why not put this just in src/node_config.cc?

Copy link
Copy Markdown
Contributor Author

@mutantcornholio mutantcornholio Jun 13, 2017

Choose a reason for hiding this comment

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

For consistency sake? I was looking at config_preserve_symlinks as an example

Copy link
Copy Markdown
Contributor

@sam-github sam-github Jun 13, 2017

Choose a reason for hiding this comment

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

This is where I'd put it. I can see how it has wider scope than the two files that will use it, but putting it in a header guarantees consistency between the place of definition and place of use.

Copy link
Copy Markdown
Contributor

@refack refack Jun 13, 2017

Choose a reason for hiding this comment

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

Ack.


// Forward declaration
class Environment;

Expand Down
129 changes: 129 additions & 0 deletions test/inspector/test-inspector-port-cluster.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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: [
Copy link
Copy Markdown
Contributor

@refack refack Jun 13, 2017

Choose a reason for hiding this comment

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

This could be fragile (because of infra issues) maybe try the argv parsing approach for the test like in #13373
Ref: #13343

Extra: when using ports in the ephemeral range (not 9229, but maybe 12346) we get port collisions when doing port +[1,2,3]

Copy link
Copy Markdown
Contributor Author

@mutantcornholio mutantcornholio Jun 13, 2017

Choose a reason for hiding this comment

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

This could be fragile

Could you describe the issue, please? I'm not quite getting what's going wrong..

we get port collisions when doing port +[1,2,3]

Port collisions between tests within single test run?

Copy link
Copy Markdown
Contributor

@refack refack Jun 13, 2017

Choose a reason for hiding this comment

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

Port collisions between tests within single test run?

Yes. Probably will not be a problem if you use common.PORT but if you use 0 the OS give other processes ports in that range. We're not sure to whom, maybe Jenkins communication, but we've seen port clashes not rarely.

Copy link
Copy Markdown
Contributor Author

@mutantcornholio mutantcornholio Jun 13, 2017

Choose a reason for hiding this comment

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

It is relevant to sequential tests too?

Copy link
Copy Markdown
Contributor

@refack refack Jun 13, 2017

Choose a reason for hiding this comment

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

Yes, unfortunatly

Copy link
Copy Markdown
Contributor

@refack refack Jun 13, 2017

Choose a reason for hiding this comment

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

This is what I suggested doing https://github.com/nodejs/node/pull/13373/files#diff-4b2a51d5cbf4c8edb50bfd0f7016bb84R16 And just close the workers immediately.
In a way it's actually a more specific test to your code changes (manipulating argv, rather than making sure the worker starts)

Copy link
Copy Markdown
Contributor Author

@mutantcornholio mutantcornholio Jun 14, 2017

Choose a reason for hiding this comment

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

I've looked at the test again and I don't think my case is relevant to yours.

Testing zero port + offset will be always dangerous because you can get port near some-constantly-occupied port.
With sequential tests that's the only case I've got in mind.

That case couldn't take place with common.PORT + offset because it would result in constant CI fails, not rare occasions.

{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}`],
Copy link
Copy Markdown
Contributor

@refack refack Jun 11, 2017

Choose a reason for hiding this comment

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

does this work? (before #13478 lands)?

Copy link
Copy Markdown
Contributor Author

@mutantcornholio mutantcornholio Jun 12, 2017

Choose a reason for hiding this comment

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

That depends on your definition of work :)

Yes, worker listens on ipv4 address, but that's not what I'm checking here.

Copy link
Copy Markdown
Contributor

@refack refack Jun 12, 2017

Choose a reason for hiding this comment

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

Ohh yeah, the bug was just that, if the hostname is not an IPv4 address resolution falls back to 127.0.0.1. #13477
So this might change after #13478.

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();
}
41 changes: 0 additions & 41 deletions test/parallel/test-cluster-inspector-debug-port.js
View file
Open in desktop

This file was deleted.