diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index fbafc55f1b74..7dcb4070d9b4 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -72,12 +72,12 @@ using v8_inspector::V8InspectorClient; #ifdef __POSIX__ static uv_sem_t start_io_thread_semaphore; #endif // __POSIX__ -static uv_async_t start_io_thread_async; -// This is just an additional check to make sure start_io_thread_async -// is not accidentally re-used or used when uninitialized. -static std::atomic_bool start_io_thread_async_initialized { false }; -// Protects the Agent* stored in start_io_thread_async.data. -static Mutex start_io_thread_async_mutex; +// Agents that asked for the debug signal handler; SIGUSR1 (or the Windows +// remote thread) starts the io thread of each. The mutex also guards the +// once-per-process watchdog setup. +static Mutex start_io_thread_agents_mutex; +static std::vector start_io_thread_agents; +static bool debug_signal_handler_started = false; template void SyncJavaScriptHookState(Environment* env, @@ -125,12 +125,11 @@ void SyncJavaScriptHookState(Environment* env, } } -// Called on the main thread. -void StartIoThreadAsyncCallback(uv_async_t* handle) { - static_cast(handle->data)->StartIoThread(); +static void RequestIoThreadStartOnAgents() { + Mutex::ScopedLock lock(start_io_thread_agents_mutex); + for (Agent* agent : start_io_thread_agents) agent->RequestIoThreadStart(); } - #ifdef __POSIX__ static void StartIoThreadWakeup(int signo, siginfo_t* info, void* ucontext) { uv_sem_post(&start_io_thread_semaphore); @@ -140,16 +139,11 @@ inline void* StartIoThreadMain(void* unused) { uv_thread_setname("SignalInspector"); for (;;) { uv_sem_wait(&start_io_thread_semaphore); - Mutex::ScopedLock lock(start_io_thread_async_mutex); - - CHECK(start_io_thread_async_initialized); - Agent* agent = static_cast(start_io_thread_async.data); - if (agent != nullptr) - agent->RequestIoThreadStart(); + RequestIoThreadStartOnAgents(); } } -static int StartDebugSignalHandler() { +static int StartWatchdogThread() { // Start a watchdog thread for calling v8::Debug::DebugBreak() because // it's not safe to call directly from the signal handler, it can // deadlock with the thread it interrupts. @@ -184,14 +178,28 @@ static int StartDebugSignalHandler() { fprintf(stderr, "node[%u]: pthread_create: %s\n", uv_os_getpid(), strerror(err)); fflush(stderr); - // Leave SIGUSR1 blocked. We don't install a signal handler, - // receiving the signal would terminate the process. + uv_sem_destroy(&start_io_thread_semaphore); return -err; } RegisterSignalHandler(SIGUSR1, StartIoThreadWakeup); // Restore original mask CHECK_EQ(0, pthread_sigmask(SIG_SETMASK, &sigmask, nullptr)); - // Unblock SIGUSR1. A pending SIGUSR1 signal will now be delivered. + return 0; +} + +static int StartDebugSignalHandler() { + { + Mutex::ScopedLock lock(start_io_thread_agents_mutex); + if (!debug_signal_handler_started) { + // Leave SIGUSR1 blocked on failure. We don't install a signal handler, + // receiving the signal would terminate the process. + if (int err = StartWatchdogThread()) return err; + debug_signal_handler_started = true; + } + } + // Unblock SIGUSR1 on this thread; PlatformInit() left it blocked. A pending + // SIGUSR1 signal will now be delivered. + sigset_t sigmask; sigemptyset(&sigmask); sigaddset(&sigmask, SIGUSR1); CHECK_EQ(0, pthread_sigmask(SIG_UNBLOCK, &sigmask, nullptr)); @@ -202,11 +210,7 @@ static int StartDebugSignalHandler() { #ifdef _WIN32 DWORD WINAPI StartIoThreadProc(void* arg) { - Mutex::ScopedLock lock(start_io_thread_async_mutex); - CHECK(start_io_thread_async_initialized); - Agent* agent = static_cast(start_io_thread_async.data); - if (agent != nullptr) - agent->RequestIoThreadStart(); + RequestIoThreadStartOnAgents(); return 0; } @@ -216,6 +220,9 @@ static int GetDebugSignalHandlerMappingName(DWORD pid, wchar_t* buf, } static int StartDebugSignalHandler() { + Mutex::ScopedLock lock(start_io_thread_agents_mutex); + if (debug_signal_handler_started) return 0; + debug_signal_handler_started = true; wchar_t mapping_name[32]; HANDLE mapping_handle; DWORD pid; @@ -899,7 +906,21 @@ Agent::Agent(Environment* env) debug_options_(env->options()->debug_options()), host_port_(env->inspector_host_port()) {} -Agent::~Agent() = default; +Agent::~Agent() { + StopAcceptingIoThreadStarts(); +} + +void Agent::StopAcceptingIoThreadStarts() { + if (start_io_thread_async_ == nullptr) return; + { + Mutex::ScopedLock lock(start_io_thread_agents_mutex); + std::erase(start_io_thread_agents, this); + } + parent_env_->RemoveCleanupHook(StopAcceptingIoThreadStartsHook, this); + parent_env_->CloseHandle(start_io_thread_async_, + [](uv_async_t* handle) { delete handle; }); + start_io_thread_async_ = nullptr; +} bool Agent::Start(const std::string& path, const DebugOptions& options, @@ -911,33 +932,25 @@ bool Agent::Start(const std::string& path, host_port_ = host_port; client_ = std::make_shared(parent_env_, is_main); - if (parent_env_->owns_inspector()) { - Mutex::ScopedLock lock(start_io_thread_async_mutex); - CHECK_EQ(start_io_thread_async_initialized.exchange(true), false); - CHECK_EQ(0, uv_async_init(parent_env_->event_loop(), - &start_io_thread_async, - StartIoThreadAsyncCallback)); - uv_unref(reinterpret_cast(&start_io_thread_async)); - start_io_thread_async.data = this; - if (parent_env_->should_start_debug_signal_handler()) { - // Ignore failure, SIGUSR1 won't work, but that should not block node - // start. - StartDebugSignalHandler(); + if (parent_env_->owns_inspector() && + parent_env_->should_start_debug_signal_handler()) { + start_io_thread_async_ = new uv_async_t; + start_io_thread_async_->data = this; + CHECK_EQ(0, + uv_async_init(parent_env_->event_loop(), + start_io_thread_async_, + [](uv_async_t* handle) { + static_cast(handle->data)->StartIoThread(); + })); + uv_unref(reinterpret_cast(start_io_thread_async_)); + { + Mutex::ScopedLock lock(start_io_thread_agents_mutex); + start_io_thread_agents.push_back(this); } - - parent_env_->AddCleanupHook([](void* data) { - Environment* env = static_cast(data); - - { - Mutex::ScopedLock lock(start_io_thread_async_mutex); - start_io_thread_async.data = nullptr; - } - - // This is global, will never get freed - env->CloseHandle(&start_io_thread_async, [](uv_async_t*) { - CHECK(start_io_thread_async_initialized.exchange(false)); - }); - }, parent_env_); + parent_env_->AddCleanupHook(StopAcceptingIoThreadStartsHook, this); + // Ignore failure, SIGUSR1 won't work, but that should not block node + // start. + StartDebugSignalHandler(); } AtExit(parent_env_, [](void* env) { @@ -1175,6 +1188,10 @@ void Agent::AllAsyncTasksCanceled() { client_->AllAsyncTasksCanceled(); } +void Agent::StopAcceptingIoThreadStartsHook(void* agent) { + static_cast(agent)->StopAcceptingIoThreadStarts(); +} + void Agent::RequestIoThreadStart() { // We need to attempt to interrupt V8 flow (in case Node is running // continuous JS code) and to wake up libuv thread (in case Node is waiting @@ -1182,14 +1199,10 @@ void Agent::RequestIoThreadStart() { if (!options().allow_attaching_debugger) { return; } - CHECK(start_io_thread_async_initialized); - uv_async_send(&start_io_thread_async); parent_env_->RequestInterrupt([this](Environment*) { StartIoThread(); }); - - CHECK(start_io_thread_async_initialized); - uv_async_send(&start_io_thread_async); + uv_async_send(start_io_thread_async_); } void Agent::ContextCreated(Local context, const ContextInfo& info) { diff --git a/src/inspector_agent.h b/src/inspector_agent.h index 73835d2a4d1d..6b92ef2a08fe 100644 --- a/src/inspector_agent.h +++ b/src/inspector_agent.h @@ -8,6 +8,7 @@ #endif #include "node_options.h" +#include "uv.h" #include "v8.h" #include @@ -123,7 +124,8 @@ class Agent { // Can only be called from the main thread. bool StartIoThread(); - // Calls StartIoThread() from off the main thread. + // Calls StartIoThread() from off the main thread. Only valid while the + // Environment owns the inspector and has not started cleanup. void RequestIoThreadStart(); const DebugOptions& options() { return debug_options_; } @@ -160,6 +162,12 @@ class Agent { // reconciles the two when it is possible and safe to call into JS. JavaScriptHookState async_hook_state_; + // Woken by the SIGUSR1 watchdog; closed by the cleanup hook or ~Agent(), + // whichever runs first, and freed by its close callback. + uv_async_t* start_io_thread_async_ = nullptr; + void StopAcceptingIoThreadStarts(); + static void StopAcceptingIoThreadStartsHook(void* agent); + // Network tracking uses JS hooks. Reconcile the protocol requested and // applied states after leaving a V8 interrupt. JavaScriptHookState network_tracking_state_; diff --git a/src/node.h b/src/node.h index 8e7d1e6a2516..a898f41b33f9 100644 --- a/src/node.h +++ b/src/node.h @@ -210,8 +210,10 @@ enum Flags : uint32_t { kNoICU = 1 << 3, // Do not modify stdio file descriptor or TTY state. kNoStdioInitialization = 1 << 4, - // Do not register Node.js-specific signal handlers - // and reset other signal handlers to default state. + // Do not register Node.js-specific signal handlers, reset other signal + // handlers to default state, or replace the calling thread's signal mask + // (without this flag, POSIX builds with the inspector set it to block + // SIGUSR1 and nothing else). kNoDefaultSignalHandling = 1 << 5, // Do not perform V8 initialization. kNoInitializeV8 = 1 << 6, diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc index fe9915185b71..a1de1b8c0fb9 100644 --- a/test/cctest/test_environment.cc +++ b/test/cctest/test_environment.cc @@ -341,9 +341,8 @@ TEST_F(EnvironmentTest, RemoveEnvironmentCleanupHookDuringCleanup) { TEST_F(EnvironmentTest, MultipleEnvironmentsPerIsolate) { const v8::HandleScope handle_scope(isolate_); const Argv argv; - // Only one of the Environments can have default flags and own the inspector. Env env1 {handle_scope, argv}; - Env env2 {handle_scope, argv, node::EnvironmentFlags::kNoFlags}; + Env env2{handle_scope, argv}; AtExit(*env1, at_exit_callback1, nullptr); AtExit(*env2, at_exit_callback2, nullptr);