cluster: fixing debug port logic for forking workers #9659

mutantcornholio · 2016-11-17T11:00:27Z

Checklist

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

Affected core subsystem(s)

cluster

Description of change

An attempt to fix #8495 and #9435

You can set debug ports for workers in cluster with cluster.settings.execArgv hacking once again!

The problem is, at current version, debug port for workers calculated from master's process.debugPort, but a lot of software uses cluster.settings which is being ignored.

After this changes, if you set a port in cluster.settings.execArgv and not starting master with debug, port offset would be added to port in cluster.settings.execArgv.

Debug port autoincrement logic seems to be preserved.

The other issue is, debug argument format evolved and current regex does not cover all the cases that node accepts. Parsing it with one regex is too hard now, so I had to write some js to do it.

Also, current logic changes every debug argument, while changing only last one is sufficient.

Fishrock123 · 2016-11-17T15:42:19Z

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

mscdex · 2016-11-17T16:32:52Z

test/parallel/test-cluster-debug-port.js


-  console.log('forked worker should have --debug-port, with offset = 2');
+  console.log('debugging with --debug=[ipv6addr]:port format should also work');


Shouldn't this test only be executed if ipv6 is available (e.g. common.hasIPv6), otherwise it's skipped?

Is it common to test against systems without ipv6 enabled on interface? (c'mon, it's 2016)
But if other ipv6-related tests do check ipv6 availability, these should do it indeed.

Yes, there are ipv6-only tests that bail immediately if it's not available. Besides, it's easy enough to support with just an added conditional.

mscdex · 2016-11-17T16:35:47Z

test/parallel/test-cluster-debug-port.js

  }).on('exit', checkExitCode);

+  console.log('debugging with --debug=ipv4addr:port format should also work');


Can we remove the console.log()s?

console.log()'s were in original test file. I've continued to use them.
I'll use js comments like other test files do.
Thanks for pointing out.

I know, but I think in general when new tests are added or old tests are edited, these kinds of unnecessary outputs are being removed. I can't remember if it was @Trott or someone else who had brought this up before. I think comments are better.

mscdex · 2016-11-17T16:37:05Z

test/parallel/test-cluster-debug-port.js

+    'forked worker should have debugPort, with offset = 1');
+  cluster.fork({
+    portSet: process.debugPort + 1
+  }).on('exit', checkExitCode);


It might be best to wrap all references to checkExitCode with common.mustCall().

~~@mscdex common.mustCall() on an exit handler won't work reliably as common.mustCall() relies on an exit handler itself.~~

Whoops! Ignore me! It won't work reliably on an exit handler on process but should be fine on child_process (I think).

Is it just like this?

cluster.fork({ portSet: process.debugPort + 1 }).on('exit', common.mustCall(checkExitCode));

Yes, like that.

mscdex · 2016-11-17T16:38:01Z

test/parallel/test-cluster-debug-port-manual.js

+
+  cluster.fork({
+    portSet: 13000 + 2
+  }).on('exit', checkExitCode);


Ditto about wrapping checkExitCode.

mscdex · 2016-11-17T16:38:14Z

test/parallel/test-cluster-debug-port-manual.js

+    portSet: 13000
+  }).on('exit', checkExitCode);
+
+  console.log('when using custom port number, ' +


Ditto about console.log() statements.

mutantcornholio · 2016-11-17T21:51:27Z

@mscdex there, i fixed it

mscdex · 2016-11-17T22:46:34Z

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

mscdex · 2016-11-17T23:04:39Z

Looks like there's a relevant failure:

not ok 118 parallel/test-cluster-debug-port
  ---
  duration_ms: 0.510
  severity: fail
  stack: |-
    Debugger listening on 127.0.0.1:12101
    Debugger listening on 127.0.0.1:12202
    Debugger listening on 127.0.0.1:12000
    Debugger listening on [::1]:12303

    assert.js:85
      throw new assert.AssertionError({
      ^
    AssertionError: false === true
        at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux-fips/nodes/ubuntu1404-64/test/parallel/test-cluster-debug-port.js:68:10)
        at Module._compile (module.js:571:32)
        at Object.Module._extensions..js (module.js:580:10)
        at Module.load (module.js:488:32)
        at tryModuleLoad (module.js:447:12)
        at Function.Module._load (module.js:439:3)
        at Module.runMain (module.js:605:10)
        at run (bootstrap_node.js:420:7)
        at startup (bootstrap_node.js:139:9)
        at bootstrap_node.js:535:3

    assert.js:85
      throw new assert.AssertionError({
      ^
    AssertionError: 1 === 0
        at Worker.checkExitCode (/home/iojs/build/workspace/node-test-commit-linux-fips/nodes/ubuntu1404-64/test/parallel/test-cluster-debug-port.js:14:12)
        at Worker.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux-fips/nodes/ubuntu1404-64/test/common.js:421:15)
        at emitTwo (events.js:106:13)
        at Worker.emit (events.js:191:7)
        at ChildProcess.<anonymous> (cluster.js:393:14)
        at ChildProcess.g (events.js:292:16)
        at emitTwo (events.js:106:13)
        at ChildProcess.emit (events.js:191:7)
        at Process.ChildProcess._handle.onexit (internal/child_process.js:215:12)

    assert.js:85
      throw new assert.AssertionError({
      ^
    AssertionError: false === true
        at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux-fips/nodes/ubuntu1404-64/test/parallel/test-cluster-debug-port.js:68:10)
        at Module._compile (module.js:571:32)
        at Object.Module._extensions..js (module.js:580:10)
        at Module.load (module.js:488:32)
        at tryModuleLoad (module.js:447:12)
        at Function.Module._load (module.js:439:3)
        at Module.runMain (module.js:605:10)
        at run (bootstrap_node.js:420:7)
        at startup (bootstrap_node.js:139:9)
        at bootstrap_node.js:535:3

    assert.js:85
      throw new assert.AssertionError({
      ^
    AssertionError: 12202 === 12205
        at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux-fips/nodes/ubuntu1404-64/test/parallel/test-cluster-debug-port.js:69:25)
        at Module._compile (module.js:571:32)
        at Object.Module._extensions..js (module.js:580:10)
        at Module.load (module.js:488:32)
        at tryModuleLoad (module.js:447:12)
        at Function.Module._load (module.js:439:3)
        at Module.runMain (module.js:605:10)
        at run (bootstrap_node.js:420:7)
        at startup (bootstrap_node.js:139:9)
        at bootstrap_node.js:535:3

    assert.js:85
      throw new assert.AssertionError({
      ^
    AssertionError: 12101 === 12104
        at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux-fips/nodes/ubuntu1404-64/test/parallel/test-cluster-debug-port.js:69:25)
        at Module._compile (module.js:571:32)
        at Object.Module._extensions..js (module.js:580:10)
        at Module.load (module.js:488:32)
        at tryModuleLoad (module.js:447:12)
        at Function.Module._load (module.js:439:3)
        at Module.runMain (module.js:605:10)
        at run (bootstrap_node.js:420:7)
        at startup (bootstrap_node.js:139:9)
        at bootstrap_node.js:535:3

mutantcornholio · 2016-11-18T08:40:21Z

I've changed the way to distinguish master and worker before requiring cluster.
Relying on process.execArgv.length wasn't very smart.
Please run CI again.

mscdex · 2016-11-18T08:45:49Z

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

mutantcornholio · 2016-11-18T08:56:10Z

https://ci.nodejs.org/job/node-test-commit-plinux/nodes=ppcbe-ubuntu1404/5200/console this fail looks strange. Could you ru-run that specific job?

mscdex · 2016-11-18T09:22:18Z

@mutantcornholio I wouldn't worry about those two failures, they are CI infrastructure-related. The new changes pass on the previously problematic config (linux-fips).

mutantcornholio · 2016-11-18T11:25:58Z

Could you please review commit message and check the checkbox if everything is okay?

mutantcornholio · 2016-11-22T09:47:06Z

Any news about the PR?

mutantcornholio · 2016-11-25T06:28:51Z

@Fishrock123 @mscdex c'mon, guys, it was a week of silence

mscdex · 2016-11-25T06:49:36Z

@mutantcornholio First line of the commit message should be <= 50 characters

mscdex · 2016-11-25T06:49:53Z

/cc @nodejs/collaborators

mutantcornholio · 2016-11-25T08:34:28Z

@mscdex fixed

Trott · 2016-11-26T05:25:42Z

/cc @cjihrig in particular I guess based on 423d8944

cjihrig · 2016-11-26T15:23:57Z

I think handling of --debug=address:port should be split out into a separate PR, or at least a separate commit.

cjihrig · 2016-11-26T15:33:30Z

test/parallel/test-cluster-debug-port.js

@@ -1,42 +1,72 @@
 'use strict';
-require('../common');
+
+// this is hack to make master process think it is running with --debug


Can you capitalize and punctuate comments please.

I'm still not good with english punctuation, but I've tried my best

mutantcornholio · 2016-11-28T12:36:18Z

I think handling of --debug=address:port should be split out into a separate PR, or at least a separate commit.

@cjihrig makes sense. Done.

mutantcornholio · 2016-12-06T11:10:31Z

@cjihrig @mscdex ping?

mutantcornholio · 2017-06-08T20:00:36Z

Guys, I'm getting tired of this.

I want this scenario:
A cluster of one master and one worker (common dev-stand at my work; we have logic in master without which worker can't work properly).
User passes inspect host:port to master (we share dev servers, this port deafults to '$UID').
Master starts without inspect.
Worker starts with inspect on that host:port.

When we were at 0.10, we used to set --debug=${PORT} in cluster.settings.execArgv and set debug port;

After upgrading to 6.9, this broke.

I found two bugs:
One bug is about cluster module overrides any debug options parsed in cluster.settings.execArgv; Including debug port numbers;
Another (at 6.9) is that cluster ignored --debug=host:port format in execArgv.

So I've bypassed the incrementing port logic by setting --debug=[::]:${PORT};
I'm exploiting two bugs in node.js with that (that's exactly why I'm fixing two bugs in one PR).

I wouldn't dare to commit THAT to our clustering system, so i've commit it right to our node_modules (it's vendored), and I'm recommiting this every time we're updating our clustering system.

With all that stuff we've talked about over the half a year, I want to do this:

Close this PR because I'm gonna rewrite it from scratch. And besause no one will ever read this comments again.
Make a PR fixing all possible formats:
- Initial host and port (debug_options in node.cc) are exported to JS via process._debugOptions object. Object because initial options won't change later. If someone needs anything else in this object, he/she will add it.
- Cluster remembers initial port and adds --inspect-port=${incrementedNumber} to each worker
- Invoking inspector.open() on master does not change ports for future forked workers
- This PR is a bugfix on semver.
Make another PR:
- Any --inspect(-brk|-port)? options from cluster.settings.execArgv are passed to workers unchanged
- incrementing logic will be disabled for this case
- If developer wants incrementing logic, he/she wiil start master with --inspect
- This PR will be major on semver

If no one objects, i'll start working on it tomorrow

@refack @sam-github @bnoordhuis @Trott @gibfahn @cjihrig @thefourtheye @jasnell @mscdex

refack · 2017-06-08T20:08:18Z

Close this PR because I'm gonna rewrite it from scratch. And besause no one will ever read this comments again.

Make a PR fixing all possible formats:
- Initial host and port (debug_options in node.cc) are exported to JS via process._debugOptions object. Object because initial options won't change later. If someone needs anything else in this object, he/she will add it.
- Cluster remembers initial port and adds --inspect-port=${incrementedNumber} to each worker
- Invoking inspector.open() on master does not change ports for future forked workers
- This PR is a bugfix on semver.

Make another PR:
- Any --inspect(-brk|-port)? options from cluster.settings.execArgv are passed to workers as they were, if they are passed in
- incrementing logic will be disabled for this case
- If developer wants incrementing logic, he/she wiil start master with --inspect
- This PR will be majon on semver

@mutantcornholio I appreciate your dedication, and am sorry for the delay and contradictory back and forth.
I think your plan is good. If (2) alone satisfies your requirements IMHO you can do just (2) and we'll pick it up from there.
I'll try to help as much as I can, so we can fix the bug quickly.

mutantcornholio · 2017-06-08T20:54:21Z

If (2) alone satisfies your requirements

No, but (3) does =)

IMHO you can do just (2) and we'll pick it up from there.

I would like to fix (2) myself. I've already did the research, ready to code and want to be a useful member of node.js community!

refack · 2017-06-08T21:03:27Z

ready to code and want to be a useful member of node.js community!

You're my new hero 💂

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

#13619 here we go

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)

* 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>

* 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>

* 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>

nodejs-github-bot added the cluster Issues and PRs related to the cluster subsystem. label Nov 17, 2016

mscdex reviewed Nov 17, 2016

View reviewed changes

mutantcornholio force-pushed the cluster-debug-port-fix branch from 7eea3df to d1e7e3d Compare November 17, 2016 21:47

mutantcornholio force-pushed the cluster-debug-port-fix branch from d1e7e3d to 19c0339 Compare November 18, 2016 08:33

mutantcornholio force-pushed the cluster-debug-port-fix branch from 19c0339 to 9e7c8d0 Compare November 25, 2016 08:34

cjihrig reviewed Nov 26, 2016

View reviewed changes

mutantcornholio force-pushed the cluster-debug-port-fix branch 2 times, most recently from 38ff472 to 0865869 Compare November 28, 2016 12:35

mutantcornholio mentioned this pull request Jun 11, 2017

Fixed cluster inspect port logic #13619

Closed

3 tasks

mutantcornholio closed this Jun 11, 2017

mutantcornholio deleted the cluster-debug-port-fix branch June 18, 2017 11:53

mutantcornholio mentioned this pull request Jun 18, 2017

Added inspect port overriding in cluster #13761

Closed

4 tasks

mutantcornholio mentioned this pull request Jul 8, 2017

cluster: overriding inspector port #14140

Merged

4 tasks

MylesBorins removed the lts-watch-v6.x label Jul 14, 2017


		console.log('forked worker should have --debug-port, with offset = 2');
		console.log('debugging with --debug=[ipv6addr]:port format should also work');

		}).on('exit', checkExitCode);

		console.log('debugging with --debug=ipv4addr:port format should also work');

cluster: fixing debug port logic for forking workers #9659

cluster: fixing debug port logic for forking workers #9659

Conversation

mutantcornholio commented Nov 17, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

Fishrock123 commented Nov 17, 2016

mscdex Nov 17, 2016 • edited Loading

Choose a reason for hiding this comment

mutantcornholio Nov 17, 2016 • 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

Trott Nov 17, 2016 • 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

mutantcornholio commented Nov 17, 2016

mscdex commented Nov 17, 2016

mscdex commented Nov 17, 2016

mutantcornholio commented Nov 18, 2016

mscdex commented Nov 18, 2016

mutantcornholio commented Nov 18, 2016

mscdex commented Nov 18, 2016 • edited Loading

mutantcornholio commented Nov 18, 2016

mutantcornholio commented Nov 22, 2016

mutantcornholio commented Nov 25, 2016

mscdex commented Nov 25, 2016

mscdex commented Nov 25, 2016

mutantcornholio commented Nov 25, 2016

Trott commented Nov 26, 2016

cjihrig commented Nov 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mutantcornholio commented Nov 28, 2016

mutantcornholio commented Dec 6, 2016

mutantcornholio commented Jun 8, 2017 • edited Loading

refack commented Jun 8, 2017

mutantcornholio commented Jun 8, 2017

refack commented Jun 8, 2017

mutantcornholio commented Jun 11, 2017

mutantcornholio commented Nov 17, 2016 •

edited

Loading

mscdex Nov 17, 2016 •

edited

Loading

mutantcornholio Nov 17, 2016 •

edited

Loading

Trott Nov 17, 2016 •

edited

Loading

mscdex commented Nov 18, 2016 •

edited

Loading

mutantcornholio commented Jun 8, 2017 •

edited

Loading