From 79713ed8e5f36d19b4cda89281f47c7c4b0936b3 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 2 Nov 2019 19:06:59 +0100 Subject: [PATCH] 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. PR-URL: https://github.com/nodejs/node/pull/30229 Reviewed-By: Colin Ihrig Reviewed-By: Gireesh Punathil Reviewed-By: Joyee Cheung Reviewed-By: Shelley Vohr --- src/inspector_agent.cc | 7 +++++++ src/node.cc | 26 +------------------------- src/node_internals.h | 5 ++++- src/node_main_instance.cc | 15 ++++++++++++++- src/node_process_methods.cc | 1 - src/node_worker.cc | 19 ------------------- 6 files changed, 26 insertions(+), 47 deletions(-) diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index fb3415c275e6db..94d2704a05d520 100644 --- 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(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 index 51df2ee58bf058..81b29dbf337bf9 100644 --- a/src/node.cc +++ b/src/node.cc @@ -151,31 +151,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 -} - void SignalExit(int signo) { ResetStdio(); #ifdef __FreeBSD__ @@ -522,6 +497,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 index 3c1a004942aecc..0fff2e6111e53f 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -90,7 +90,6 @@ void PrintCaughtException(v8::Isolate* isolate, v8::Local context, const v8::TryCatch& try_catch); -void WaitForInspectorDisconnect(Environment* env); void ResetStdio(); // Safe to call more than once and from signal handlers. void SignalExit(int signo); #ifdef __POSIX__ @@ -313,6 +312,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 index 0744063e0dc3a9..25bede66edbce8 100644 --- a/src/node_main_instance.cc +++ b/src/node_main_instance.cc @@ -145,13 +145,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 index 0d44ecc5ec6365..5aa2068817045c 100644 --- a/src/node_process_methods.cc +++ b/src/node_process_methods.cc @@ -426,7 +426,6 @@ static void DebugEnd(const FunctionCallbackInfo& args) { static void ReallyExit(const FunctionCallbackInfo& 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 index d9e5891fddff20..311aaa8e038c1c 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -37,17 +37,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 wrap, const std::string& url, @@ -191,9 +180,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 env_; OnScopeLeave cleanup_env([&]() { @@ -223,10 +209,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 @@ -270,7 +252,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());