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

sameer-coder · 2018-03-06T09:04:22Z

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

Checklist

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

Affected core subsystem(s)

cluster

benjamingr · 2018-03-06T09:37:01Z

Thanks for doing this!

@nodejs/v8-inspector @nodejs/cluster

richardlau

Thanks for submitting this. Left a few comments. The test probably should be spawning more than one worker to properly test #19026 just like the test added in #13619.

richardlau · 2018-03-06T09:49:48Z

lib/internal/cluster/master.js

@@ -102,11 +102,14 @@ function createWorkerProcess(id, env) {
  const workerEnv = util._extend({}, process.env);
  const execArgv = cluster.settings.execArgv.slice();
  const debugArgRegex = /--inspect(?:-brk|-port)?|--debug-port/;
+  const nodeOptions = process.env.NODE_OPTIONS ?
+    process.env.NODE_OPTIONS.split(' ') : [];


There's no need to split NODE_OPTIONS. Later on match() can be called on the whole string.

@richardlau I have removed the split and doing a direct match. Please review again.

richardlau · 2018-03-06T09:52:02Z

test/parallel/test-inspect-support-for-node_options.js

+
+if (process.config.variables.node_without_node_options)
+  common.skip('missing NODE_OPTIONS support');
+


Could you also call common.skipIfInspectorDisabled()?

sameer-coder · 2018-03-06T13:23:23Z

@richardlau Thanks for your comments. I am re-writing the test to spawn more workers like how its done in test-inspector-port-cluster.js test. Will get back to you.

cjihrig

LGTM with nits addressed.

cjihrig · 2018-03-06T13:35:46Z

test/parallel/test-inspect-support-for-node_options.js

+    cluster.fork(env);
+
+    cluster.on('online', (worker) => {
+      process.exit(0);


Please don't call process.exit(). Instead, disconnect the worker and let the test exit naturally.

cjihrig · 2018-03-06T13:36:09Z

test/parallel/test-inspect-support-for-node_options.js

+
+function checkForInspectSupport(flag) {
+  const env = Object.assign({}, process.env, { NODE_OPTIONS: flag });
+  const o = JSON.stringify(flag);


Could you use a more meaningful name than o?

sameer-coder · 2018-03-07T09:23:53Z

@richardlau - I have updated the code to match the entire options string and also added common.skipIfInspectorDisabled(). Also, I have changed the test so that it spawns multiple workers now.

@cjihrig - I have made changes as per your comments to use worker disconnect instead of process.exit()

jasnell · 2018-03-07T15:01:28Z

test/parallel/test-inspect-support-for-node_options.js

    });

    cluster.on('exit', (worker, code, signal) => {
-      assert.fail(`For ${o}, failed to start a cluster with inspect option`);
+      if (worker.exitedAfterDisconnect === false) {
+        assert.fail(`For ${nodeOptions}, failed to start cluster\


Nit: please avoid using multiline template strings.

@jasnell I have changed the code. No longer using multiline string. Please review again.

sameer-coder · 2018-03-12T07:06:58Z

Did anyone get a chance to review this again?
I have updated the code as per all the suggestions

benjamingr · 2018-03-12T09:39:53Z

Added the author-ready tag - should land soon :)

Trott · 2018-03-13T23:49:39Z

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

richardlau

Sorry, thought I'd re-reviewed this.

I think we also moved some of the inspector tests to sequential because they were otherwise competing for port 9229 when run in parallel.

richardlau · 2018-03-14T18:59:49Z

test/parallel/test-inspect-support-for-node_options.js

+const common = require('../common');
+const cluster = require('cluster');
+const assert = require('assert');
+const numCPUs = require('os').cpus().length;


This needs to be a minimum of 2 to properly test #19026 (i.e. if numCPUs is 1 then the test will pass without the corresponding lib/internal/cluster/master.js changes).

To clarify, by "this" I mean the number of workers spawned in the subsequent for loop.

@richardlau you are right. I am no longer using numCPUs and directly spawning multiple workers.

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

@jasnell

…or-node_options Changed multi-line template string in test-inspect-support-for-node_options as per feedback from @jasnell. PR nodejs#19165 Fixes : nodejs#19026

…ptions Removed numCpus and hardcoded to spawn multiple workers in test-inspect-support-for-node_options. PR nodejs#19165 Fixes : nodejs#19026

richardlau · 2018-03-16T08:31:24Z

test/parallel/test-inspect-support-for-node_options.js

+    });
+
+    cluster.on('exit', (worker, code, signal) => {
+      if (worker.exitedAfterDisconnect === false) {


Should this be an assert rather than if-fail?

Does this look ok?

cluster.on('exit', common.mustCall((worker, code, signal) => { const errMsg = `For NODE_OPTIONS ${nodeOptions}, failed to start cluster`; assert.strictEqual(worker.exitedAfterDisconnect, true, errMsg); }, 2));

richardlau · 2018-03-16T08:32:24Z

test/parallel/test-inspect-support-for-node_options.js

+      worker.disconnect();
+    });
+
+    cluster.on('exit', (worker, code, signal) => {


Wrap the callback in common.mustCall.

…rt-for-node_options Using common.mustCall to wrap callback and added assert to replace if-fail in test-inspect-support-for-node_options. PR nodejs#19165 Fixes : nodejs#19026

richardlau · 2018-03-16T17:33:53Z

test/parallel/test-inspect-support-for-node_options.js

+    cluster.on('exit', common.mustCall((worker, code, signal) => {
+      const errMsg = `For NODE_OPTIONS ${nodeOptions}, failed to start cluster`;
+      assert.strictEqual(worker.exitedAfterDisconnect, true, errMsg);
+    }, 2));


Tiny nit: maybe make the 2 here and in the for loop that forks the workers a const since they should match?

@richardlau I have added the const and replaced the 2 both in the for loop and callback.

const numWorkers added to specify number of workers spawned in test-inspect-support-for-node_options. PR nodejs#19165 Fixes : nodejs#19026

richardlau · 2018-03-17T06:15:24Z

One more CI: https://ci.nodejs.org/job/node-test-pull-request/13716/

richardlau · 2018-03-21T18:27:09Z

Windows failure is #19263 and unrelated to this PR.

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>

richardlau · 2018-03-21T19:29:24Z

Landed in 9b34ea6.

richardlau · 2018-03-21T19:30:06Z

Congratulations @sameer-coder on your first contribution to Node.js. 🎉

sameer-coder · 2018-03-22T06:11:58Z

Thank you very much @richardlau for the opportunity to pick this up and then helping with the code.
I am absolutely thrilled at my first contribution and look forward to contributing more!

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>

Notable changes: * cluster: - Add support for `NODE_OPTIONS="--inspect"` (Sameer Srivastava) #19165 * crypto: - Expose the public key of a certificate (Hannes Magnusson) #17690 * n-api: - Add `napi_fatal_exception` to trigger an `uncaughtException` in JavaScript (Mathias Buus) #19337 * path: - Fix regression in `posix.normalize` (Michaël Zasso) #19520 * stream: - Improve stream creation performance (Brian White) #19401 * Added new collaborators - [BethGriggs](https://github.com/BethGriggs) Beth Griggs

This is a security release. All Node.js users should consult the security release summary at: https://nodejs.org/en/blog/vulnerability/march-2018-security-releases/ for details on patched vulnerabilities. Fixes for the following CVEs are included in this release: * CVE-2018-7158 * CVE-2018-7159 * CVE-2018-7160 Notable changes: * Upgrade to OpenSSL 1.0.2o: Does not contain any security fixes that are known to impact Node.js. * **Fix for inspector DNS rebinding vulnerability (CVE-2018-7160)**: A malicious website could use a DNS rebinding attack to trick a web browser to bypass same-origin-policy checks and allow HTTP connections to localhost or to hosts on the local network, potentially to an open inspector port as a debugger, therefore gaining full code execution access. The inspector now only allows connections that have a browser `Host` value of `localhost` or `localhost6`. * **Fix for `'path'` module regular expression denial of service (CVE-2018-7158)**: A regular expression used for parsing POSIX an Windows paths could be used to cause a denial of service if an attacker were able to have a specially crafted path string passed through one of the impacted `'path'` module functions. * **Reject spaces in HTTP `Content-Length` header values (CVE-2018-7159)**: The Node.js HTTP parser allowed for spaces inside `Content-Length` header values. Such values now lead to rejected connections in the same way as non-numeric values. * **Update root certificates**: 5 additional root certificates have been added to the Node.js binary and 30 have been removed. * cluster: - Add support for `NODE_OPTIONS="--inspect"` (Sameer Srivastava) #19165 * crypto: - Expose the public key of a certificate (Hannes Magnusson) #17690 * n-api: - Add `napi_fatal_exception` to trigger an `uncaughtException` in JavaScript (Mathias Buus) #19337 * path: - Fix regression in `posix.normalize` (Michaël Zasso) #19520 * stream: - Improve stream creation performance (Brian White) #19401 * Added new collaborators - [BethGriggs](https://github.com/BethGriggs) Beth Griggs PR-URL: https://github.com/nodejs-private/node-private/pull/111

nodejs-github-bot added the cluster Issues and PRs related to the cluster subsystem. label Mar 6, 2018

sameer-coder mentioned this pull request Mar 6, 2018

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

Closed

sameer-coder changed the title ~~cluster:~~ cluster: Add support to cluster to work with `NODE_OPTIONS="--inspect" Mar 6, 2018

sameer-coder force-pushed the sameer branch from 00a994b to 3bd35db Compare March 6, 2018 09:27

sameer-coder closed this Mar 6, 2018

sameer-coder reopened this Mar 6, 2018

benjamingr added the inspector Issues and PRs related to the V8 inspector protocol label Mar 6, 2018

richardlau reviewed Mar 6, 2018

View reviewed changes

cjihrig approved these changes Mar 6, 2018

View reviewed changes

jasnell reviewed Mar 7, 2018

View reviewed changes

benjamingr added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 12, 2018

jasnell approved these changes Mar 12, 2018

View reviewed changes

richardlau reviewed Mar 14, 2018

View reviewed changes

sameer-coder added 4 commits March 16, 2018 10:36

cluster: Changed multi-line template string in test-inspect-support-f…

eb8f567

…or-node_options Changed multi-line template string in test-inspect-support-for-node_options as per feedback from @jasnell. PR nodejs#19165 Fixes : nodejs#19026

cluster: spawning multiple workers in test-inspect-support-for-node_o…

ff81da0

…ptions Removed numCpus and hardcoded to spawn multiple workers in test-inspect-support-for-node_options. PR nodejs#19165 Fixes : nodejs#19026

sameer-coder force-pushed the sameer branch from 04c1931 to ff81da0 Compare March 16, 2018 05:17

richardlau reviewed Mar 16, 2018

View reviewed changes

cluster: Using common.mustCall to wrap callback in test-inspect-suppo…

d2cf4a4

…rt-for-node_options Using common.mustCall to wrap callback and added assert to replace if-fail in test-inspect-support-for-node_options. PR nodejs#19165 Fixes : nodejs#19026

richardlau approved these changes Mar 16, 2018

View reviewed changes

cluster: const numWorkers added in test-inspect-support-for-node_options

907b8ff

const numWorkers added to specify number of workers spawned in test-inspect-support-for-node_options. PR nodejs#19165 Fixes : nodejs#19026

richardlau closed this Mar 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sameer-coder commented Mar 6, 2018

benjamingr commented Mar 6, 2018

richardlau left a comment

richardlau Mar 6, 2018

sameer-coder Mar 8, 2018

richardlau Mar 6, 2018

sameer-coder Mar 8, 2018

sameer-coder commented Mar 6, 2018

cjihrig left a comment

cjihrig Mar 6, 2018

cjihrig Mar 6, 2018

sameer-coder commented Mar 7, 2018

jasnell Mar 7, 2018

sameer-coder Mar 8, 2018

sameer-coder commented Mar 12, 2018 •

edited

Loading

benjamingr commented Mar 12, 2018

Trott commented Mar 13, 2018

richardlau left a comment

richardlau Mar 14, 2018

richardlau Mar 14, 2018

sameer-coder Mar 16, 2018

richardlau Mar 16, 2018

sameer-coder Mar 16, 2018 •

edited

Loading

richardlau Mar 16, 2018

sameer-coder Mar 16, 2018

richardlau Mar 16, 2018

sameer-coder Mar 17, 2018

richardlau commented Mar 17, 2018

richardlau commented Mar 21, 2018

richardlau commented Mar 21, 2018

richardlau commented Mar 21, 2018

sameer-coder commented Mar 22, 2018


		if (process.config.variables.node_without_node_options)
		common.skip('missing NODE_OPTIONS support');

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

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

Conversation

sameer-coder commented Mar 6, 2018

Checklist

Affected core subsystem(s)

benjamingr commented Mar 6, 2018

richardlau left a 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

sameer-coder commented Mar 6, 2018

cjihrig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sameer-coder commented Mar 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sameer-coder commented Mar 12, 2018 • edited Loading

benjamingr commented Mar 12, 2018

Trott commented Mar 13, 2018

richardlau left a 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

sameer-coder Mar 16, 2018 • 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

richardlau commented Mar 17, 2018

richardlau commented Mar 21, 2018

richardlau commented Mar 21, 2018

richardlau commented Mar 21, 2018

sameer-coder commented Mar 22, 2018

sameer-coder commented Mar 12, 2018 •

edited

Loading

sameer-coder Mar 16, 2018 •

edited

Loading