tracing: prevent race conditions during shutdown

src/inspector/tracing_agent.cc

@@ -45,10 +45,7 @@ class InspectorTraceWriter : public node::tracing::AsyncTraceWriter {
 };
 }  // namespace
 
-TracingAgent::TracingAgent(Environment* env)
-                           : env_(env),
-                             trace_writer_(
-                                  tracing::Agent::EmptyClientHandle()) {
+TracingAgent::TracingAgent(Environment* env) : env_(env) {
 }
 
 TracingAgent::~TracingAgent() {

src/inspector/tracing_agent.h

@@ -9,6 +9,7 @@ namespace node {
 class Environment;
 
 namespace tracing {
+class ClientHandle;
 class Agent;
 }  // namespace tracing
 
@@ -32,8 +33,7 @@ class TracingAgent : public NodeTracing::Backend {
   void DisconnectTraceClient();
 
   Environment* env_;
-  std::unique_ptr<std::pair<tracing::Agent*, int>,
-                  void (*)(std::pair<tracing::Agent*, int>*)> trace_writer_;
+  std::unique_ptr<tracing::ClientHandle> trace_writer_;
   std::unique_ptr<NodeTracing::Frontend> frontend_;
 };
 

src/tracing/agent.cc

@@ -44,19 +44,23 @@ using v8::platform::tracing::TraceWriter;
 using std::string;
 
 Agent::Agent(const std::string& log_file_pattern)
-    : log_file_pattern_(log_file_pattern), file_writer_(EmptyClientHandle()) {
+    : log_file_pattern_(log_file_pattern),
+      handle_(std::make_shared<AgentHandle>(this)) {
   tracing_controller_ = new TracingController();
   tracing_controller_->Initialize(nullptr);
 }
 
+Agent::~Agent() {
+  handle_->Reset();
+}
+
 void Agent::Start() {
   if (started_)
     return;
 
   CHECK_EQ(uv_loop_init(&tracing_loop_), 0);
-
   NodeTraceBuffer* trace_buffer_ = new NodeTraceBuffer(
-      NodeTraceBuffer::kBufferChunks, this, &tracing_loop_);
+      NodeTraceBuffer::kBufferChunks, handle_, &tracing_loop_);
   tracing_controller_->Initialize(trace_buffer_);
 
   // This thread should be created *after* async handles are created
@@ -66,16 +70,15 @@ void Agent::Start() {
   started_ = true;
 }
 
-Agent::ClientHandle Agent::AddClient(const std::set<std::string>& categories,
-                                     std::unique_ptr<AsyncTraceWriter> writer) {
+std::unique_ptr<ClientHandle> Agent::AddClient(
+    const std::set<std::string>& categories,
+    std::unique_ptr<AsyncTraceWriter> writer) {
   Start();
   ScopedSuspendTracing suspend(tracing_controller_, this);
   int id = next_writer_id_++;
   writers_[id] = std::move(writer);
   categories_[id] = categories;
-
-  auto client_id = new std::pair<Agent*, int>(this, id);
-  return ClientHandle(client_id, &DisconnectClient);
+  return std::unique_ptr<ClientHandle>(new ClientHandle(handle_, id));
 }
 
 void Agent::Stop() {
@@ -138,7 +141,7 @@ void Agent::Enable(const std::set<std::string>& categories) {
     file_writer_ = AddClient(full_list, std::move(writer));
   } else {
     ScopedSuspendTracing suspend(tracing_controller_, this);
-    categories_[file_writer_->second] = full_list;
+    categories_[file_writer_->id()] = full_list;
   }
 }
 
@@ -151,8 +154,8 @@ void Agent::Disable(const std::set<std::string>& categories) {
   if (!file_writer_)
     return;
   ScopedSuspendTracing suspend(tracing_controller_, this);
-  categories_[file_writer_->second] = { file_writer_categories_.begin(),
-                                        file_writer_categories_.end() };
+  categories_[file_writer_->id()] = { file_writer_categories_.begin(),
+                                      file_writer_categories_.end() };
 }
 
 TraceConfig* Agent::CreateTraceConfig() {
@@ -184,5 +187,37 @@ void Agent::Flush(bool blocking) {
   for (const auto& id_writer : writers_)
     id_writer.second->Flush(blocking);
 }
+
+ClientHandle::~ClientHandle() {
+  agent_->Disconnect(id_);
+}
+
+AgentHandle::AgentHandle(Agent* agent) : agent_(agent) {}
+
+void AgentHandle::Reset() {
+  Mutex::ScopedLock scoped_lock(mutex_);
+  agent_ = nullptr;
+}
+
+void AgentHandle::AppendTraceEvent(TraceObject* trace_event) {
+  Mutex::ScopedLock scoped_lock(mutex_);
+  if (agent_ != nullptr) {
+    agent_->AppendTraceEvent(trace_event);
+  }
+}
+
+void AgentHandle::Flush(bool blocking) {
+  Mutex::ScopedLock scoped_lock(mutex_);
+  if (agent_ != nullptr) {
+    agent_->Flush(blocking);
+  }
+}
+
+void AgentHandle::Disconnect(int client) {
+  // This is only called from a main thread, same as Reset
+  if (agent_ != nullptr)
+    agent_->Disconnect(client);
+}
+
 }  // namespace tracing
 }  // namespace node

src/tracing/agent.h

@@ -2,6 +2,7 @@
 #define SRC_TRACING_AGENT_H_
 
 #include "libplatform/v8-tracing.h"
+#include "node_mutex.h"
 #include "uv.h"
 #include "v8.h"
 
@@ -31,24 +32,49 @@ class TracingController : public v8::platform::tracing::TracingController {
   }
 };
 
+class Agent;
 
-class Agent {
+// Allows agent to be destroyed early. Methods are noop after the agent was
+// destroyed
+class AgentHandle {
  public:
-  // Resetting the pointer disconnects client
-  using ClientHandle = std::unique_ptr<std::pair<Agent*, int>,
-                                       void (*)(std::pair<Agent*, int>*)>;
+  explicit AgentHandle(Agent* agent);
+  void Reset();
+  void AppendTraceEvent(TraceObject* trace_event);
+  void Flush(bool blocking);
+  void Disconnect(int client);
 
-  static ClientHandle EmptyClientHandle() {
-    return ClientHandle(nullptr, DisconnectClient);
-  }
+ private:
+  Mutex mutex_;
+  Agent* agent_;
+};
+
+class ClientHandle {
+ public:
+  ClientHandle(std::shared_ptr<AgentHandle> agent, int id)
+               : agent_(agent), id_(id) {}
+  ClientHandle(const ClientHandle&) = delete;
+  ClientHandle& operator=(const ClientHandle&) = delete;
+  ~ClientHandle();
+  int id() const { return id_; }
+
+ private:
+  std::shared_ptr<AgentHandle> agent_;
+  const int id_;
+};
+
+class Agent {
+ public:
   explicit Agent(const std::string& log_file_pattern);
+  ~Agent();
   void Stop();
 
   TracingController* GetTracingController() { return tracing_controller_; }
 
   // Destroying the handle disconnects the client
-  ClientHandle AddClient(const std::set<std::string>& categories,
-                         std::unique_ptr<AsyncTraceWriter> writer);
+  std::unique_ptr<ClientHandle> AddClient(
+      const std::set<std::string>& categories,
+      std::unique_ptr<AsyncTraceWriter> writer);
 
   // These 3 methods operate on a "default" client, e.g. the file writer
   void Enable(const std::string& categories);
@@ -60,17 +86,13 @@ class Agent {
   void Flush(bool blocking);
 
   TraceConfig* CreateTraceConfig();
+  void Disconnect(int client);
 
  private:
   static void ThreadCb(void* arg);
-  static void DisconnectClient(std::pair<Agent*, int>* id_agent) {
-    id_agent->first->Disconnect(id_agent->second);
-    delete id_agent;
-  }
 
   void Start();
   void StopTracing();
-  void Disconnect(int client);
 
   const std::string& log_file_pattern_;
   uv_thread_t thread_;
@@ -79,10 +101,11 @@ class Agent {
 
   std::unordered_map<int, std::set<std::string>> categories_;
   TracingController* tracing_controller_ = nullptr;
-  ClientHandle file_writer_;
+  std::unique_ptr<ClientHandle> file_writer_;
   int next_writer_id_ = 1;
   std::unordered_map<int, std::unique_ptr<AsyncTraceWriter>> writers_;
   std::multiset<std::string> file_writer_categories_;
+  std::shared_ptr<AgentHandle> handle_;
 };
 
 }  // namespace tracing

src/tracing/node_trace_buffer.cc

@@ -4,7 +4,7 @@ namespace node {
 namespace tracing {
 
 InternalTraceBuffer::InternalTraceBuffer(size_t max_chunks, uint32_t id,
-                                         Agent* agent)
+                                         std::shared_ptr<AgentHandle> agent)
     : flushing_(false), max_chunks_(max_chunks),
       agent_(agent), id_(id) {
   chunks_.resize(max_chunks);
@@ -87,7 +87,7 @@ void InternalTraceBuffer::ExtractHandle(
 }
 
 NodeTraceBuffer::NodeTraceBuffer(size_t max_chunks,
-    Agent* agent, uv_loop_t* tracing_loop)
+    std::shared_ptr<AgentHandle> agent, uv_loop_t* tracing_loop)
     : tracing_loop_(tracing_loop),
       buffer1_(max_chunks, 0, agent),
       buffer2_(max_chunks, 1, agent) {

src/tracing/node_trace_buffer.h

@@ -19,7 +19,8 @@ class NodeTraceBuffer;
 
 class InternalTraceBuffer {
  public:
-  InternalTraceBuffer(size_t max_chunks, uint32_t id, Agent* agent);
+  InternalTraceBuffer(size_t max_chunks, uint32_t id,
+                      std::shared_ptr<AgentHandle> agent);
 
   TraceObject* AddTraceEvent(uint64_t* handle);
   TraceObject* GetEventByHandle(uint64_t handle);
@@ -41,7 +42,7 @@ class InternalTraceBuffer {
   Mutex mutex_;
   bool flushing_;
   size_t max_chunks_;
-  Agent* agent_;
+  std::shared_ptr<AgentHandle> agent_;
   std::vector<std::unique_ptr<TraceBufferChunk>> chunks_;
   size_t total_chunks_ = 0;
   uint32_t current_chunk_seq_ = 1;
@@ -50,7 +51,9 @@ class InternalTraceBuffer {
 
 class NodeTraceBuffer : public TraceBuffer {
  public:
-  NodeTraceBuffer(size_t max_chunks, Agent* agent, uv_loop_t* tracing_loop);
+  NodeTraceBuffer(size_t max_chunks,
+                  std::shared_ptr<AgentHandle> agent,
+                  uv_loop_t* tracing_loop);
   ~NodeTraceBuffer();
 
   TraceObject* AddTraceEvent(uint64_t* handle) override;

test/parallel/test-tracing-no-crash.js

@@ -0,0 +1,16 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+const { spawn } = require('child_process');
+
+common.crashOnUnhandledRejection();
+
+function CheckNoSignalAndErrorCodeOne(code, signal) {
+  assert.strictEqual(null, signal);
+  assert.strictEqual(1, code);
+}
+
+const child = spawn(process.execPath,
+                    ['--trace-event-categories', 'madeup', '-e',
+                     'throw new Error()']);
+child.on('exit', common.mustCall(CheckNoSignalAndErrorCodeOne));