src: make WaitForInspectorDisconnect an exit hook

Run inspector cleanup code on Environment teardown. This is part of a series of changes to make embedding easier, by requiring fewer internal methods to build a fully functioning Node.js instance.
nodejs · addaleax · Nov 2, 2019 · Nov 2, 2019 · Nov 2, 2019 · Nov 2, 2019
commit e4e4e76cc7689772d6dc60eb71611adc3af9f3c7
diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc
@@ -778,6 +778,13 @@ bool Agent::Start(const std::string& path,
     StartDebugSignalHandler();
   }
 
+  AtExit(parent_env_, [](void* env) {
+    Agent* agent = static_cast<Environment*>(env)->inspector_agent();
+    if (agent->IsActive()) {
+      agent->WaitForDisconnect();
+    }
+  }, parent_env_);
+
   bool wait_for_connect = options.wait_for_connect();
   if (parent_handle_) {
     wait_for_connect = parent_handle_->WaitForConnect();

diff --git a/src/node.cc b/src/node.cc
@@ -162,31 +162,6 @@ bool v8_is_profiling = false;
 struct V8Platform v8_platform;
 }  // namespace per_process
 
-#ifdef __POSIX__
-static const unsigned kMaxSignal = 32;
-#endif
-
-void WaitForInspectorDisconnect(Environment* env) {
-#if HAVE_INSPECTOR
-
-  if (env->inspector_agent()->IsActive()) {
-    // Restore signal dispositions, the app is done and is no longer
-    // capable of handling signals.
-#if defined(__POSIX__) && !defined(NODE_SHARED_MODE)
-    struct sigaction act;
-    memset(&act, 0, sizeof(act));
-    for (unsigned nr = 1; nr < kMaxSignal; nr += 1) {
-      if (nr == SIGKILL || nr == SIGSTOP || nr == SIGPROF)
-        continue;
-      act.sa_handler = (nr == SIGPIPE) ? SIG_IGN : SIG_DFL;
-      CHECK_EQ(0, sigaction(nr, &act, nullptr));
-    }
-#endif
-    env->inspector_agent()->WaitForDisconnect();
-  }
-#endif
-}
-
 #ifdef __POSIX__
 void SignalExit(int signo, siginfo_t* info, void* ucontext) {
   ResetStdio();
@@ -558,6 +533,7 @@ inline void PlatformInit() {
   CHECK_EQ(err, 0);
 #endif  // HAVE_INSPECTOR
 
+  // TODO(addaleax): NODE_SHARED_MODE does not really make sense here.
 #ifndef NODE_SHARED_MODE
   // Restore signal dispositions, the parent process may have changed them.
   struct sigaction act;

diff --git a/src/node_internals.h b/src/node_internals.h
@@ -90,7 +90,6 @@ void PrintCaughtException(v8::Isolate* isolate,
                           v8::Local<v8::Context> context,
                           const v8::TryCatch& try_catch);
 
-void WaitForInspectorDisconnect(Environment* env);
 void ResetStdio();  // Safe to call more than once and from signal handlers.
 #ifdef __POSIX__
 void SignalExit(int signal, siginfo_t* info, void* ucontext);
@@ -310,6 +309,10 @@ void StartProfilers(Environment* env);
 }
 #endif  // HAVE_INSPECTOR
 
+#ifdef __POSIX__
+static constexpr unsigned kMaxSignal = 32;
+#endif
+
 bool HasSignalJSHandler(int signum);
 
 #ifdef _WIN32

diff --git a/src/node_main_instance.cc b/src/node_main_instance.cc
@@ -148,13 +148,26 @@ int NodeMainInstance::Run() {
 
     env->set_trace_sync_io(false);
     exit_code = EmitExit(env.get());
-    WaitForInspectorDisconnect(env.get());
   }
 
   env->set_can_call_into_js(false);
   env->stop_sub_worker_contexts();
   ResetStdio();
   env->RunCleanup();
+
+  // TODO(addaleax): Neither NODE_SHARED_MODE nor HAVE_INSPECTOR really
+  // make sense here.
+#if HAVE_INSPECTOR && defined(__POSIX__) && !defined(NODE_SHARED_MODE)
+  struct sigaction act;
+  memset(&act, 0, sizeof(act));
+  for (unsigned nr = 1; nr < kMaxSignal; nr += 1) {
+    if (nr == SIGKILL || nr == SIGSTOP || nr == SIGPROF)
+      continue;
+    act.sa_handler = (nr == SIGPIPE) ? SIG_IGN : SIG_DFL;
+    CHECK_EQ(0, sigaction(nr, &act, nullptr));
+  }
+#endif
+
   RunAtExit(env.get());
 
   per_process::v8_platform.DrainVMTasks(isolate_);

diff --git a/src/node_process_methods.cc b/src/node_process_methods.cc
@@ -433,7 +433,6 @@ static void DebugEnd(const FunctionCallbackInfo<Value>& args) {
 static void ReallyExit(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
   RunAtExit(env);
-  WaitForInspectorDisconnect(env);
   int code = args[0]->Int32Value(env->context()).FromMaybe(0);
   env->Exit(code);
 }

diff --git a/src/node_worker.cc b/src/node_worker.cc
@@ -42,17 +42,6 @@ using v8::Value;
 namespace node {
 namespace worker {
 
-namespace {
-
-#if HAVE_INSPECTOR
-void WaitForWorkerInspectorToStop(Environment* child) {
-  child->inspector_agent()->WaitForDisconnect();
-  child->inspector_agent()->Stop();
-}
-#endif
-
-}  // anonymous namespace
-
 Worker::Worker(Environment* env,
                Local<Object> wrap,
                const std::string& url,
@@ -251,9 +240,6 @@ void Worker::Run() {
     Locker locker(isolate_);
     Isolate::Scope isolate_scope(isolate_);
     SealHandleScope outer_seal(isolate_);
-#if HAVE_INSPECTOR
-    bool inspector_started = false;
-#endif
 
     DeleteFnPtr<Environment, FreeEnvironment> env_;
     OnScopeLeave cleanup_env([&]() {
@@ -283,10 +269,6 @@ void Worker::Run() {
         env_->stop_sub_worker_contexts();
         env_->RunCleanup();
         RunAtExit(env_.get());
-#if HAVE_INSPECTOR
-        if (inspector_started)
-          WaitForWorkerInspectorToStop(env_.get());
-#endif
 
         // This call needs to be made while the `Environment` is still alive
         // because we assume that it is available for async tracking in the
@@ -344,7 +326,6 @@ void Worker::Run() {
         env_->InitializeDiagnostics();
 #if HAVE_INSPECTOR
         env_->InitializeInspector(inspector_parent_handle_.release());
-        inspector_started = true;
 #endif
         HandleScope handle_scope(isolate_);
         AsyncCallbackScope callback_scope(env_.get());