src: refactor default trace writer out of agent

The agent code is supposed to manage multiple writers/clients.
Adding the default writer into the mix breaks that encapsulation.
Instead, manage default options through a separate "virtual"
default client handle, and keep the file writer management
all to the actual users.

PR-URL: https://github.com/nodejs/node/pull/21867
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>

10 files changed, 130 insertions, 87 deletions

diff --git a/src/env-inl.h b/src/env-inl.h
index 31c478b8c3..c38e37f786 100644
--- a/src/env-inl.h
+++ b/src/env-inl.h
@@ -325,8 +325,8 @@ inline v8::Isolate* Environment::isolate() const {
   return isolate_;
 }
 
-inline tracing::Agent* Environment::tracing_agent() const {
-  return tracing_agent_;
+inline tracing::AgentWriterHandle* Environment::tracing_agent_writer() const {
+  return tracing_agent_writer_;
 }
 
 inline Environment* Environment::from_timer_handle(uv_timer_t* handle) {
diff --git a/src/env.cc b/src/env.cc
index 769676275b..244c6d8be3 100644
--- a/src/env.cc
+++ b/src/env.cc
@@ -105,10 +105,10 @@ void InitThreadLocalOnce() {
 
 Environment::Environment(IsolateData* isolate_data,
                          Local<Context> context,
-                         tracing::Agent* tracing_agent)
+                         tracing::AgentWriterHandle* tracing_agent_writer)
     : isolate_(context->GetIsolate()),
       isolate_data_(isolate_data),
-      tracing_agent_(tracing_agent),
+      tracing_agent_writer_(tracing_agent_writer),
       immediate_info_(context->GetIsolate()),
       tick_info_(context->GetIsolate()),
       timer_base_(uv_now(isolate_data->event_loop())),
diff --git a/src/env.h b/src/env.h
index 29f84a4af1..91ae782428 100644
--- a/src/env.h
+++ b/src/env.h
@@ -55,7 +55,7 @@ class performance_state;
 }
 
 namespace tracing {
-class Agent;
+class AgentWriterHandle;
 }
 
 namespace worker {
@@ -587,7 +587,7 @@ class Environment {
 
   Environment(IsolateData* isolate_data,
               v8::Local<v8::Context> context,
-              tracing::Agent* tracing_agent);
+              tracing::AgentWriterHandle* tracing_agent_writer);
   ~Environment();
 
   void Start(int argc,
@@ -625,7 +625,7 @@ class Environment {
   inline bool profiler_idle_notifier_started() const;
 
   inline v8::Isolate* isolate() const;
-  inline tracing::Agent* tracing_agent() const;
+  inline tracing::AgentWriterHandle* tracing_agent_writer() const;
   inline uv_loop_t* event_loop() const;
   inline uint32_t watched_providers() const;
 
@@ -879,7 +879,7 @@ class Environment {
 
   v8::Isolate* const isolate_;
   IsolateData* const isolate_data_;
-  tracing::Agent* const tracing_agent_;
+  tracing::AgentWriterHandle* const tracing_agent_writer_;
   uv_timer_t timer_handle_;
   uv_check_t immediate_check_handle_;
   uv_idle_t immediate_idle_handle_;
diff --git a/src/inspector/tracing_agent.cc b/src/inspector/tracing_agent.cc
index 92c3597590..6e962b289a 100644
--- a/src/inspector/tracing_agent.cc
+++ b/src/inspector/tracing_agent.cc
@@ -74,10 +74,11 @@ DispatchResponse TracingAgent::start(
   if (categories_set.empty())
     return DispatchResponse::Error("At least one category should be enabled");
 
-  trace_writer_ = env_->tracing_agent()->AddClient(
+  trace_writer_ = env_->tracing_agent_writer()->agent()->AddClient(
       categories_set,
       std::unique_ptr<InspectorTraceWriter>(
-          new InspectorTraceWriter(frontend_.get())));
+          new InspectorTraceWriter(frontend_.get())),
+      tracing::Agent::kIgnoreDefaultCategories);
   return DispatchResponse::OK();
 }
 
diff --git a/src/node.cc b/src/node.cc
index b5b7ef1a5b..a7b7f45b30 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -61,6 +61,7 @@
 #include "req_wrap-inl.h"
 #include "string_bytes.h"
 #include "tracing/agent.h"
+#include "tracing/node_trace_writer.h"
 #include "util.h"
 #include "uv.h"
 #if NODE_USE_V8_PLATFORM
@@ -427,17 +428,24 @@ static struct {
 #endif  // HAVE_INSPECTOR
 
   void StartTracingAgent() {
-    tracing_file_writer_ = tracing_agent_->AddClient(
-        trace_enabled_categories,
-        new tracing::NodeTraceWriter(trace_file_pattern));
+    if (trace_enabled_categories.empty()) {
+      tracing_file_writer_ = tracing_agent_->DefaultHandle();
+    } else {
+      tracing_file_writer_ = tracing_agent_->AddClient(
+          ParseCommaSeparatedSet(trace_enabled_categories),
+          std::unique_ptr<tracing::AsyncTraceWriter>(
+              new tracing::NodeTraceWriter(trace_file_pattern,
+                                           tracing_agent_->loop())),
+          tracing::Agent::kUseDefaultCategories);
+    }
   }
 
   void StopTracingAgent() {
     tracing_file_writer_.reset();
   }
 
-  tracing::Agent* GetTracingAgent() const {
-    return tracing_agent_.get();
+  tracing::AgentWriterHandle* GetTracingAgentWriter() {
+    return &tracing_file_writer_;
   }
 
   NodePlatform* Platform() {
@@ -466,7 +474,9 @@ static struct {
   }
   void StopTracingAgent() {}
 
-  tracing::Agent* GetTracingAgent() const { return nullptr; }
+  tracing::AgentWriterHandle* GetTracingAgentWriter() {
+    return nullptr;
+  }
 
   NodePlatform* Platform() {
     return nullptr;
@@ -3590,7 +3600,7 @@ Environment* CreateEnvironment(IsolateData* isolate_data,
   HandleScope handle_scope(isolate);
   Context::Scope context_scope(context);
   auto env = new Environment(isolate_data, context,
-                             v8_platform.GetTracingAgent());
+                             v8_platform.GetTracingAgentWriter());
   env->Start(argc, argv, exec_argc, exec_argv, v8_is_profiling);
   return env;
 }
@@ -3649,7 +3659,7 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data,
   HandleScope handle_scope(isolate);
   Local<Context> context = NewContext(isolate);
   Context::Scope context_scope(context);
-  Environment env(isolate_data, context, v8_platform.GetTracingAgent());
+  Environment env(isolate_data, context, v8_platform.GetTracingAgentWriter());
   env.Start(argc, argv, exec_argc, exec_argv, v8_is_profiling);
 
   const char* path = argc > 1 ? argv[1] : nullptr;
diff --git a/src/node_trace_events.cc b/src/node_trace_events.cc
index 06063a449b..d59b925557 100644
--- a/src/node_trace_events.cc
+++ b/src/node_trace_events.cc
@@ -58,7 +58,7 @@ void NodeCategorySet::New(const FunctionCallbackInfo<Value>& args) {
     if (!*val) return;
     categories.emplace(*val);
   }
-  CHECK_NOT_NULL(env->tracing_agent());
+  CHECK_NOT_NULL(env->tracing_agent_writer());
   new NodeCategorySet(env, args.This(), std::move(categories));
 }
 
@@ -69,7 +69,7 @@ void NodeCategorySet::Enable(const FunctionCallbackInfo<Value>& args) {
   CHECK_NOT_NULL(category_set);
   const auto& categories = category_set->GetCategories();
   if (!category_set->enabled_ && !categories.empty()) {
-    env->tracing_agent()->Enable(categories);
+    env->tracing_agent_writer()->Enable(categories);
     category_set->enabled_ = true;
   }
 }
@@ -81,14 +81,15 @@ void NodeCategorySet::Disable(const FunctionCallbackInfo<Value>& args) {
   CHECK_NOT_NULL(category_set);
   const auto& categories = category_set->GetCategories();
   if (category_set->enabled_ && !categories.empty()) {
-    env->tracing_agent()->Disable(categories);
+    env->tracing_agent_writer()->Disable(categories);
     category_set->enabled_ = false;
   }
 }
 
 void GetEnabledCategories(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
-  std::string categories = env->tracing_agent()->GetEnabledCategories();
+  std::string categories =
+      env->tracing_agent_writer()->agent()->GetEnabledCategories();
   if (!categories.empty()) {
     args.GetReturnValue().Set(
       String::NewFromUtf8(env->isolate(),
diff --git a/src/tracing/agent.cc b/src/tracing/agent.cc
index 9cc21863e3..641c476c1d 100644
--- a/src/tracing/agent.cc
+++ b/src/tracing/agent.cc
@@ -1,23 +1,24 @@
 #include "tracing/agent.h"
 
-#include <sstream>
 #include <string>
 #include "tracing/node_trace_buffer.h"
-#include "tracing/node_trace_writer.h"
 
 namespace node {
 namespace tracing {
 
-namespace {
-
-class ScopedSuspendTracing {
+class Agent::ScopedSuspendTracing {
  public:
-  ScopedSuspendTracing(TracingController* controller, Agent* agent)
-                       : controller_(controller), agent_(agent) {
-    controller->StopTracing();
+  ScopedSuspendTracing(TracingController* controller, Agent* agent,
+                       bool do_suspend = true)
+    : controller_(controller), agent_(do_suspend ? agent : nullptr) {
+    if (do_suspend) {
+      CHECK(agent_->started_);
+      controller->StopTracing();
+    }
   }
 
   ~ScopedSuspendTracing() {
+    if (agent_ == nullptr) return;
     TraceConfig* config = agent_->CreateTraceConfig();
     if (config != nullptr) {
       controller_->StartTracing(config);
@@ -29,8 +30,10 @@ class ScopedSuspendTracing {
   Agent* agent_;
 };
 
+namespace {
+
 std::set<std::string> flatten(
-    const std::unordered_map<int, std::set<std::string>>& map) {
+    const std::unordered_map<int, std::multiset<std::string>>& map) {
   std::set<std::string> result;
   for (const auto& id_value : map)
     result.insert(id_value.second.begin(), id_value.second.end());
@@ -43,18 +46,17 @@ using v8::platform::tracing::TraceConfig;
 using v8::platform::tracing::TraceWriter;
 using std::string;
 
-Agent::Agent(const std::string& log_file_pattern)
-    : log_file_pattern_(log_file_pattern) {
+Agent::Agent() {
   tracing_controller_ = new TracingController();
   tracing_controller_->Initialize(nullptr);
+
+  CHECK_EQ(uv_loop_init(&tracing_loop_), 0);
 }
 
 void Agent::Start() {
   if (started_)
     return;
 
-  CHECK_EQ(uv_loop_init(&tracing_loop_), 0);
-
   NodeTraceBuffer* trace_buffer_ = new NodeTraceBuffer(
       NodeTraceBuffer::kBufferChunks, this, &tracing_loop_);
   tracing_controller_->Initialize(trace_buffer_);
@@ -71,18 +73,30 @@ void Agent::Start() {
 
 AgentWriterHandle Agent::AddClient(
     const std::set<std::string>& categories,
-    std::unique_ptr<AsyncTraceWriter> writer) {
+    std::unique_ptr<AsyncTraceWriter> writer,
+    enum UseDefaultCategoryMode mode) {
   Start();
+
+  const std::set<std::string>* use_categories = &categories;
+
+  std::set<std::string> categories_with_default;
+  if (mode == kUseDefaultCategories) {
+    categories_with_default.insert(categories.begin(), categories.end());
+    categories_with_default.insert(categories_[kDefaultHandleId].begin(),
+                                   categories_[kDefaultHandleId].end());
+    use_categories = &categories_with_default;
+  }
+
   ScopedSuspendTracing suspend(tracing_controller_, this);
   int id = next_writer_id_++;
   writers_[id] = std::move(writer);
-  categories_[id] = categories;
+  categories_[id] = { use_categories->begin(), use_categories->end() };
 
   return AgentWriterHandle(this, id);
 }
 
-void Agent::Stop() {
-  file_writer_.reset();
+AgentWriterHandle Agent::DefaultHandle() {
+  return AgentWriterHandle(this, kDefaultHandleId);
 }
 
 void Agent::StopTracing() {
@@ -99,54 +113,30 @@ void Agent::StopTracing() {
 }
 
 void Agent::Disconnect(int client) {
+  if (client == kDefaultHandleId) return;
   ScopedSuspendTracing suspend(tracing_controller_, this);
   writers_.erase(client);
   categories_.erase(client);
 }
 
-void Agent::Enable(const std::string& categories) {
+void Agent::Enable(int id, const std::set<std::string>& categories) {
   if (categories.empty())
     return;
-  std::set<std::string> categories_set;
-  std::istringstream category_list(categories);
-  while (category_list.good()) {
-    std::string category;
-    getline(category_list, category, ',');
-    categories_set.emplace(std::move(category));
-  }
-  Enable(categories_set);
-}
 
-void Agent::Enable(const std::set<std::string>& categories) {
-  if (categories.empty())
-    return;
-
-  file_writer_categories_.insert(categories.begin(), categories.end());
-  std::set<std::string> full_list(file_writer_categories_.begin(),
-                                  file_writer_categories_.end());
-  if (file_writer_.empty()) {
-    // Ensure background thread is running
-    Start();
-    std::unique_ptr<NodeTraceWriter> writer(
-        new NodeTraceWriter(log_file_pattern_, &tracing_loop_));
-    file_writer_ = AddClient(full_list, std::move(writer));
-  } else {
-    ScopedSuspendTracing suspend(tracing_controller_, this);
-    categories_[file_writer_.id_] = full_list;
-  }
+  ScopedSuspendTracing suspend(tracing_controller_, this,
+                               id != kDefaultHandleId);
+  categories_[id].insert(categories.begin(), categories.end());
 }
 
-void Agent::Disable(const std::set<std::string>& categories) {
+void Agent::Disable(int id, const std::set<std::string>& categories) {
+  ScopedSuspendTracing suspend(tracing_controller_, this,
+                               id != kDefaultHandleId);
+  std::multiset<std::string>& writer_categories = categories_[id];
   for (const std::string& category : categories) {
-    auto it = file_writer_categories_.find(category);
-    if (it != file_writer_categories_.end())
-      file_writer_categories_.erase(it);
+    auto it = writer_categories.find(category);
+    if (it != writer_categories.end())
+      writer_categories.erase(it);
   }
-  if (file_writer_.empty())
-    return;
-  ScopedSuspendTracing suspend(tracing_controller_, this);
-  categories_[file_writer_.id_] = { file_writer_categories_.begin(),
-                                    file_writer_categories_.end() };
 }
 
 TraceConfig* Agent::CreateTraceConfig() const {
@@ -178,5 +168,6 @@ void Agent::Flush(bool blocking) {
   for (const auto& id_writer : writers_)
     id_writer.second->Flush(blocking);
 }
+
 }  // namespace tracing
 }  // namespace node
diff --git a/src/tracing/agent.h b/src/tracing/agent.h
index 022e86f11a..b62dc5fe5b 100644
--- a/src/tracing/agent.h
+++ b/src/tracing/agent.h
@@ -44,6 +44,11 @@ class AgentWriterHandle {
   inline bool empty() const { return agent_ == nullptr; }
   inline void reset();
 
+  inline void Enable(const std::set<std::string>& categories);
+  inline void Disable(const std::set<std::string>& categories);
+
+  inline Agent* agent() { return agent_; }
+
  private:
   inline AgentWriterHandle(Agent* agent, int id) : agent_(agent), id_(id) {}
 
@@ -58,19 +63,23 @@ class AgentWriterHandle {
 
 class Agent {
  public:
-  explicit Agent(const std::string& log_file_pattern);
-  void Stop();
+  Agent();
 
   TracingController* GetTracingController() { return tracing_controller_; }
 
+  enum UseDefaultCategoryMode {
+    kUseDefaultCategories,
+    kIgnoreDefaultCategories
+  };
+
   // Destroying the handle disconnects the client
   AgentWriterHandle 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);
-  void Enable(const std::set<std::string>& categories);
-  void Disable(const std::set<std::string>& categories);
+                              std::unique_ptr<AsyncTraceWriter> writer,
+                              enum UseDefaultCategoryMode mode);
+  // A handle that is only used for managing the default categories
+  // (which can then implicitly be used through using `USE_DEFAULT_CATEGORIES`
+  // when adding a client later).
+  AgentWriterHandle DefaultHandle();
 
   // Returns a comma-separated list of enabled categories.
   std::string GetEnabledCategories() const;
@@ -82,6 +91,9 @@ class Agent {
 
   TraceConfig* CreateTraceConfig() const;
 
+  // TODO(addaleax): This design is broken and inherently thread-unsafe.
+  inline uv_loop_t* loop() { return &tracing_loop_; }
+
  private:
   friend class AgentWriterHandle;
 
@@ -91,19 +103,22 @@ class Agent {
   void StopTracing();
   void Disconnect(int client);
 
-  const std::string& log_file_pattern_;
+  void Enable(int id, const std::set<std::string>& categories);
+  void Disable(int id, const std::set<std::string>& categories);
+
   uv_thread_t thread_;
   uv_loop_t tracing_loop_;
+
   bool started_ = false;
+  class ScopedSuspendTracing;
 
   // Each individual Writer has one id.
   int next_writer_id_ = 1;
+  enum { kDefaultHandleId = -1 };
   // These maps store the original arguments to AddClient(), by id.
-  std::unordered_map<int, std::set<std::string>> categories_;
+  std::unordered_map<int, std::multiset<std::string>> categories_;
   std::unordered_map<int, std::unique_ptr<AsyncTraceWriter>> writers_;
   TracingController* tracing_controller_ = nullptr;
-  AgentWriterHandle file_writer_;
-  std::multiset<std::string> file_writer_categories_;
 };
 
 void AgentWriterHandle::reset() {
@@ -124,6 +139,14 @@ AgentWriterHandle::AgentWriterHandle(AgentWriterHandle&& other) {
   *this = std::move(other);
 }
 
+void AgentWriterHandle::Enable(const std::set<std::string>& categories) {
+  if (agent_ != nullptr) agent_->Enable(id_, categories);
+}
+
+void AgentWriterHandle::Disable(const std::set<std::string>& categories) {
+  if (agent_ != nullptr) agent_->Disable(id_, categories);
+}
+
 }  // namespace tracing
 }  // namespace node
 
diff --git a/src/util.cc b/src/util.cc
index 3e808e13fe..66be18eae2 100644
--- a/src/util.cc
+++ b/src/util.cc
@@ -24,6 +24,7 @@
 #include "node_internals.h"
 #include "uv.h"
 #include <stdio.h>
+#include <sstream>
 
 namespace node {
 
@@ -118,4 +119,17 @@ void GetHumanReadableProcessName(char (*name)[1024]) {
   snprintf(*name, sizeof(*name), "%s[%d]", title, uv_os_getpid());
 }
 
+std::set<std::string> ParseCommaSeparatedSet(const std::string& in) {
+  std::set<std::string> out;
+  if (in.empty())
+    return out;
+  std::istringstream in_stream(in);
+  while (in_stream.good()) {
+    std::string item;
+    getline(in_stream, item, ',');
+    out.emplace(std::move(item));
+  }
+  return out;
+}
+
 }  // namespace node
diff --git a/src/util.h b/src/util.h
index f7dcc5ea35..a9fce79ebe 100644
--- a/src/util.h
+++ b/src/util.h
@@ -36,6 +36,7 @@
 
 #include <string>
 #include <functional>  // std::function
+#include <set>
 
 namespace node {
 
@@ -476,6 +477,8 @@ struct FunctionDeleter {
 template <typename T, void (*function)(T*)>
 using DeleteFnPtr = typename FunctionDeleter<T, function>::Pointer;
 
+std::set<std::string> ParseCommaSeparatedSet(const std::string& in);
+
 }  // namespace node
 
 #endif  // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS