trace_events: destroy platform before tracing

For safer shutdown, we should destroy the platform – and background threads - before the tracing infrastructure is destroyed. This change fixes the relative order of NodePlatform disposition and the tracing agent shutting down. This matches the nesting order for startup. Make the tracing agent own the tracing controller instead of platform to match the above. Fixes: #22865 PR-URL: #22938 Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
nodejs · Oct 13, 2018 · afca579 · afca579
1 parent 63f0e77
commit afca579
Show file tree

Hide file tree

Showing 5 changed files with 18 additions and 14 deletions.
diff --git a/src/node.cc b/src/node.cc
@@ -279,15 +279,18 @@ static struct {
     controller->AddTraceStateObserver(new NodeTraceStateObserver(controller));
     tracing::TraceEventHelper::SetTracingController(controller);
     StartTracingAgent();
+    // Tracing must be initialized before platform threads are created.
     platform_ = new NodePlatform(thread_pool_size, controller);
     V8::InitializePlatform(platform_);
   }
 
   void Dispose() {
-    tracing_agent_.reset(nullptr);
     platform_->Shutdown();
     delete platform_;
     platform_ = nullptr;
+    // Destroy tracing after the platform (and platform threads) have been
+    // stopped.
+    tracing_agent_.reset(nullptr);
   }
 
   void DrainVMTasks(Isolate* isolate) {

diff --git a/src/node_platform.cc b/src/node_platform.cc
@@ -280,10 +280,9 @@ int PerIsolatePlatformData::unref() {
 NodePlatform::NodePlatform(int thread_pool_size,
                            TracingController* tracing_controller) {
   if (tracing_controller) {
-    tracing_controller_.reset(tracing_controller);
+    tracing_controller_ = tracing_controller;
   } else {
-    TracingController* controller = new TracingController();
-    tracing_controller_.reset(controller);
+    tracing_controller_ = new TracingController();
   }
   background_task_runner_ =
       std::make_shared<BackgroundTaskRunner>(thread_pool_size);
@@ -457,7 +456,7 @@ double NodePlatform::CurrentClockTimeMillis() {
 }
 
 TracingController* NodePlatform::GetTracingController() {
-  return tracing_controller_.get();
+  return tracing_controller_;
 }
 
 template <class T>

diff --git a/src/node_platform.h b/src/node_platform.h
@@ -161,7 +161,8 @@ class NodePlatform : public MultiIsolatePlatform {
   std::unordered_map<v8::Isolate*,
                      std::shared_ptr<PerIsolatePlatformData>> per_isolate_;
 
-  std::unique_ptr<v8::TracingController> tracing_controller_;
+
+  v8::TracingController* tracing_controller_;
   std::shared_ptr<BackgroundTaskRunner> background_task_runner_;
 };
 

diff --git a/src/tracing/agent.cc b/src/tracing/agent.cc
@@ -48,8 +48,7 @@ using v8::platform::tracing::TraceConfig;
 using v8::platform::tracing::TraceWriter;
 using std::string;
 
-Agent::Agent() {
-  tracing_controller_ = new TracingController();
+Agent::Agent() : tracing_controller_(new TracingController()) {
   tracing_controller_->Initialize(nullptr);
 
   CHECK_EQ(uv_loop_init(&tracing_loop_), 0);
@@ -117,7 +116,7 @@ AgentWriterHandle Agent::AddClient(
     use_categories = &categories_with_default;
   }
 
-  ScopedSuspendTracing suspend(tracing_controller_, this);
+  ScopedSuspendTracing suspend(tracing_controller_.get(), this);
   int id = next_writer_id_++;
   AsyncTraceWriter* raw = writer.get();
   writers_[id] = std::move(writer);
@@ -157,7 +156,7 @@ void Agent::Disconnect(int client) {
     Mutex::ScopedLock lock(initialize_writer_mutex_);
     to_be_initialized_.erase(writers_[client].get());
   }
-  ScopedSuspendTracing suspend(tracing_controller_, this);
+  ScopedSuspendTracing suspend(tracing_controller_.get(), this);
   writers_.erase(client);
   categories_.erase(client);
 }
@@ -166,13 +165,13 @@ void Agent::Enable(int id, const std::set<std::string>& categories) {
   if (categories.empty())
     return;
 
-  ScopedSuspendTracing suspend(tracing_controller_, this,
+  ScopedSuspendTracing suspend(tracing_controller_.get(), this,
                                id != kDefaultHandleId);
   categories_[id].insert(categories.begin(), categories.end());
 }
 
 void Agent::Disable(int id, const std::set<std::string>& categories) {
-  ScopedSuspendTracing suspend(tracing_controller_, this,
+  ScopedSuspendTracing suspend(tracing_controller_.get(), this,
                                id != kDefaultHandleId);
   std::multiset<std::string>& writer_categories = categories_[id];
   for (const std::string& category : categories) {

diff --git a/src/tracing/agent.h b/src/tracing/agent.h
@@ -68,7 +68,9 @@ class Agent {
   Agent();
   ~Agent();
 
-  TracingController* GetTracingController() { return tracing_controller_; }
+  TracingController* GetTracingController() {
+    return tracing_controller_.get();
+  }
 
   enum UseDefaultCategoryMode {
     kUseDefaultCategories,
@@ -119,7 +121,7 @@ class Agent {
   // These maps store the original arguments to AddClient(), by id.
   std::unordered_map<int, std::multiset<std::string>> categories_;
   std::unordered_map<int, std::unique_ptr<AsyncTraceWriter>> writers_;
-  TracingController* tracing_controller_ = nullptr;
+  std::unique_ptr<TracingController> tracing_controller_;
 
   // Variables related to initializing per-event-loop properties of individual
   // writers, such as libuv handles.