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

Instead of using a shared pointer of the entire debug option set, pass the parsed debug option to inspector classes by value because they are set once the CLI argument parsing is done. Add another shared pointer to HostPort being used by the inspector server, which is copied from the one in the debug options initially. The port of the shared HostPort is 9229 by default and can be specified as 0 initially but will be set to the actual port of the server once it starts listening. This makes the shared state clearer and makes it possible to use `require('internal/options')` in JS land to query the CLI options instead of using `process._breakFirstLine` and other underscored properties of `process` since we are now certain that these values should not be altered once the parsing is done and can be passed around in copies without locks. PR-URL: nodejs#24772 Reviewed-By: Anna Henningsen <[email protected]>
refack · Jan 10, 2019 · 28201e5 · 28201e5
1 parent f073cda
commit 28201e5
Show file tree

Hide file tree

Showing 15 changed files with 160 additions and 98 deletions.
diff --git a/src/env-inl.h b/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_;
 }

diff --git a/src/env.cc b/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.

diff --git a/src/env.h b/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;

diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc
@@ -669,8 +669,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) {
@@ -681,13 +682,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(),
@@ -699,13 +701,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(
@@ -725,8 +732,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;
   }
@@ -867,8 +873,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;

diff --git a/src/inspector_agent.h b/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;

diff --git a/src/inspector_io.cc b/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);
   }

diff --git a/src/inspector_io.h b/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_;
 };

diff --git a/src/inspector_js_api.cc b/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()) {

diff --git a/src/node.cc b/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.
   }