From b2c1bb381fc9bb9aa882d29a3ef86fc6650c7535 Mon Sep 17 00:00:00 2001
From: Santiago Gimeno <santiago.gimeno@gmail.com>
Date: Thu, 23 Nov 2023 12:57:05 +0100
Subject: [PATCH] agents: fix OTLPAgent race conditions on cleanup

Move the `exit_lock_` out of the class and make it static so we can use
it even if the agent has been deleted.
Also, make sure we use the `exit_lock_` in `OTLPAgent::config_msg_cb_`.

Fixes: https://github.com/nodesource/nsolid/issues/29
PR-URL: https://github.com/nodesource/nsolid/pull/30
Reviewed-by: Trevor Norris <trev.norris@gmail.com>
---
 agents/otlp/src/otlp_agent.cc | 24 +++++++++++++++---------
 agents/otlp/src/otlp_agent.h  |  2 --
 2 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/agents/otlp/src/otlp_agent.cc b/agents/otlp/src/otlp_agent.cc
index a0ac78bdf8e..b90c37cd1a0 100644
--- a/agents/otlp/src/otlp_agent.cc
+++ b/agents/otlp/src/otlp_agent.cc
@@ -22,6 +22,7 @@ static const size_t kTraceIdSize             = 32;
 static const size_t kSpanIdSize              = 16;
 
 static std::atomic<bool> is_running_ = { false };
+nsuv::ns_rwlock exit_lock_;
 static resource::ResourceAttributes resource_attributes_;
 
 namespace node {
@@ -203,7 +204,7 @@ int OTLPAgent::config(const nlohmann::json& config) {
 /*static*/ void OTLPAgent::config_agent_cb_(std::string config,
                                             OTLPAgent* agent) {
   // Check if the agent is already delete or if it's closing
-  nsuv::ns_rwlock::scoped_rdlock lock(agent->exit_lock_);
+  nsuv::ns_rwlock::scoped_rdlock lock(exit_lock_);
   if (!is_running_) {
     return;
   }
@@ -217,6 +218,11 @@ int OTLPAgent::config(const nlohmann::json& config) {
 
 
 /*static*/ void OTLPAgent::config_msg_cb_(nsuv::ns_async*, OTLPAgent* agent) {
+  nsuv::ns_rwlock::scoped_rdlock lock(exit_lock_);
+  if (!is_running_) {
+    return;
+  }
+
   json config_msg;
   while (agent->config_msg_q_.dequeue(config_msg)) {
     agent->config(config_msg);
@@ -225,7 +231,7 @@ int OTLPAgent::config(const nlohmann::json& config) {
 
 
 /*static*/ void OTLPAgent::env_msg_cb_(nsuv::ns_async*, OTLPAgent* agent) {
-  nsuv::ns_rwlock::scoped_rdlock lock(agent->exit_lock_);
+  nsuv::ns_rwlock::scoped_rdlock lock(exit_lock_);
   if (!is_running_) {
     return;
   }
@@ -249,7 +255,7 @@ int OTLPAgent::config(const nlohmann::json& config) {
 
 /*static*/ void OTLPAgent::on_thread_add_(SharedEnvInst envinst,
                                           OTLPAgent* agent) {
-  nsuv::ns_rwlock::scoped_rdlock lock(agent->exit_lock_);
+  nsuv::ns_rwlock::scoped_rdlock lock(exit_lock_);
   if (!is_running_) {
     return;
   }
@@ -261,7 +267,7 @@ int OTLPAgent::config(const nlohmann::json& config) {
 
 /*static*/ void OTLPAgent::on_thread_remove_(SharedEnvInst envinst,
                                              OTLPAgent* agent) {
-  nsuv::ns_rwlock::scoped_rdlock lock(agent->exit_lock_);
+  nsuv::ns_rwlock::scoped_rdlock lock(exit_lock_);
   if (!is_running_) {
     return;
   }
@@ -309,7 +315,7 @@ void OTLPAgent::do_stop() {
 void OTLPAgent::trace_hook_(Tracer* tracer,
                             const Tracer::SpanStor& stor,
                             OTLPAgent* agent) {
-  nsuv::ns_rwlock::scoped_rdlock lock(agent->exit_lock_);
+  nsuv::ns_rwlock::scoped_rdlock lock(exit_lock_);
   if (!is_running_) {
     return;
   }
@@ -321,7 +327,7 @@ void OTLPAgent::trace_hook_(Tracer* tracer,
 
 void OTLPAgent::span_msg_cb_(nsuv::ns_async*, OTLPAgent* agent) {
   // Don't exit until all the pending spans are sent
-  nsuv::ns_rwlock::scoped_rdlock lock(agent->exit_lock_);
+  nsuv::ns_rwlock::scoped_rdlock lock(exit_lock_);
   if (!is_running_) {
     return;
   }
@@ -396,7 +402,7 @@ void OTLPAgent::span_msg_cb_(nsuv::ns_async*, OTLPAgent* agent) {
 
 
 /*static*/void OTLPAgent::metrics_timer_cb_(nsuv::ns_timer*, OTLPAgent* agent) {
-  nsuv::ns_rwlock::scoped_rdlock lock(agent->exit_lock_);
+  nsuv::ns_rwlock::scoped_rdlock lock(exit_lock_);
   if (!is_running_) {
     return;
   }
@@ -411,7 +417,7 @@ void OTLPAgent::span_msg_cb_(nsuv::ns_async*, OTLPAgent* agent) {
 
 
 /*static*/void OTLPAgent::metrics_msg_cb_(nsuv::ns_async*, OTLPAgent* agent) {
-  nsuv::ns_rwlock::scoped_rdlock lock(agent->exit_lock_);
+  nsuv::ns_rwlock::scoped_rdlock lock(exit_lock_);
   if (!is_running_) {
     return;
   }
@@ -437,7 +443,7 @@ void OTLPAgent::span_msg_cb_(nsuv::ns_async*, OTLPAgent* agent) {
 
 /*static*/void OTLPAgent::thr_metrics_cb_(ThreadMetrics* metrics,
                                           OTLPAgent* agent) {
-  nsuv::ns_rwlock::scoped_rdlock lock(agent->exit_lock_);
+  nsuv::ns_rwlock::scoped_rdlock lock(exit_lock_);
   if (!is_running_) {
     return;
   }
diff --git a/agents/otlp/src/otlp_agent.h b/agents/otlp/src/otlp_agent.h
index fc45e5a1e83..33f369ecfa4 100644
--- a/agents/otlp/src/otlp_agent.h
+++ b/agents/otlp/src/otlp_agent.h
@@ -112,8 +112,6 @@ class OTLPAgent {
   uv_cond_t start_cond_;
   uv_mutex_t start_lock_;
 
-  nsuv::ns_rwlock exit_lock_;
-
   bool hooks_init_;
 
   nsuv::ns_async env_msg_;