Skip to content
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

cluster: fix debugging port collision #12395

Closed
wants to merge 1 commit into from

Conversation

Ajido
Copy link
Contributor

@Ajido Ajido commented Apr 13, 2017

Fixing an issue that giving a debugging option including a hostname
stop the cluster worker by EADDRINUSE.

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)

src, cluster, test

@@ -115,7 +115,7 @@ function createWorkerProcess(id, env) {

for (var i = 0; i < execArgv.length; i++) {
const match = execArgv[i].match(
/^(--inspect|--inspect-(brk|port)|--debug|--debug-(brk|port))(=\d+)?$/
/^(--inspect|--inspect-(brk|port)|--debug|--debug-(brk|port))(=.*)?$/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will match flags that specify a host, but it seems like the host would then be lost when the flag is rewritten for the worker process a few lines down.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I'm thinking about addding process._debugHostname to avoid complex regex, what do you think?

diff --git a/lib/internal/cluster/master.js b/lib/internal/cluster/master.js
index c7b06b8..2a7379b 100644
--- a/lib/internal/cluster/master.js
+++ b/lib/internal/cluster/master.js
@@ -124,7 +124,7 @@ function createWorkerProcess(id, env) {
         ++debugPortOffset;
       }

-      execArgv[i] = match[1] + '=' + debugPort;
+      execArgv[i] = match[1] + '=' + process._debugHostname + ':' + debugPort;
     }
   }

diff --git a/src/node.cc b/src/node.cc
index c241f73..f091ac7 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -3361,6 +3361,8 @@ void SetupProcessObject(Environment* env,
                              DebugPortSetter,
                              env->as_external()).FromJust());

+  READONLY_PROPERTY(process, "_debugHostname", OneByteString(env->isolate(), debug_options.host_name().c_str()));
+
   // define various internal methods
   env->SetMethod(process,
                  "_startProfilerIdleNotifier",

@Fishrock123 Fishrock123 added cluster Issues and PRs related to the cluster subsystem. inspector Issues and PRs related to the V8 inspector protocol labels Apr 13, 2017
Fixing an issue that giving a debugging option including a hostname
stop the cluster worker by EADDRINUSE.
@Ajido Ajido force-pushed the cluster-debugging-port-fix branch from 1029159 to ba90b6e Compare May 2, 2017 14:13
@bnoordhuis
Copy link
Member

Superseded by #13619. Thanks for the pull request though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cluster Issues and PRs related to the cluster subsystem. inspector Issues and PRs related to the V8 inspector protocol
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants