-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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.
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
Added inspect port overriding in cluster #13761
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Not even sure It's semver-major
IMHO it's a bug fix.
lib/internal/cluster/master.js
Outdated
.filter((arg) => arg.match(debugArgRegex)) | ||
.sort(); | ||
|
||
let debugSettingsOverriden = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion:
const debugSettingsOverriden = clusterDebugArgs.any((v, i) => v != masterDebugArgs[i]);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option, make cluster.settings.execArgv
a getter/setter property and do check-on-assignment
There is also #13228 and initiating debugger with
d:\code\node-cur$ node --inspect=9229 --inspect=0
Debugger listening on ws://127.0.0.1:52153/590e5e9c-6118-475a-94e5-78103994953c
For help see https://nodejs.org/en/docs/inspector |
IMHO since no tests were changes, and this feels more like a bug fix then behaviour change this could be P.S. @mutantcornholio https://github.com/nodejs/node/blob/master/doc/api/cluster.md#clustersetupmastersettings should be updated to reflect it does accept |
I promise to update the docs before it gets merged (see unchecked box in checklist?) I like the idea of making cluster.settings.execArgv a getter/setter. |
Sorry, I think it is 6 years ago since I wrote that code :p Node.js had poor support for debugging at that time, I think it mostly happened through the terminal gdb interface at that time. So debugging cluster workers was not something we had considered. We just ensured that it would be possible to execute the worker as standalone and debug it that way. Obviously, things have changed since then. I don't use the cluster module these days, so I don't have any opinion, just stories to tell :) |
👍 |
/cc @mcollina @nodejs/v8-inspector |
Wait, |
hmmm...
So we can be sure noone is using it directly 😜 🤣 Next PR turn |
I just looked at my codebase and it shows that I've been using it directly for almost two years :D
Are you saying that we should enforce setting Looks better than a warning in docs, for me. |
Yes enforcing, since there is quite a bit of logic in const smartAlec = ['myarg'];
cluster.setupMaster({execArgv: smartAlec});
smartAlec.push('--invalid');
smartAlec.shift(); |
I'm a bit lost on the status of this. It seems the two points @mutantcornholio raised are still not addressed, right? |
Well, first one is pretty straightforward and I'll just use a bigger regex. For the second one, preventing P.S. Is it okay to do setupMaster enforcing in this PR? That looks like a bigger scope issue than overriding inspect port increment logic. |
Can you explain/rephrase this? |
https://github.com/nodejs/node/blob/master/doc/api/cluster.md#clustersettings says "This object is not intended to be changed or set manually." I need to detect setting debug args to So @refack suggested to forbid users to make changes to That will make my task much easier, but should I do both tasks in one PR? |
IMHO that should be in a separate PR, so this one can stay |
That way, I'll probably won't fix |
Incremental progression is good... |
|
If master starts with same debug args as worker, code of this PR won't detect it. But you can start master with |
57ea09c
to
7ce2925
Compare
Current cluster inspect port logic doen't let user to manually set inspect port for workers. After this commit, adding any --inspect* options to execArgv in cluster.setupMaster's options will disable port autoincrement
7ce2925
to
fb9d660
Compare
I've got a little different idea. Check these parts of cluster docs: So, once I call Considering this, just checking I've updated the code and tests (run CI please). |
@@ -41,15 +42,15 @@ if (schedulingPolicy === undefined) { | |||
|
|||
cluster.schedulingPolicy = schedulingPolicy; | |||
|
|||
cluster.setupMaster = function(options) { | |||
cluster.setupMaster = function(options = {}) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@@ -26,6 +26,7 @@ cluster.SCHED_RR = SCHED_RR; // Master distributes connections. | |||
var ids = 0; | |||
var debugPortOffset = 1; | |||
var initialized = false; | |||
let debugSettingsOverriden = false; |
There was a problem hiding this comment.
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)
@@ -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=/; |
There was a problem hiding this comment.
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
I like it. It's simple! cluster.setupMaster({execArgv: ['--inspect=12312']});
fork() // goes to 12312
cluster.setupMaster({execArgv: [--inspect]});
fork() // goes to 9229
fork() // boom |
💥 cluster.setupMaster({execArgv: ['--inspect=12312']});
fork() // goes to 12312
cluster.setupMaster({execArgv: [--inspect], overideDebugPort: false});
fork() // goes to 9229
fork() // goes to 9230
let port = 6666;
cluster.setupMaster({execArgv: [--inspect], overideDebugPort: () => port++});
fork() // goes to 6666
fork() // goes to 6667 |
I thought about that:
Maybe we should do that instead. Boolean option to disable autoincrement logic? |
Quick glance through the code looks good but didn't go through enough to sign off. I will say that it would be helpful to include some details about this in the docs. |
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); |
There was a problem hiding this comment.
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.
This is
|
An option called I'd better split it into boolean And setting What do you think? |
If you do go with an option, maybe drop the |
Dropping So, I'm making two explicit options and removing implicit override in |
I'm just summarizing current thoughts:
Plus:
Things to think about:
My alternative —
oooff a lot to think about... |
I don't see, why we should do both
Well, maybe. But we need a better name for the option, this way. Any ideas?
Once again, I don't think that I would understand what |
I'm convinced.
So:
So now I'm not sure why we need (1) as I see it (2) covers all use-cases. If |
Now I am convinced. I'll do that in a couple of days. |
Closed in favor of #14140 |
* Capitalization and punctuation. * `setupMaster` contained info about `settings` which where incomplete. PR-URL: nodejs#14140 Fixes: nodejs#8495 Fixes: nodejs#12941 Refs: nodejs#9659 Refs: nodejs#13761 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
10 ports for each test-case is too much. Not enough ports for new test cases, considering ~100 ports per file. PR-URL: nodejs#14140 Fixes: nodejs#8495 Fixes: nodejs#12941 Refs: nodejs#9659 Refs: nodejs#13761 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Added an option to override inspector port for workers using `settings.inspectPort` will override default port incrementing behavior. Also, using this option allows to set 0 port for the whole cluster. PR-URL: nodejs#14140 Fixes: nodejs#8495 Fixes: nodejs#12941 Refs: nodejs#9659 Refs: nodejs#13761 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
* Capitalization and punctuation. * `setupMaster` contained info about `settings` which where incomplete. PR-URL: #14140 Fixes: #8495 Fixes: #12941 Refs: #9659 Refs: #13761 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
10 ports for each test-case is too much. Not enough ports for new test cases, considering ~100 ports per file. PR-URL: #14140 Fixes: #8495 Fixes: #12941 Refs: #9659 Refs: #13761 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Added an option to override inspector port for workers using `settings.inspectPort` will override default port incrementing behavior. Also, using this option allows to set 0 port for the whole cluster. PR-URL: #14140 Fixes: #8495 Fixes: #12941 Refs: #9659 Refs: #13761 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
* Capitalization and punctuation. * `setupMaster` contained info about `settings` which where incomplete. PR-URL: #14140 Fixes: #8495 Fixes: #12941 Refs: #9659 Refs: #13761 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
10 ports for each test-case is too much. Not enough ports for new test cases, considering ~100 ports per file. PR-URL: #14140 Fixes: #8495 Fixes: #12941 Refs: #9659 Refs: #13761 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Added an option to override inspector port for workers using `settings.inspectPort` will override default port incrementing behavior. Also, using this option allows to set 0 port for the whole cluster. PR-URL: #14140 Fixes: #8495 Fixes: #12941 Refs: #9659 Refs: #13761 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This is third part that I've mentioned in #9659 (comment)
Current cluster inspect port logic doen't let user to manually set inspect port for workers.
After this commit, adding any
--inspect*
options tocluster.settings.execArgv
will disable port autoincrement.Fixes #8495 and #12941.
I've got a problem with these cases:
--inspect-port
but adds--inspect
tocluster.settings.execArgv
(set the base port, but don't run master with inspector), autoincrementing logic will be disabled. I can't say if this behavior is good or bad. Probably, port overriding should take place only if--inspect=*
or--(inspect|debug)-port=
are passed.cluster.settings.execArgv
will already contain everything inprocess.execArgv
and adding--inspect=0
tocluster.settings
doesn't change anything. (link: test: fix test-inspector-port-zero-cluster #13373 (comment))So maybe instead of this, I should just add
inspectPort
option right tocluster.settings
?Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)