Skip to content

Commit

Permalink
inspector: properly shut down uv_async_t
Browse files Browse the repository at this point in the history
Closing in the Agent destructor is too late, because that happens
when the Environment is destroyed, not when libuv handles are closed.

This fixes a situation in which the same libuv loop is re-used for
multiple Environment instances sequentially, e.g. in our cctest.

PR-URL: #30612
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
  • Loading branch information
addaleax committed Nov 28, 2019
1 parent 7686b5a commit 3e79c00
Showing 1 changed file with 21 additions and 8 deletions.
29 changes: 21 additions & 8 deletions src/inspector_agent.cc
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ 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;

class StartIoTask : public Task {
public:
Expand Down Expand Up @@ -97,6 +99,8 @@ static void StartIoThreadWakeup(int signo, siginfo_t* info, void* ucontext) {
inline void* StartIoThreadMain(void* unused) {
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<Agent*>(start_io_thread_async.data);
if (agent != nullptr)
Expand Down Expand Up @@ -157,6 +161,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<Agent*>(start_io_thread_async.data);
if (agent != nullptr)
Expand Down Expand Up @@ -748,14 +753,7 @@ Agent::Agent(Environment* env)
debug_options_(env->options()->debug_options()),
host_port_(env->inspector_host_port()) {}

Agent::~Agent() {
if (start_io_thread_async.data == this) {
CHECK(start_io_thread_async_initialized.exchange(false));
start_io_thread_async.data = nullptr;
// This is global, will never get freed
uv_close(reinterpret_cast<uv_handle_t*>(&start_io_thread_async), nullptr);
}
}
Agent::~Agent() {}

bool Agent::Start(const std::string& path,
const DebugOptions& options,
Expand All @@ -768,6 +766,7 @@ bool Agent::Start(const std::string& path,

client_ = std::make_shared<NodeInspectorClient>(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,
Expand All @@ -776,6 +775,20 @@ bool Agent::Start(const std::string& path,
start_io_thread_async.data = this;
// Ignore failure, SIGUSR1 won't work, but that should not block node start.
StartDebugSignalHandler();

parent_env_->AddCleanupHook([](void* data) {
Environment* env = static_cast<Environment*>(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_);
}

AtExit(parent_env_, [](void* env) {
Expand Down

0 comments on commit 3e79c00

Please sign in to comment.