src: reorganize inspector and diagnostics initialization

- Split the initialization of the inspector and other diagnostics into `Environment::InitializeInspector()` and `Environment::InitializeDiagnostics()` - these need to be reinitialized separately after snapshot deserialization. - Do not store worker url alongside the inspector parent handle, instead just get it from the handle. - Rename `Worker::profiler_idle_notifier_started_` to `Worker::start_profiler_idle_notifier_` because it stores the state inherited from the parent env to use for initializing itself.
nodejs · joyeecheung · May 20, 2019 · May 20, 2019 · May 20, 2019 · May 20, 2019
commit 6430100093be0dc805d029153e249ca0bbf8d4f8
diff --git a/src/env.cc b/src/env.cc
@@ -348,8 +348,6 @@ Environment::Environment(IsolateData* isolate_data,
   credentials::SafeGetenv("NODE_DEBUG_NATIVE", &debug_cats, this);
   set_debug_categories(debug_cats, true);
 
-  isolate()->GetHeapProfiler()->AddBuildEmbedderGraphCallback(
-      BuildEmbedderGraph, this);
   if (options_->no_force_async_hooks_checks) {
     async_hooks_.no_force_checks();
   }

diff --git a/src/env.h b/src/env.h
@@ -74,6 +74,10 @@ namespace profiler {
 class V8CoverageConnection;
 class V8CpuProfilerConnection;
 }  // namespace profiler
+
+namespace inspector {
+class ParentInspectorHandle;
+}
 #endif  // HAVE_INSPECTOR
 
 namespace worker {
@@ -795,6 +799,12 @@ class Environment : public MemoryRetainer {
   void MemoryInfo(MemoryTracker* tracker) const override;
 
   void CreateProperties();
+#if HAVE_INSPECTOR && NODE_USE_V8_PLATFORM
+  // If the environment is created for a worker, pass parent_handle and
+  // the ownership if transferred into the Environment.
+  int InitializeInspector(inspector::ParentInspectorHandle* parent_handle);
+#endif
+  void InitializeDiagnostics();
 
   inline size_t async_callback_scope_depth() const;
   inline void PushAsyncCallbackScope();

diff --git a/src/inspector/worker_inspector.h b/src/inspector/worker_inspector.h
@@ -60,6 +60,7 @@ class ParentInspectorHandle {
   bool WaitForConnect() {
     return wait_;
   }
+  const std::string& url() const { return url_; }
 
  private:
   int id_;

diff --git a/src/node.cc b/src/node.cc
@@ -46,6 +46,7 @@
 #endif
 
 #if HAVE_INSPECTOR
+#include "inspector_agent.h"
 #include "inspector_io.h"
 #endif
 
@@ -58,6 +59,10 @@
 #endif  // NODE_USE_V8_PLATFORM
 #include "v8-profiler.h"
 
+#if HAVE_INSPECTOR
+#include "inspector/worker_inspector.h"  // ParentInspectorHandle
+#endif
+
 #ifdef NODE_ENABLE_VTUNE_PROFILING
 #include "../deps/v8/src/third_party/vtune/v8-vtune.h"
 #endif
@@ -135,7 +140,6 @@ using v8::Local;
 using v8::Maybe;
 using v8::MaybeLocal;
 using v8::Object;
-using v8::Script;
 using v8::String;
 using v8::Undefined;
 using v8::V8;
@@ -234,24 +238,60 @@ MaybeLocal<Value> ExecuteBootstrapper(Environment* env,
   return scope.EscapeMaybe(result);
 }
 
+#if HAVE_INSPECTOR && NODE_USE_V8_PLATFORM
+int Environment::InitializeInspector(
+    inspector::ParentInspectorHandle* parent_handle) {
+  std::string inspector_path;
+  if (parent_handle != nullptr) {
+    DCHECK(!is_main_thread());
+    inspector_path = parent_handle->url();
+    inspector_agent_->SetParentHandle(
+        std::unique_ptr<inspector::ParentInspectorHandle>(parent_handle));
+  } else {
+    inspector_path = argv_.size() > 1 ? argv_[1].c_str() : "";
+  }
+  CHECK(!inspector_agent_->IsListening());
+  // 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.
+  inspector_agent_->Start(inspector_path,
+                          options_->debug_options(),
+                          inspector_host_port(),
+                          is_main_thread());
+  if (options_->debug_options().inspector_enabled &&
+      !inspector_agent_->IsListening()) {
+    return 12;  // Signal internal error
+  }
+
+  profiler::StartProfilers(this);
+
+  if (options_->debug_options().break_node_first_line) {
+    inspector_agent_->PauseOnNextJavascriptStatement("Break at bootstrap");
+  }
+
+  return 0;
+}
+#endif  // HAVE_INSPECTOR && NODE_USE_V8_PLATFORM
+
+void Environment::InitializeDiagnostics() {
+  isolate_->GetHeapProfiler()->AddBuildEmbedderGraphCallback(
+      Environment::BuildEmbedderGraph, this);
+
+#if defined HAVE_DTRACE || defined HAVE_ETW
+  InitDTrace(this);
+#endif
+}
+
 MaybeLocal<Value> RunBootstrapping(Environment* env) {
   CHECK(!env->has_run_bootstrapping_code());
 
   EscapableHandleScope scope(env->isolate());
   Isolate* isolate = env->isolate();
   Local<Context> context = env->context();
 
-#if HAVE_INSPECTOR
-  profiler::StartProfilers(env);
-#endif  // HAVE_INSPECTOR
 
   // Add a reference to the global object
   Local<Object> global = context->Global();
-
-#if defined HAVE_DTRACE || defined HAVE_ETW
-  InitDTrace(env);
-#endif
-
   Local<Object> process = env->process_object();
 
   // Setting global properties for the bootstrappers to use:
@@ -261,12 +301,6 @@ MaybeLocal<Value> RunBootstrapping(Environment* env) {
   global->Set(context, FIXED_ONE_BYTE_STRING(env->isolate(), "global"), global)
       .Check();
 
-#if HAVE_INSPECTOR
-  if (env->options()->debug_options().break_node_first_line) {
-    env->inspector_agent()->PauseOnNextJavascriptStatement(
-        "Break at bootstrap");
-  }
-#endif  // HAVE_INSPECTOR
 
   // Create binding loaders
   std::vector<Local<String>> loaders_params = {

diff --git a/src/node_main_instance.cc b/src/node_main_instance.cc
@@ -195,27 +195,16 @@ std::unique_ptr<Environment> NodeMainInstance::CreateMainEnvironment(
                                       Environment::kOwnsInspector));
   env->InitializeLibuv(per_process::v8_is_profiling);
 
+  // TODO(joyeecheung): when we snapshot the bootstrapped context,
+  // the inspector and diagnostics setup should after after deserialization.
 #if HAVE_INSPECTOR && NODE_USE_V8_PLATFORM
-  CHECK(!env->inspector_agent()->IsListening());
-  // 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.
-  env->inspector_agent()->Start(args_.size() > 1 ? args_[1].c_str() : "",
-                                env->options()->debug_options(),
-                                env->inspector_host_port(),
-                                true);
-  if (env->options()->debug_options().inspector_enabled &&
-      !env->inspector_agent()->IsListening()) {
-    *exit_code = 12;  // Signal internal error.
+  *exit_code = env->InitializeInspector(nullptr);
+#endif
+  if (*exit_code != 0) {
     return env;
   }
-#else
-  // inspector_enabled can't be true if !HAVE_INSPECTOR or
-  // !NODE_USE_V8_PLATFORM
-  // - the option parser should not allow that.
-  CHECK(!env->options()->debug_options().inspector_enabled);
-#endif  // HAVE_INSPECTOR && NODE_USE_V8_PLATFORM
 
+  env->InitializeDiagnostics();
   if (RunBootstrapping(env.get()).IsEmpty()) {
     *exit_code = 1;
   }

diff --git a/src/node_worker.cc b/src/node_worker.cc
@@ -41,17 +41,6 @@ namespace worker {
 namespace {
 
 #if NODE_USE_V8_PLATFORM && HAVE_INSPECTOR
-void StartWorkerInspector(
-    Environment* child,
-    std::unique_ptr<inspector::ParentInspectorHandle> parent_handle,
-    const std::string& url) {
-  child->inspector_agent()->SetParentHandle(std::move(parent_handle));
-  child->inspector_agent()->Start(url,
-                                  child->options()->debug_options(),
-                                  child->inspector_host_port(),
-                                  false);
-}
-
 void WaitForWorkerInspectorToStop(Environment* child) {
   child->inspector_agent()->WaitForDisconnect();
   child->inspector_agent()->Stop();
@@ -66,11 +55,11 @@ Worker::Worker(Environment* env,
                std::shared_ptr<PerIsolateOptions> per_isolate_opts,
                std::vector<std::string>&& exec_argv)
     : AsyncWrap(env, wrap, AsyncWrap::PROVIDER_WORKER),
-      url_(url),
       per_isolate_opts_(per_isolate_opts),
       exec_argv_(exec_argv),
       platform_(env->isolate_data()->platform()),
-      profiler_idle_notifier_started_(env->profiler_idle_notifier_started()),
+      // XXX(joyeecheung): should this be per_process::v8_is_profiling instead?
+      start_profiler_idle_notifier_(env->profiler_idle_notifier_started()),
       thread_id_(Environment::AllocateThreadId()),
       env_vars_(env->env_vars()) {
   Debug(this, "Creating new worker instance with thread id %llu", thread_id_);
@@ -264,7 +253,7 @@ void Worker::Run() {
         env_->set_abort_on_uncaught_exception(false);
         env_->set_worker_context(this);
 
-        env_->InitializeLibuv(profiler_idle_notifier_started_);
+        env_->InitializeLibuv(start_profiler_idle_notifier_);
       }
       {
         Mutex::ScopedLock lock(mutex_);
@@ -275,12 +264,10 @@ void Worker::Run() {
       if (is_stopped()) return;
       {
 #if NODE_USE_V8_PLATFORM && HAVE_INSPECTOR
-        StartWorkerInspector(env_.get(),
-                             std::move(inspector_parent_handle_),
-                             url_);
-#endif
+        env_->InitializeInspector(inspector_parent_handle_.release());
         inspector_started = true;
-
+#endif
+        env_->InitializeDiagnostics();
         HandleScope handle_scope(isolate_);
         AsyncCallbackScope callback_scope(env_.get());
         env_->async_hooks()->push_async_ids(1, 0);

diff --git a/src/node_worker.h b/src/node_worker.h
@@ -54,15 +54,14 @@ class Worker : public AsyncWrap {
  private:
   void OnThreadStopped();
   void CreateEnvMessagePort(Environment* env);
-  const std::string url_;
 
   std::shared_ptr<PerIsolateOptions> per_isolate_opts_;
   std::vector<std::string> exec_argv_;
   std::vector<std::string> argv_;
 
   MultiIsolatePlatform* platform_;
   v8::Isolate* isolate_ = nullptr;
-  bool profiler_idle_notifier_started_;
+  bool start_profiler_idle_notifier_;
   uv_thread_t tid_;
 
 #if NODE_USE_V8_PLATFORM && HAVE_INSPECTOR