inspector: split the HostPort being used and the one parsed from CLI

src/env-inl.h

@@ -585,6 +585,10 @@ inline std::shared_ptr<EnvironmentOptions> Environment::options() {
   return options_;
 }
 
+inline std::shared_ptr<HostPort> Environment::inspector_host_port() {
+  return inspector_host_port_;
+}
+
 inline std::shared_ptr<PerIsolateOptions> IsolateData::options() {
   return options_;
 }

src/env.cc

@@ -5,6 +5,7 @@
 #include "node_file.h"
 #include "node_internals.h"
 #include "node_native_module.h"
+#include "node_options-inl.h"
 #include "node_platform.h"
 #include "node_worker.h"
 #include "tracing/agent.h"
@@ -192,7 +193,7 @@ Environment::Environment(IsolateData* isolate_data,
   // part of the per-Isolate option set, for which in turn the defaults are
   // part of the per-process option set.
   options_.reset(new EnvironmentOptions(*isolate_data->options()->per_env));
-  options_->debug_options.reset(new DebugOptions(*options_->debug_options));
+  inspector_host_port_.reset(new HostPort(options_->debug_options().host_port));
 
 #if HAVE_INSPECTOR
   // We can only create the inspector agent after having cloned the options.

src/env.h

@@ -911,6 +911,7 @@ class Environment {
                                  void* data);
 
   inline std::shared_ptr<EnvironmentOptions> options();
+  inline std::shared_ptr<HostPort> inspector_host_port();
 
  private:
   inline void CreateImmediate(native_immediate_callback cb,
@@ -942,6 +943,14 @@ class Environment {
   std::vector<double> destroy_async_id_list_;
 
   std::shared_ptr<EnvironmentOptions> options_;
+  // options_ contains debug options parsed from CLI arguments,
+  // while inspector_host_port_ stores the actual inspector host
+  // and port being used. For example the port is -1 by default
+  // and can be specified as 0 (meaning any port allocated when the
+  // server starts listening), but when the inspector server starts
+  // the inspector_host_port_->port() will be the actual port being
+  // used.
+  std::shared_ptr<HostPort> inspector_host_port_;
 
   uint32_t module_id_counter_ = 0;
   uint32_t script_id_counter_ = 0;

src/inspector_agent.cc

@@ -667,8 +667,9 @@ class NodeInspectorClient : public V8InspectorClient {
 };
 
 Agent::Agent(Environment* env)
-  : parent_env_(env),
-    debug_options_(env->options()->debug_options) {}
+    : parent_env_(env),
+      debug_options_(env->options()->debug_options()),
+      host_port_(env->inspector_host_port()) {}
 
 Agent::~Agent() {
   if (start_io_thread_async.data == this) {
@@ -679,13 +680,14 @@ Agent::~Agent() {
 }
 
 bool Agent::Start(const std::string& path,
-                  std::shared_ptr<DebugOptions> options,
+                  const DebugOptions& options,
+                  std::shared_ptr<HostPort> host_port,
                   bool is_main) {
-  if (options == nullptr) {
-    options = std::make_shared<DebugOptions>();
-  }
   path_ = path;
   debug_options_ = options;
+  CHECK_NE(host_port, nullptr);
+  host_port_ = host_port;
+
   client_ = std::make_shared<NodeInspectorClient>(parent_env_, is_main);
   if (parent_env_->is_main_thread()) {
     CHECK_EQ(0, uv_async_init(parent_env_->event_loop(),
@@ -697,13 +699,18 @@ bool Agent::Start(const std::string& path,
     StartDebugSignalHandler();
   }
 
-  bool wait_for_connect = options->wait_for_connect();
+  bool wait_for_connect = options.wait_for_connect();
   if (parent_handle_) {
     wait_for_connect = parent_handle_->WaitForConnect();
     parent_handle_->WorkerStarted(client_->getThreadHandle(), wait_for_connect);
-  } else if (!options->inspector_enabled || !StartIoThread()) {
+  } else if (!options.inspector_enabled || !StartIoThread()) {
     return false;
   }
+
+  // TODO(joyeecheung): we should not be using process as a global object
+  // to transport --inspect-brk. Instead, the JS land can get this through
+  // require('internal/options') since it should be set once CLI parsing
+  // is done.
   if (wait_for_connect) {
     HandleScope scope(parent_env_->isolate());
     parent_env_->process_object()->DefineOwnProperty(
@@ -723,8 +730,7 @@ bool Agent::StartIoThread() {
 
   CHECK_NOT_NULL(client_);
 
-  io_ = InspectorIo::Start(
-      client_->getThreadHandle(), path_, debug_options_);
+  io_ = InspectorIo::Start(client_->getThreadHandle(), path_, host_port_);
   if (io_ == nullptr) {
     return false;
   }
@@ -865,8 +871,7 @@ void Agent::ContextCreated(Local<Context> context, const ContextInfo& info) {
 }
 
 bool Agent::WillWaitForConnect() {
-  if (debug_options_->wait_for_connect())
-    return true;
+  if (debug_options_.wait_for_connect()) return true;
   if (parent_handle_)
     return parent_handle_->WaitForConnect();
   return false;

src/inspector_agent.h

@@ -11,7 +11,7 @@
 #error("This header can only be used when inspector is enabled")
 #endif
 
-#include "node_options.h"
+#include "node_options-inl.h"
 #include "node_persistent.h"
 #include "v8.h"
 
@@ -50,8 +50,9 @@ class Agent {
 
   // Create client_, may create io_ if option enabled
   bool Start(const std::string& path,
-             std::shared_ptr<DebugOptions> options,
-             bool is_worker);
+             const DebugOptions& options,
+             std::shared_ptr<HostPort> host_port,
+             bool is_main);
   // Stop and destroy io_
   void Stop();
 
@@ -104,7 +105,8 @@ class Agent {
   // Calls StartIoThread() from off the main thread.
   void RequestIoThreadStart();
 
-  std::shared_ptr<DebugOptions> options() { return debug_options_; }
+  const DebugOptions& options() { return debug_options_; }
+  std::shared_ptr<HostPort> host_port() { return host_port_; }
   void ContextCreated(v8::Local<v8::Context> context, const ContextInfo& info);
 
   // Interface for interacting with inspectors in worker threads
@@ -121,7 +123,13 @@ class Agent {
   std::unique_ptr<InspectorIo> io_;
   std::unique_ptr<ParentInspectorHandle> parent_handle_;
   std::string path_;
-  std::shared_ptr<DebugOptions> debug_options_;
+
+  // This is a copy of the debug options parsed from CLI in the Environment.
+  // Do not use the host_port in that, instead manipulate the shared host_port_
+  // pointer which is meant to store the actual host and port of the inspector
+  // server.
+  DebugOptions debug_options_;
+  std::shared_ptr<HostPort> host_port_;
 
   bool pending_enable_async_hook_ = false;
   bool pending_disable_async_hook_ = false;

src/inspector_io.cc

@@ -242,9 +242,9 @@ class InspectorIoDelegate: public node::inspector::SocketServerDelegate {
 std::unique_ptr<InspectorIo> InspectorIo::Start(
     std::shared_ptr<MainThreadHandle> main_thread,
     const std::string& path,
-    std::shared_ptr<DebugOptions> options) {
+    std::shared_ptr<HostPort> host_port) {
   auto io = std::unique_ptr<InspectorIo>(
-      new InspectorIo(main_thread, path, options));
+      new InspectorIo(main_thread, path, host_port));
   if (io->request_queue_->Expired()) {  // Thread is not running
     return nullptr;
   }
@@ -253,9 +253,12 @@ std::unique_ptr<InspectorIo> InspectorIo::Start(
 
 InspectorIo::InspectorIo(std::shared_ptr<MainThreadHandle> main_thread,
                          const std::string& path,
-                         std::shared_ptr<DebugOptions> options)
-                         : main_thread_(main_thread), options_(options),
-                           thread_(), script_name_(path), id_(GenerateID()) {
+                         std::shared_ptr<HostPort> host_port)
+    : main_thread_(main_thread),
+      host_port_(host_port),
+      thread_(),
+      script_name_(path),
+      id_(GenerateID()) {
   Mutex::ScopedLock scoped_lock(thread_start_lock_);
   CHECK_EQ(uv_thread_create(&thread_, InspectorIo::ThreadMain, this), 0);
   thread_start_condition_.Wait(scoped_lock);
@@ -287,16 +290,17 @@ void InspectorIo::ThreadMain() {
   std::unique_ptr<InspectorIoDelegate> delegate(
       new InspectorIoDelegate(queue, main_thread_, id_,
                               script_path, script_name_));
-  InspectorSocketServer server(std::move(delegate), &loop,
-                               options_->host().c_str(),
-                               options_->port());
+  InspectorSocketServer server(std::move(delegate),
+                               &loop,
+                               host_port_->host().c_str(),
+                               host_port_->port());
   request_queue_ = queue->handle();
   // Its lifetime is now that of the server delegate
   queue.reset();
   {
     Mutex::ScopedLock scoped_lock(thread_start_lock_);
     if (server.Start()) {
-      port_ = server.Port();
+      host_port_->set_port(server.Port());
     }
     thread_start_condition_.Broadcast(scoped_lock);
   }

src/inspector_io.h

@@ -46,21 +46,22 @@ class InspectorIo {
   // bool Start();
   // Returns empty pointer if thread was not started
   static std::unique_ptr<InspectorIo> Start(
-      std::shared_ptr<MainThreadHandle> main_thread, const std::string& path,
-      std::shared_ptr<DebugOptions> options);
+      std::shared_ptr<MainThreadHandle> main_thread,
+      const std::string& path,
+      std::shared_ptr<HostPort> host_port);
 
   // Will block till the transport thread shuts down
   ~InspectorIo();
 
   void StopAcceptingNewConnections();
-  const std::string& host() const { return options_->host(); }
-  int port() const { return port_; }
+  const std::string& host() const { return host_port_->host(); }
+  int port() const { return host_port_->port(); }
   std::vector<std::string> GetTargetIds() const;
 
  private:
   InspectorIo(std::shared_ptr<MainThreadHandle> handle,
               const std::string& path,
-              std::shared_ptr<DebugOptions> options);
+              std::shared_ptr<HostPort> host_port);
 
   // Wrapper for agent->ThreadMain()
   static void ThreadMain(void* agent);
@@ -74,7 +75,7 @@ class InspectorIo {
   // Used to post on a frontend interface thread, lives while the server is
   // running
   std::shared_ptr<RequestQueue> request_queue_;
-  std::shared_ptr<DebugOptions> options_;
+  std::shared_ptr<HostPort> host_port_;
 
   // The IO thread runs its own uv_loop to implement the TCP server off
   // the main thread.
@@ -84,7 +85,6 @@ class InspectorIo {
   Mutex thread_start_lock_;
   ConditionVariable thread_start_condition_;
   std::string script_name_;
-  int port_ = -1;
   // May be accessed from any thread
   const std::string id_;
 };

src/inspector_js_api.cc

@@ -243,12 +243,12 @@ void Open(const FunctionCallbackInfo<Value>& args) {
 
   if (args.Length() > 0 && args[0]->IsUint32()) {
     uint32_t port = args[0].As<Uint32>()->Value();
-    agent->options()->host_port.port = port;
+    agent->host_port()->set_port(static_cast<int>(port));
   }
 
   if (args.Length() > 1 && args[1]->IsString()) {
     Utf8Value host(env->isolate(), args[1].As<String>());
-    agent->options()->host_port.host_name = *host;
+    agent->host_port()->set_host(*host);
   }
 
   if (args.Length() > 2 && args[2]->IsBoolean()) {

src/node.cc

@@ -27,6 +27,7 @@
 #include "node_internals.h"
 #include "node_metadata.h"
 #include "node_native_module.h"
+#include "node_options-inl.h"
 #include "node_perf.h"
 #include "node_platform.h"
 #include "node_revert.h"
@@ -257,13 +258,15 @@ static struct {
   }
 
 #if HAVE_INSPECTOR
-  bool StartInspector(Environment* env, const char* script_path,
-                      std::shared_ptr<DebugOptions> options) {
+  bool StartInspector(Environment* env, const char* script_path) {
     // Inspector agent can't fail to start, but if it was configured to listen
     // right away on the websocket port and fails to bind/etc, this will return
     // false.
     return env->inspector_agent()->Start(
-        script_path == nullptr ? "" : script_path, options, true);
+        script_path == nullptr ? "" : script_path,
+        env->options()->debug_options(),
+        env->inspector_host_port(),
+        true);
   }
 
   bool InspectorStarted(Environment* env) {
@@ -304,8 +307,7 @@ static struct {
   void Dispose() {}
   void DrainVMTasks(Isolate* isolate) {}
   void CancelVMTasks(Isolate* isolate) {}
-  bool StartInspector(Environment* env, const char* script_path,
-                      const DebugOptions& options) {
+  bool StartInspector(Environment* env, const char* script_path) {
     env->ThrowError("Node compiled with NODE_USE_V8_PLATFORM=0");
     return true;
   }
@@ -1090,24 +1092,24 @@ void SetupProcessObject(Environment* env,
 
   // TODO(refack): move the following 4 to `node_config`
   // --inspect-brk
-  if (env->options()->debug_options->wait_for_connect()) {
+  if (env->options()->debug_options().wait_for_connect()) {
     READONLY_DONT_ENUM_PROPERTY(process,
                                 "_breakFirstLine", True(env->isolate()));
   }
 
-  if (env->options()->debug_options->break_node_first_line) {
+  if (env->options()->debug_options().break_node_first_line) {
     READONLY_DONT_ENUM_PROPERTY(process,
                                 "_breakNodeFirstLine", True(env->isolate()));
   }
 
   // --inspect --debug-brk
-  if (env->options()->debug_options->deprecated_invocation()) {
+  if (env->options()->debug_options().deprecated_invocation()) {
     READONLY_DONT_ENUM_PROPERTY(process,
                                 "_deprecatedDebugBrk", True(env->isolate()));
   }
 
   // --debug or, --debug-brk without --inspect
-  if (env->options()->debug_options->invalid_invocation()) {
+  if (env->options()->debug_options().invalid_invocation()) {
     READONLY_DONT_ENUM_PROPERTY(process,
                                 "_invalidDebug", True(env->isolate()));
   }
@@ -1255,7 +1257,7 @@ void LoadEnvironment(Environment* env) {
           ->GetFunction(context)
           .ToLocalChecked(),
       Boolean::New(isolate,
-                   env->options()->debug_options->break_node_first_line)};
+                   env->options()->debug_options().break_node_first_line)};
 
   MaybeLocal<Value> loader_exports;
   // Bootstrap internal loaders
@@ -1290,12 +1292,10 @@ void LoadEnvironment(Environment* env) {
   }
 }
 
-
-static void StartInspector(Environment* env, const char* path,
-                           std::shared_ptr<DebugOptions> debug_options) {
+static void StartInspector(Environment* env, const char* path) {
 #if HAVE_INSPECTOR
   CHECK(!env->inspector_agent()->IsListening());
-  v8_platform.StartInspector(env, path, debug_options);
+  v8_platform.StartInspector(env, path);
 #endif  // HAVE_INSPECTOR
 }
 
@@ -1938,9 +1938,9 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data,
   env.Start(args, exec_args, v8_is_profiling);
 
   const char* path = args.size() > 1 ? args[1].c_str() : nullptr;
-  StartInspector(&env, path, env.options()->debug_options);
+  StartInspector(&env, path);
 
-  if (env.options()->debug_options->inspector_enabled &&
+  if (env.options()->debug_options().inspector_enabled &&
       !v8_platform.InspectorStarted(&env)) {
     return 12;  // Signal internal error.
   }