From 193d09b818ff2edf4be788ae3be05b1c6f9068e1 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 2 Nov 2019 18:58:24 +0100 Subject: [PATCH 1/6] src: track no of active JS signal handlers This makes it possible to tell whether a signal is being tracked in JS. --- src/node_internals.h | 2 ++ src/signal_wrap.cc | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+) diff --git a/src/node_internals.h b/src/node_internals.h index 2ec230d8b54bfd..fd05e8d8fb0791 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -311,6 +311,8 @@ void EndStartedProfilers(Environment* env); } #endif // HAVE_INSPECTOR +bool HasSignalJSHandler(int signum); + #ifdef _WIN32 typedef SYSTEMTIME TIME_TYPE; #else // UNIX, OSX diff --git a/src/signal_wrap.cc b/src/signal_wrap.cc index 90b91f35a86c8b..7d9ea842213de5 100644 --- a/src/signal_wrap.cc +++ b/src/signal_wrap.cc @@ -38,8 +38,13 @@ using v8::Object; using v8::String; using v8::Value; +void DecreaseSignalHandlerCount(int signum); + namespace { +static Mutex handled_signals_mutex; +static std::map handled_signals; // Signal -> number of handlers + class SignalWrap : public HandleWrap { public: static void Initialize(Local target, @@ -85,6 +90,11 @@ class SignalWrap : public HandleWrap { CHECK_EQ(r, 0); } + void Close(v8::Local close_callback) override { + if (active_) DecreaseSignalHandlerCount(handle_.signum); + HandleWrap::Close(close_callback); + } + static void Start(const FunctionCallbackInfo& args) { SignalWrap* wrap; ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); @@ -112,21 +122,50 @@ class SignalWrap : public HandleWrap { wrap->MakeCallback(env->onsignal_string(), 1, &arg); }, signum); + + if (err == 0) { + CHECK(!wrap->active_); + wrap->active_ = true; + Mutex::ScopedLock lock(handled_signals_mutex); + handled_signals[signum]++; + } + args.GetReturnValue().Set(err); } static void Stop(const FunctionCallbackInfo& args) { SignalWrap* wrap; ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); + + if (wrap->active_) { + wrap->active_ = false; + fprintf(stderr, "Stop %d\n", wrap->handle_.signum); + DecreaseSignalHandlerCount(wrap->handle_.signum); + } + int err = uv_signal_stop(&wrap->handle_); args.GetReturnValue().Set(err); } uv_signal_t handle_; + bool active_ = false; }; } // anonymous namespace + +void DecreaseSignalHandlerCount(int signum) { + Mutex::ScopedLock lock(handled_signals_mutex); + int new_handler_count = --handled_signals[signum]; + CHECK_GE(new_handler_count, 0); + if (new_handler_count == 0) + handled_signals.erase(signum); +} + +bool HasSignalJSHandler(int signum) { + Mutex::ScopedLock lock(handled_signals_mutex); + return handled_signals.find(signum) != handled_signals.end(); +} } // namespace node From 49e17211043a14b08aaf158244a0079a8303332f Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 2 Nov 2019 19:06:47 +0100 Subject: [PATCH 2/6] src: make EndStartedProfilers an exit hook Run `EndStartedProfilers` 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. --- src/inspector_profiler.cc | 27 +++++++++++++++++++++------ src/node.cc | 1 - src/node_errors.cc | 4 +--- src/node_internals.h | 1 - src/node_process_methods.cc | 13 +++++++++---- src/node_worker.cc | 3 --- 6 files changed, 31 insertions(+), 18 deletions(-) diff --git a/src/inspector_profiler.cc b/src/inspector_profiler.cc index b5f63b2b41389d..e0d02d6952a3f9 100644 --- a/src/inspector_profiler.cc +++ b/src/inspector_profiler.cc @@ -4,6 +4,7 @@ #include "diagnosticfilename-inl.h" #include "memory_tracker-inl.h" #include "node_file.h" +#include "node_errors.h" #include "node_internals.h" #include "util-inl.h" #include "v8-inspector.h" @@ -13,6 +14,7 @@ namespace node { namespace profiler { +using errors::TryCatchScope; using v8::Context; using v8::Function; using v8::FunctionCallbackInfo; @@ -219,12 +221,21 @@ void V8CoverageConnection::WriteProfile(Local message) { } // append source-map cache information to coverage object: - Local source_map_cache_getter = env_->source_map_cache_getter(); Local source_map_cache_v; - if (!source_map_cache_getter->Call(env()->context(), - Undefined(isolate), 0, nullptr) - .ToLocal(&source_map_cache_v)) { - return; + { + TryCatchScope try_catch(env()); + { + Isolate::AllowJavascriptExecutionScope allow_js_here(isolate); + Local source_map_cache_getter = env_->source_map_cache_getter(); + if (!source_map_cache_getter->Call( + context, Undefined(isolate), 0, nullptr) + .ToLocal(&source_map_cache_v)) { + return; + } + } + if (try_catch.HasCaught() && !try_catch.HasTerminated()) { + PrintCaughtException(isolate, context, try_catch); + } } // Avoid writing to disk if no source-map data: if (!source_map_cache_v->IsUndefined()) { @@ -351,7 +362,7 @@ void V8HeapProfilerConnection::End() { // For now, we only support coverage profiling, but we may add more // in the future. -void EndStartedProfilers(Environment* env) { +static void EndStartedProfilers(Environment* env) { Debug(env, DebugCategory::INSPECTOR_PROFILER, "EndStartedProfilers\n"); V8ProfilerConnection* connection = env->cpu_profiler_connection(); if (connection != nullptr && !connection->ending()) { @@ -390,6 +401,10 @@ std::string GetCwd(Environment* env) { } void StartProfilers(Environment* env) { + AtExit(env, [](void* env) { + EndStartedProfilers(static_cast(env)); + }, env); + Isolate* isolate = env->isolate(); Local coverage_str = env->env_vars()->Get( isolate, FIXED_ONE_BYTE_STRING(isolate, "NODE_V8_COVERAGE")) diff --git a/src/node.cc b/src/node.cc index f437ea4be96450..95563bc91eab89 100644 --- a/src/node.cc +++ b/src/node.cc @@ -168,7 +168,6 @@ static const unsigned kMaxSignal = 32; void WaitForInspectorDisconnect(Environment* env) { #if HAVE_INSPECTOR - profiler::EndStartedProfilers(env); if (env->inspector_agent()->IsActive()) { // Restore signal dispositions, the app is done and is no longer diff --git a/src/node_errors.cc b/src/node_errors.cc index ec14746514e420..e094fe4681fc56 100644 --- a/src/node_errors.cc +++ b/src/node_errors.cc @@ -993,9 +993,7 @@ void TriggerUncaughtException(Isolate* isolate, // Now we are certain that the exception is fatal. ReportFatalException(env, error, message, EnhanceFatalException::kEnhance); -#if HAVE_INSPECTOR - profiler::EndStartedProfilers(env); -#endif + RunAtExit(env); // If the global uncaught exception handler sets process.exitCode, // exit with that code. Otherwise, exit with 1. diff --git a/src/node_internals.h b/src/node_internals.h index fd05e8d8fb0791..ee9d6b3e473a89 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -307,7 +307,6 @@ void SetIsolateCreateParamsForNode(v8::Isolate::CreateParams* params); #if HAVE_INSPECTOR namespace profiler { void StartProfilers(Environment* env); -void EndStartedProfilers(Environment* env); } #endif // HAVE_INSPECTOR diff --git a/src/node_process_methods.cc b/src/node_process_methods.cc index 3a2c1efd812fca..3f0590aef7fb8c 100644 --- a/src/node_process_methods.cc +++ b/src/node_process_methods.cc @@ -172,11 +172,15 @@ static void Kill(const FunctionCallbackInfo& args) { if (!args[0]->Int32Value(context).To(&pid)) return; int sig; if (!args[1]->Int32Value(context).To(&sig)) return; - // TODO(joyeecheung): white list the signals? -#if HAVE_INSPECTOR - profiler::EndStartedProfilers(env); -#endif + uv_pid_t own_pid = uv_os_getpid(); + if (sig > 0 && + (pid == 0 || pid == -1 || pid == own_pid || pid == -own_pid) && + !HasSignalJSHandler(sig)) { + // This is most likely going to terminate this process. + // It's not an exact method but it might be close enough. + RunAtExit(env); + } int err = uv_kill(pid, sig); args.GetReturnValue().Set(err); @@ -428,6 +432,7 @@ 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 c79968bad90c68..17d7d2dc445635 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -397,9 +397,6 @@ void Worker::Run() { if (exit_code_ == 0 && !stopped) exit_code_ = exit_code; -#if HAVE_INSPECTOR - profiler::EndStartedProfilers(env_.get()); -#endif Debug(this, "Exiting thread for worker %llu with exit code %d", thread_id_, exit_code_); } From e4e4e76cc7689772d6dc60eb71611adc3af9f3c7 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 2 Nov 2019 19:06:59 +0100 Subject: [PATCH 3/6] 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. --- 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 469e0b4f8f335a..f13e68c067529e 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 95563bc91eab89..92e1469c1b6a3a 100644 --- 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 index ee9d6b3e473a89..9a80d998d5d24e 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. #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 index 1c1a09280b70d8..e25e52e1e94e89 100644 --- 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 index 3f0590aef7fb8c..f2fb5cf38cf211 100644 --- a/src/node_process_methods.cc +++ b/src/node_process_methods.cc @@ -433,7 +433,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 17d7d2dc445635..a08f5e7d5eefd5 100644 --- 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 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 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()); From ded953f80fcb7dd0308a5e77ceb10413c7464c0e Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 2 Nov 2019 21:04:57 +0100 Subject: [PATCH 4/6] src: use unique_ptr for InitializeInspector() This makes more sense than releasing and re-wrapping the raw pointer. --- src/env.h | 3 ++- src/node.cc | 7 +++---- src/node_main_instance.cc | 6 +++++- src/node_worker.cc | 2 +- 4 files changed, 11 insertions(+), 7 deletions(-) diff --git a/src/env.h b/src/env.h index a649675c7cc902..15832edcda1df5 100644 --- a/src/env.h +++ b/src/env.h @@ -874,7 +874,8 @@ class Environment : public MemoryRetainer { #if HAVE_INSPECTOR // 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); + int InitializeInspector( + std::unique_ptr parent_handle); #endif v8::MaybeLocal BootstrapInternalLoaders(); diff --git a/src/node.cc b/src/node.cc index 92e1469c1b6a3a..1246f20026ff3a 100644 --- a/src/node.cc +++ b/src/node.cc @@ -202,13 +202,12 @@ MaybeLocal ExecuteBootstrapper(Environment* env, #if HAVE_INSPECTOR int Environment::InitializeInspector( - inspector::ParentInspectorHandle* parent_handle) { + std::unique_ptr parent_handle) { std::string inspector_path; - if (parent_handle != nullptr) { + if (parent_handle) { DCHECK(!is_main_thread()); inspector_path = parent_handle->url(); - inspector_agent_->SetParentHandle( - std::unique_ptr(parent_handle)); + inspector_agent_->SetParentHandle(std::move(parent_handle)); } else { inspector_path = argv_.size() > 1 ? argv_[1].c_str() : ""; } diff --git a/src/node_main_instance.cc b/src/node_main_instance.cc index e25e52e1e94e89..91fb73495c4367 100644 --- a/src/node_main_instance.cc +++ b/src/node_main_instance.cc @@ -7,6 +7,10 @@ #include #endif +#if HAVE_INSPECTOR +#include "inspector/worker_inspector.h" // ParentInspectorHandle +#endif + namespace node { using v8::Context; @@ -221,7 +225,7 @@ std::unique_ptr NodeMainInstance::CreateMainEnvironment( // TODO(joyeecheung): when we snapshot the bootstrapped context, // the inspector and diagnostics setup should after after deserialization. #if HAVE_INSPECTOR - *exit_code = env->InitializeInspector(nullptr); + *exit_code = env->InitializeInspector({}); #endif if (*exit_code != 0) { return env; diff --git a/src/node_worker.cc b/src/node_worker.cc index a08f5e7d5eefd5..cc6c2813a07881 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -325,7 +325,7 @@ void Worker::Run() { { env_->InitializeDiagnostics(); #if HAVE_INSPECTOR - env_->InitializeInspector(inspector_parent_handle_.release()); + env_->InitializeInspector(std::move(inspector_parent_handle_)); #endif HandleScope handle_scope(isolate_); AsyncCallbackScope callback_scope(env_.get()); From 36787d7b5c28177dd902412a8795ab299fa33312 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 2 Nov 2019 21:11:40 +0100 Subject: [PATCH 5/6] src: run RunBeforeExitCallbacks as part of EmitBeforeExit 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. This also aligns the worker_threads code with the main thread code. --- src/api/hooks.cc | 2 ++ src/node_main_instance.cc | 2 -- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/api/hooks.cc b/src/api/hooks.cc index cec58cee00847c..2dd0cc994ea7f2 100644 --- a/src/api/hooks.cc +++ b/src/api/hooks.cc @@ -30,6 +30,8 @@ void AtExit(Environment* env, void (*cb)(void* arg), void* arg) { } void EmitBeforeExit(Environment* env) { + env->RunBeforeExitCallbacks(); + HandleScope handle_scope(env->isolate()); Context::Scope context_scope(env->context()); Local exit_code = env->process_object() diff --git a/src/node_main_instance.cc b/src/node_main_instance.cc index 91fb73495c4367..b7ca367ad89a55 100644 --- a/src/node_main_instance.cc +++ b/src/node_main_instance.cc @@ -136,8 +136,6 @@ int NodeMainInstance::Run() { more = uv_loop_alive(env->event_loop()); if (more && !env->is_stopping()) continue; - env->RunBeforeExitCallbacks(); - if (!uv_loop_alive(env->event_loop())) { EmitBeforeExit(env.get()); } From e18e4f062c1d722aceb5148719752e3123a6115a Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 6 Nov 2019 14:51:51 +0100 Subject: [PATCH 6/6] fixup! src: track no of active JS signal handlers --- src/signal_wrap.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/src/signal_wrap.cc b/src/signal_wrap.cc index 7d9ea842213de5..cf67dc590f6d51 100644 --- a/src/signal_wrap.cc +++ b/src/signal_wrap.cc @@ -139,7 +139,6 @@ class SignalWrap : public HandleWrap { if (wrap->active_) { wrap->active_ = false; - fprintf(stderr, "Stop %d\n", wrap->handle_.signum); DecreaseSignalHandlerCount(wrap->handle_.signum); }