Fixed cluster inspect port logic #13619

mutantcornholio · 2017-06-11T19:33:59Z

This is first PR mentioned in
#9659 (comment)

Turned out, instead of parsing execArgv and deciding what port to use, we could just add
--inspect-port to whatever port we calculated (thanks, @sam-github).
I've exported initial debug options to process._debugOptions, but I need it only in tests for now.

To test new cases (different host:port variations), I wrote new test
suite inspector/test-inspector-port-cluster.js and deleted
parallel/test-cluster-inspector-debug-port.js in favor of
it. cluster.setupMaster wasn't sufficient, I had actually start master process with debug arguments.
(parallel/test-cluster-inspector-debug-port.js was still passing at the moment)

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

cluster

cc/ @refack @sam-github @Trott @bnoordhuis @cjihrig

Trott · 2017-06-11T21:04:19Z

/cc @mcollina

Trott · 2017-06-11T21:06:36Z

/cc @nodejs/testing

refack

Some nits.
Would like more review from others.

refack · 2017-06-11T23:31:09Z

src/node.cc

+  // process._debugOptions
+  Local<Object> debugOptions = Object::New(env->isolate());
+  READONLY_DONT_ENUM_PROPERTY(process,
+                              "_debugOptions",


A few comments (don't fix then until we get more feedback)

There is preference to have this in src/node_constants.cc but I'm not sure how easy that is to move it

I would call it _cliDebugOptions and add enabled.

Maybe there's a more efficient way to set these since they are constants?

Everything in src/node_constants.cc looks like something defined at compile time, therefore it's no place for these. You're sure about that?

#12949 (comment)
It's not trivial, so I deferred that move. I have no strong feeling about this, maybe someone else does.

It should be in require('config'), IIRC, not the node constants, right @jasnell ?

Yes, please drop this off process.binding('config') rather than adding a new property off process

// The config binding is used to provide an internal view of compile or runtime // config options that are required internally by lib/*.js code. This is an // alternative to dropping additional properties onto the process object as // has been the practice previously in node.cc.

I didn't know that! Yeah, ok, makes sense

@jasnell done

refack · 2017-06-11T23:33:22Z

test/inspector/test-inspector-port-cluster.js

+    port = debuggerPort + offset++ * 10;
+
+    spawnMaster({
+      execArgv: [`--inspect=[::]:${port}`],


does this work? (before #13478 lands)?

That depends on your definition of work :)

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

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.

refack · 2017-06-12T10:38:29Z

/cc @nodejs/v8-inspector @mcollina

mcollina · 2017-06-12T14:46:02Z

I think this should be documented both in doc/api/cluster.md and in the website in the inspector tab.

I have no time to try this out right now, but the code LGTM pending docs.

mutantcornholio · 2017-06-13T07:01:56Z

@mcollina what exactly should be documented?
That debug ports are incremented on each worker in cluster? That's the way it works since v4 and I've just fixed some corner cases.
Or should I document adding --inspect-port to worker's execArgv? That's, IMHO, a little under-the-hood'ish.

Btw, if we're about to document inspect port incrementing behavior (or is it already somewhere in docs?)...
After this PR get merges, I'm planning another PR (#9659 (comment), step 3) and that change should probably get documented. I can document both cases there at once.

P.S. could anyone run the CI please?

jasnell · 2017-06-13T15:50:45Z

CI: https://ci.nodejs.org/job/node-test-pull-request/8633/

refack · 2017-06-13T17:51:53Z

test/inspector/test-inspector-port-cluster.js

+
+  spawnMaster({
+    execArgv: [`--inspect=${port}`],
+    workers: [


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]

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?

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.

It is relevant to sequential tests too?

Yes, unfortunatly

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)

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.

refack · 2017-06-13T18:01:27Z

Ignore ARM (Pi3) fail. CI is ✔️

@mutantcornholio You could document inspector port incrementing (Ref: #13343 (comment)) but since it wasn't changed by this PR, IMHO that's up to you.

AFAICT this still overrides cluster.settings.execArgv so #12941 is still broken and there's no way to opt-out of auto-port-incrementing, right?

mutantcornholio · 2017-06-13T18:36:26Z

Yes, that will in next PR. After this gets to master.

jasnell · 2017-06-13T19:16:04Z

test/inspector/test-inspector-port-cluster.js

+
+const assert = require('assert');
+const cluster = require('cluster');
+const common = require('../common');


common should be the first require.

sam-github · 2017-06-13T21:33:09Z

I think you may need to bring the check for debugArvRE back.

Unless I am missing something, there is a subtle trade-off here.

After your change:

con: after enough workers have forked, debugPortOffset will be very large, eventually, larger than 16K, at which point no new worker with can start a debugger because the port is invalid
pro: every worker will have a different debug port, so no matter what the CLI for the master was, every worker can simultaneously have its debuger turned on with SIGUSR1

But before:

con: with no debug/inspect args on the master CLI, every worker and master had same debug port, so only one could be debugged at a time
pro: every worker would be able to be debugged with SIGUSR1 because it would never have an invalid port, no matter how many workers got (re)started, and you could turn the port on and off on workers to choose which one to debug

I don't have an opinion on which set of pro-vs-con is better or worse, but changing the balance from one to the other could be seen as semver-major.

Clearly @eugeneo's idea of using new inspect protocol features so that only the master listens on the debug port, and the workers communicate with the master and debug messages to them tunnel over one single TCP connection is the eat-your-cake-and-have-it-too solution, but that'll be a while coming.

sam-github · 2017-06-13T21:36:14Z

Other than the possible semver-majorness of this (which could probably be fixed with one regex test), I really like its simplicity, anything that does more with less code is 💯 by me.

mutantcornholio · 2017-06-13T22:43:14Z

I think you may need to bring the check for debugArvRE back.
after enough workers have forked, debugPortOffset will be very large, eventually, larger than 16K

Yeah, I think you're right. That may be a problem. In our production, worker ids sometimes may go up to a 1K.
I guess, I'll return debug regex.

refack · 2017-06-13T22:47:10Z

Unless I am missing something, there is a subtle trade-off here.

I'm trying to figure this out also, but it seems to me that ports would increment for children anyway 🤔 (that's #12941) the user has no control over that...

sam-github · 2017-06-13T22:49:41Z

I think the debug port used to be incremented only in this case:

node/lib/internal/cluster/master.js

Line 109 in 9dc3f93

const match = execArgv[i].match(debugArgvRE);

Instead of parsing execArgv, just adding --inspect-port with whatever debug port should be. Also exporting initial debug options to `process._debugOptions`, but needed it only in tests for now. parallel/test-cluster-inspector-debug-port.js deleted in favor of inspector/test-inspector-port-cluster.js Refs: nodejs#9659 (comment)

mutantcornholio · 2017-06-13T23:07:27Z

@sam-github done

* 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: nodejs#13619 Refs: nodejs#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>

refack · 2017-06-16T02:52:15Z

Landed in 2777a7e 🍾

refack · 2017-06-16T02:52:59Z

Quick extra sanity on master: https://ci.nodejs.org/job/node-test-commit-linuxone/6666/

mutantcornholio · 2017-06-16T04:54:53Z

^_^

Trott · 2017-06-16T05:25:52Z

Thanks and congrats, @mutantcornholio! 🎉

* 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: #13619 Refs: #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>

MylesBorins · 2017-07-17T18:33:22Z

While we have not been backporting most inspector changes, this looks potentially useful.

Thoughts on backporting to v6.x?

mutantcornholio · 2017-07-17T19:37:28Z

While I had problems making this work on 6.x (see #9659), it is possible to backport it. Don't know if it worth the trouble though.

The problem was there at least since 4.x, maybe even 0.12.

That, and you'll break the hack I'm using to set worker's debug port (#9659 (comment)) :D

refack · 2017-07-17T23:50:56Z

Even though this is not specifically an inspector/debugger fix, it just deals with ports.
I'd say don't port as there's a workaround (a.k.a. hack) — --debug=[::]:${PORT}
We can alway revisit if requested by users.

When using cluster and --inspect as cli argument it is correctly handled and each worker will use a different port, this was fixed by nodejs#13619. But when env var NODE_OPTIONS="--inspect" is set this logic doesn't apply and the workers will fail as they try to attach to the same port. Fixes: nodejs#19026

…k with NODE_OPTIONS="--inspect" When using cluster and --inspect as cli argument it is correctly handled and each worker will use a different port, this was fixed by nodejs#13619. But when env var NODE_OPTIONS="--inspect" is set this logic doesn't apply and the workers will fail as they try to attach to the same port. Fixes: nodejs#19026

When using cluster and --inspect as cli argument it is correctly handled and each worker will use a different port, this was fixed by #13619. But when env var NODE_OPTIONS="--inspect" is set this logic doesn't apply and the workers will fail as they try to attach to the same port. Fixes: #19026 PR-URL: #19165 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jun 11, 2017

mutantcornholio mentioned this pull request Jun 11, 2017

cluster: fixing debug port logic for forking workers #9659

Closed

3 tasks

mscdex added cluster Issues and PRs related to the cluster subsystem. inspector Issues and PRs related to the V8 inspector protocol labels Jun 11, 2017

refack mentioned this pull request Jun 11, 2017

test: fix test-inspector-port-zero-cluster #13373

Merged

3 tasks

refack approved these changes Jun 11, 2017

View reviewed changes

refack self-assigned this Jun 11, 2017

refack reviewed Jun 13, 2017

View reviewed changes

mutantcornholio mentioned this pull request Jun 13, 2017

Programatically setting the debug flag has no effects #12941

Closed

jasnell reviewed Jun 13, 2017

View reviewed changes

jasnell approved these changes Jun 13, 2017

View reviewed changes

mutantcornholio force-pushed the cluster-inspect branch 3 times, most recently from 5867317 to 5a685d9 Compare June 13, 2017 20:39

mutantcornholio force-pushed the cluster-inspect branch from 5a685d9 to 5e101d9 Compare June 13, 2017 23:07

refack closed this Jun 16, 2017

addaleax mentioned this pull request Jun 17, 2017

v8.2.0 proposal #13744

Merged

addaleax mentioned this pull request Jun 21, 2017

v8.1.3 proposal #13861

Merged

MylesBorins added the lts-watch-v6.x label Jul 17, 2017

This was referenced Jul 17, 2017

Feature request: API for debugging node child processes #9621

Closed

cluster: fix debugging port collision #12395

Closed

refack added dont-land-on-v6.x and removed lts-watch-v6.x labels Jul 17, 2017

mcfedr mentioned this pull request Feb 27, 2018

Cluster fails with NODE_OPTIONS="--inspect" #19026

Closed

sameer-coder mentioned this pull request Mar 6, 2018

cluster: Add support to cluster to work with `NODE_OPTIONS="--inspect" #19165

Closed

refack removed their assignment Oct 20, 2018

Fixed cluster inspect port logic #13619

Fixed cluster inspect port logic #13619

Conversation

mutantcornholio commented Jun 11, 2017

Checklist

Affected core subsystem(s)

Trott commented Jun 11, 2017

Trott commented Jun 11, 2017

refack left a comment • edited Loading

Choose a reason for hiding this comment

refack Jun 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Jun 12, 2017

mcollina commented Jun 12, 2017

mutantcornholio commented Jun 13, 2017

jasnell commented Jun 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Jun 13, 2017

mutantcornholio commented Jun 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-github commented Jun 13, 2017 • edited Loading

sam-github commented Jun 13, 2017

mutantcornholio commented Jun 13, 2017

refack commented Jun 13, 2017

sam-github commented Jun 13, 2017

mutantcornholio commented Jun 13, 2017

refack commented Jun 16, 2017

refack commented Jun 16, 2017

mutantcornholio commented Jun 16, 2017

Trott commented Jun 16, 2017

MylesBorins commented Jul 17, 2017

mutantcornholio commented Jul 17, 2017

refack commented Jul 17, 2017

refack left a comment •

edited

Loading

refack Jun 11, 2017 •

edited

Loading

sam-github commented Jun 13, 2017 •

edited

Loading