cluster: overriding inspector port #14140

mutantcornholio · 2017-07-08T22:55:55Z

As suggested in #13761 (comment), I've added new option inspectPort to cluster.settings.

This allows to override inspect port incrementing behavior.

Two main configurations that need this:

One master and one worker. Master runs without inspector. Worker runs with inspector on specified port.
Cluster with many workers, all process with --inspect=0. If we want whole cluster run with inspector and don't want to catch port collisions, we run master with --inspect=0 and use setupMaster(inspectPort: 0) to override port increment.

Fixes: #8495
Fixes: #12941
Ref: #9659
Ref: #13761

_{[refack] rearranged references so metadata generated better detects them}

Checklist

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

Affected core subsystem(s)

cluster

mutantcornholio · 2017-07-08T22:57:26Z

cc/ @refack @Trott @mcollina

refack

Beautifull

refack · 2017-07-08T23:35:21Z

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

mutantcornholio · 2017-07-08T23:43:38Z

Running JS linter...
./node tools/jslint.js -j 2 -f tap -o test-eslint.tap \
	benchmark doc lib test tools
gmake: *** [Makefile:876: jslint-ci] Error 1

Is something's wrong with CI?

joaocgreis · 2017-07-09T01:42:08Z

I aborted the CI run becuase one of the windows workers was stuck, unrelated to this PR.

@mutantcornholio about the linter, there is indeed an error here, but the CI job does not show details:

C:\Users\Administrator\Desktop\node\test\inspector\test-inspector-port-cluster.js
  107:53  error  Strings must use singlequote  quotes

refack · 2017-07-09T03:10:30Z

Is something's wrong with CI?

Yes and no. It's a known limitation that currently Jenkins can't parse the linter's output nodejs/build#720

refack · 2017-07-09T03:12:48Z

@mutantcornholio if you go the the CI runner just after the job ended you can view the TAP output:

not ok 20 - /usr/home/iojs/build/workspace/node-test-linter/test/inspector/test-inspector-port-cluster.js
  ---
  message: Strings must use singlequote.
  severity: error
  data:
    line: 107
    column: 53
    ruleId: quotes
  ...

Trott · 2017-07-09T03:13:20Z

/cc @bnoordhuis @cjihrig

mutantcornholio · 2017-07-09T12:05:08Z

Fixed codestyle, thanks.

refack · 2017-07-09T12:41:06Z

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

mcollina

LGTM as semver-minor. However, I would say do not backport to 6, and just land it in 8.

mcollina · 2017-07-10T10:29:55Z

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


  for (const worker of workers) {
+    if (clusterSettings) {
+      if (clusterSettings.inspectPort === 'addTwo') {
+        clusterSettings.inspectPort = () => { return debugPort += 2; };


can you please add a common.mustCall here?

cjihrig

Can you please split all of the unrelated changes into separate commits or PRs.

cjihrig · 2017-07-10T13:14:51Z

lib/internal/cluster/master.js

-    debugPortOffset++;
+    let inspectPort;
+    if (cluster.settings.inspectPort) {
+      if (typeof cluster.settings.inspectPort === 'function') {


Style nit: no curly braces for single line if statements.

mutantcornholio · 2017-07-10T13:17:22Z

Can you please split all of the unrelated changes into separate commits or PRs.

You mean style changes in docs? Or is there anything else?

cjihrig · 2017-07-10T13:23:54Z

I meant the unrelated documentation changes. The change in offset from 10 to 5 is probably not needed either.

mutantcornholio · 2017-07-10T13:32:01Z

The change in offset from 10 to 5 is probably not needed either.

It is needed because I've added some tests and reached 100 ports "limit" per test file.

Would it be sufficient if I break the changes into several commits, but it all will stay in one PR?

cjihrig · 2017-07-10T13:33:52Z

Different commits in the same PR is OK. In the future, I'd say separate PRs though.

refack · 2017-07-10T13:40:18Z

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

+  const debugOptions = process.binding('config').debugOptions;
+
+  if (+expectedPort) {
+    assert.strictEqual(process.debugPort, +expectedPort);


semantic nit: I'd rather you test

if ('expectedPort' in process.env) assert.strictEqual(process.debugPort, Number(expectedPort));

And the next test.

refack · 2017-07-10T13:56:56Z

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

+
+  port = debuggerPort + offset++ * 5;
+
+  spawnMaster({


Could you add a test for the trivial case:

{ execArgv: ['--inspect'], clusterSettings: {inspectPort: port + 2}, workers: [{expectedPort: port + 2}] }

had to rewrite tests a little bit for this, but made it.

mutantcornholio · 2017-07-10T20:02:20Z

@mcollina @refack @cjihrig done!

refack · 2017-07-14T23:18:10Z

freeBSD fail is unrelated - #14241
plinux fail is known - #14206

Landed in b430053 592b0ed ca5b0c4

Long ride #9659 -> #13761 -> here

refack · 2017-07-14T23:42:54Z

@mutantcornholio don't you want your real name in the commit logs? Too late for this one, but if when you do another PR here's the guide https://help.github.com/articles/setting-your-username-in-git/

mutantcornholio · 2017-07-15T00:31:05Z

I kinda got used to Cornholio <[email protected]>, but I'll think about it :)

refack · 2017-07-15T04:19:16Z

I kinda got used to Cornholio [email protected], but I'll think about it :)

For a long time I wanted to ask if you need T.P.?

- Add missing `changes:` entry - Use full sentences - Use proper indentation Ref: nodejs#14140 PR-URL: nodejs#14349 Reviewed-By: Colin Ihrig <[email protected]>

* 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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

- Add missing `changes:` entry - Use full sentences - Use proper indentation Ref: #14140 PR-URL: #14349 Reviewed-By: Colin Ihrig <[email protected]>

Notable changes: * **Async Hooks** * Multiple improvements to Promise support in `async_hooks` have been made. * **Build** * The compiler version requirement to build Node with GCC has been raised to GCC 4.9.4. [[`820b011ed6`](820b011ed6)] [#13466](#13466) * **Cluster** * Users now have more fine-grained control over the inspector port used by individual cluster workers. Previously, cluster workers would simply increment from the master's debug port. [[`dfc46e262a`](dfc46e262a)] [#14140](#14140) * **DNS** * The server used for DNS queries can now use a custom port. [[`ebe7bb29aa`](ebe7bb29aa)] [#13723](#13723) * Support for `dns.resolveAny()` has been added. [[`6e30e2558e`](6e30e2558e)] [#13137](#13137) * **npm** * The `npm` CLI has been updated to version 5.3.0. In particular, it now comes with the `npx` binary, which is also shipped with Node. [[`dc3f6b9ac1`](dc3f6b9ac1)] [#14235](#14235) * `npm` Changelogs: - [v5.0.4](https://github.com/npm/npm/releases/tag/v5.0.4) - [v5.1.0](https://github.com/npm/npm/releases/tag/v5.1.0) - [v5.2.0](https://github.com/npm/npm/releases/tag/v5.2.0) - [v5.3.0](https://github.com/npm/npm/releases/tag/v5.3.0) PR-URL: #13744

- Add missing `changes:` entry - Use full sentences - Use proper indentation Ref: #14140 PR-URL: #14349 Reviewed-By: Colin Ihrig <[email protected]>

@addaleax

Big thanks to @addaleax who prepared the vast majority of this release. Notable changes: * **Async Hooks** * Multiple improvements to Promise support in `async_hooks` have been made. * **Build** * The compiler version requirement to build Node with GCC has been raised to GCC 4.9.4. [[`820b011ed6`](nodejs@820b011ed6)] [nodejs#13466](nodejs#13466) * **Cluster** * Users now have more fine-grained control over the inspector port used by individual cluster workers. Previously, cluster workers would simply increment from the master's debug port. [[`dfc46e262a`](nodejs@dfc46e262a)] [nodejs#14140](nodejs#14140) * **DNS** * The server used for DNS queries can now use a custom port. [[`ebe7bb29aa`](nodejs@ebe7bb29aa)] [nodejs#13723](nodejs#13723) * Support for `dns.resolveAny()` has been added. [[`6e30e2558e`](nodejs@6e30e2558e)] [nodejs#13137](nodejs#13137) * **npm** * The `npm` CLI has been updated to version 5.3.0. In particular, it now comes with the `npx` binary, which is also shipped with Node. [[`dc3f6b9ac1`](nodejs@dc3f6b9ac1)] [nodejs#14235](nodejs#14235) * `npm` Changelogs: - [v5.0.4](https://github.com/npm/npm/releases/tag/v5.0.4) - [v5.1.0](https://github.com/npm/npm/releases/tag/v5.1.0) - [v5.2.0](https://github.com/npm/npm/releases/tag/v5.2.0) - [v5.3.0](https://github.com/npm/npm/releases/tag/v5.3.0) PR-URL: nodejs#13744

* 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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

- Add missing `changes:` entry - Use full sentences - Use proper indentation Ref: #14140 PR-URL: #14349 Reviewed-By: Colin Ihrig <[email protected]>

@addaleax

Big thanks to @addaleax who prepared the vast majority of this release. Notable changes: * **Async Hooks** * Multiple improvements to Promise support in `async_hooks` have been made. * **Build** * The compiler version requirement to build Node with GCC has been raised to GCC 4.9.4. [[`820b011ed6`](820b011ed6)] [#13466](#13466) * **Cluster** * Users now have more fine-grained control over the inspector port used by individual cluster workers. Previously, cluster workers would simply increment from the master's debug port. [[`dfc46e262a`](dfc46e262a)] [#14140](#14140) * **DNS** * The server used for DNS queries can now use a custom port. [[`ebe7bb29aa`](ebe7bb29aa)] [#13723](#13723) * Support for `dns.resolveAny()` has been added. [[`6e30e2558e`](6e30e2558e)] [#13137](#13137) * **npm** * The `npm` CLI has been updated to version 5.3.0. In particular, it now comes with the `npx` binary, which is also shipped with Node. [[`dc3f6b9ac1`](dc3f6b9ac1)] [#14235](#14235) * `npm` Changelogs: - [v5.0.4](https://github.com/npm/npm/releases/tag/v5.0.4) - [v5.1.0](https://github.com/npm/npm/releases/tag/v5.1.0) - [v5.2.0](https://github.com/npm/npm/releases/tag/v5.2.0) - [v5.3.0](https://github.com/npm/npm/releases/tag/v5.3.0) PR-URL: #13744

@addaleax

Big thanks to @addaleax who prepared the vast majority of this release. Notable changes: * **Async Hooks** * Multiple improvements to Promise support in `async_hooks` have been made. * **Build** * The compiler version requirement to build Node with GCC has been raised to GCC 4.9.4. [[`820b011ed6`](820b011ed6)] [#13466](#13466) * **Cluster** * Users now have more fine-grained control over the inspector port used by individual cluster workers. Previously, cluster workers would simply increment from the master's debug port. [[`dfc46e262a`](dfc46e262a)] [#14140](#14140) * **DNS** * The server used for DNS queries can now use a custom port. [[`ebe7bb29aa`](ebe7bb29aa)] [#13723](#13723) * Support for `dns.resolveAny()` has been added. [[`6e30e2558e`](6e30e2558e)] [#13137](#13137) * **npm** * The `npm` CLI has been updated to version 5.3.0. In particular, it now comes with the `npx` binary, which is also shipped with Node. [[`dc3f6b9ac1`](dc3f6b9ac1)] [#14235](#14235) * `npm` Changelogs: - [v5.0.4](https://github.com/npm/npm/releases/tag/v5.0.4) - [v5.1.0](https://github.com/npm/npm/releases/tag/v5.1.0) - [v5.2.0](https://github.com/npm/npm/releases/tag/v5.2.0) - [v5.3.0](https://github.com/npm/npm/releases/tag/v5.3.0) PR-URL: #13744

nodejs-github-bot added the cluster Issues and PRs related to the cluster subsystem. label Jul 8, 2017

mutantcornholio mentioned this pull request Jul 8, 2017

Added inspect port overriding in cluster #13761

Closed

4 tasks

refack approved these changes Jul 8, 2017

View reviewed changes

joaocgreis mentioned this pull request Jul 9, 2017

Now: Jenkins is in a weird mood nodejs/build#761

Closed

mutantcornholio force-pushed the cluster-inspect-override-1 branch from 1d68e76 to 71bad16 Compare July 9, 2017 12:01

mcollina approved these changes Jul 10, 2017

View reviewed changes

mcollina added dont-land-on-v4.x semver-minor PRs that contain new features and should be released in the next minor version. labels Jul 10, 2017

cjihrig previously requested changes Jul 10, 2017

View reviewed changes

cjihrig reviewed Jul 10, 2017

View reviewed changes

refack reviewed Jul 10, 2017

View reviewed changes

mutantcornholio force-pushed the cluster-inspect-override-1 branch 2 times, most recently from 6c2fe24 to 17a1611 Compare July 10, 2017 20:02

refack force-pushed the cluster-inspect-override-1 branch from b20864c to b430053 Compare July 14, 2017 23:16

refack merged commit b430053 into nodejs:master Jul 14, 2017

bnoordhuis mentioned this pull request Jul 17, 2017

Feature request: API for debugging node child processes #9621

Closed

addaleax mentioned this pull request Jul 18, 2017

doc: fix inspectPort documentation in cluster.md #14349

Merged

addaleax mentioned this pull request Jul 18, 2017

v8.2.0 proposal #13744

Merged

addaleax added a commit that referenced this pull request Jul 18, 2017

doc: fix inspectPort documentation in cluster.md

7df10f5

- Add missing `changes:` entry - Use full sentences - Use proper indentation Ref: #14140 PR-URL: #14349 Reviewed-By: Colin Ihrig <[email protected]>

addaleax added a commit that referenced this pull request Jul 18, 2017

doc: fix inspectPort documentation in cluster.md

e9491f1

- Add missing `changes:` entry - Use full sentences - Use proper indentation Ref: #14140 PR-URL: #14349 Reviewed-By: Colin Ihrig <[email protected]>

Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017

doc: fix inspectPort documentation in cluster.md

64867a9

- Add missing `changes:` entry - Use full sentences - Use proper indentation Ref: #14140 PR-URL: #14349 Reviewed-By: Colin Ihrig <[email protected]>

nazrhyn mentioned this pull request Aug 9, 2018

cluster: forked children eagerly ignore/replace inspect port from cluster settings #21853

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cluster: overriding inspector port #14140

cluster: overriding inspector port #14140

mutantcornholio commented Jul 8, 2017 •

edited by refack

Loading

mutantcornholio commented Jul 8, 2017

refack left a comment

refack commented Jul 8, 2017

mutantcornholio commented Jul 8, 2017

joaocgreis commented Jul 9, 2017

refack commented Jul 9, 2017

refack commented Jul 9, 2017

Trott commented Jul 9, 2017

mutantcornholio commented Jul 9, 2017

refack commented Jul 9, 2017

mcollina left a comment

mcollina Jul 10, 2017

cjihrig left a comment

cjihrig Jul 10, 2017

mutantcornholio commented Jul 10, 2017

cjihrig commented Jul 10, 2017

mutantcornholio commented Jul 10, 2017

cjihrig commented Jul 10, 2017

refack Jul 10, 2017

refack Jul 10, 2017 •

edited

Loading

mutantcornholio Jul 10, 2017

mutantcornholio commented Jul 10, 2017

refack commented Jul 14, 2017

refack commented Jul 14, 2017

mutantcornholio commented Jul 15, 2017

refack commented Jul 15, 2017

cluster: overriding inspector port #14140

cluster: overriding inspector port #14140

Conversation

mutantcornholio commented Jul 8, 2017 • edited by refack Loading

Checklist

Affected core subsystem(s)

mutantcornholio commented Jul 8, 2017

refack left a comment

Choose a reason for hiding this comment

refack commented Jul 8, 2017

mutantcornholio commented Jul 8, 2017

joaocgreis commented Jul 9, 2017

refack commented Jul 9, 2017

refack commented Jul 9, 2017

Trott commented Jul 9, 2017

mutantcornholio commented Jul 9, 2017

refack commented Jul 9, 2017

mcollina left a comment

Choose a reason for hiding this comment

mcollina Jul 10, 2017

Choose a reason for hiding this comment

cjihrig left a comment

Choose a reason for hiding this comment

cjihrig Jul 10, 2017

Choose a reason for hiding this comment

mutantcornholio commented Jul 10, 2017

cjihrig commented Jul 10, 2017

mutantcornholio commented Jul 10, 2017

cjihrig commented Jul 10, 2017

refack Jul 10, 2017

Choose a reason for hiding this comment

refack Jul 10, 2017 • edited Loading

Choose a reason for hiding this comment

mutantcornholio Jul 10, 2017

Choose a reason for hiding this comment

mutantcornholio commented Jul 10, 2017

refack commented Jul 14, 2017

refack commented Jul 14, 2017

mutantcornholio commented Jul 15, 2017

refack commented Jul 15, 2017

mutantcornholio commented Jul 8, 2017 •

edited by refack

Loading

refack Jul 10, 2017 •

edited

Loading