From 7139e97e8b667ece3093443d30cff5ae960a21a1 Mon Sep 17 00:00:00 2001 From: Ali Ijaz Sheikh Date: Tue, 18 Sep 2018 14:04:45 -0700 Subject: [PATCH 1/2] trace_event: destroy platform before tracing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: https://github.com/nodejs/node/issues/22865 --- src/node.cc | 5 ++++- src/node_platform.cc | 7 +++---- src/node_platform.h | 2 +- src/tracing/agent.cc | 11 +++++------ src/tracing/agent.h | 4 ++-- 5 files changed, 15 insertions(+), 14 deletions(-) diff --git a/src/node.cc b/src/node.cc index cd9de349bc70e9..4680a3f71f23cb 100644 --- a/src/node.cc +++ b/src/node.cc @@ -297,15 +297,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 index 1c237159f2d2e9..01931475cf9de6 100644 --- a/src/node_platform.cc +++ b/src/node_platform.cc @@ -250,10 +250,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(); } worker_thread_task_runner_ = std::make_shared(thread_pool_size); @@ -423,7 +422,7 @@ double NodePlatform::CurrentClockTimeMillis() { } TracingController* NodePlatform::GetTracingController() { - return tracing_controller_.get(); + return tracing_controller_; } template diff --git a/src/node_platform.h b/src/node_platform.h index 9b9720f63804d5..5dc972aed9f382 100644 --- a/src/node_platform.h +++ b/src/node_platform.h @@ -154,7 +154,7 @@ class NodePlatform : public MultiIsolatePlatform { std::unordered_map> per_isolate_; - std::unique_ptr tracing_controller_; + v8::TracingController* tracing_controller_; std::shared_ptr worker_thread_task_runner_; }; diff --git a/src/tracing/agent.cc b/src/tracing/agent.cc index 5a4d637bda0356..fd5cb3387b14ae 100644 --- 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& 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& categories) { - ScopedSuspendTracing suspend(tracing_controller_, this, + ScopedSuspendTracing suspend(tracing_controller_.get(), this, id != kDefaultHandleId); std::multiset& writer_categories = categories_[id]; for (const std::string& category : categories) { diff --git a/src/tracing/agent.h b/src/tracing/agent.h index e79480a0369ff7..130734bd057dc5 100644 --- a/src/tracing/agent.h +++ b/src/tracing/agent.h @@ -70,7 +70,7 @@ class Agent { Agent(); ~Agent(); - TracingController* GetTracingController() { return tracing_controller_; } + TracingController* GetTracingController() { return tracing_controller_.get(); } enum UseDefaultCategoryMode { kUseDefaultCategories, @@ -121,7 +121,7 @@ class Agent { // These maps store the original arguments to AddClient(), by id. std::unordered_map> categories_; std::unordered_map> writers_; - TracingController* tracing_controller_ = nullptr; + std::unique_ptr tracing_controller_; // Variables related to initializing per-event-loop properties of individual // writers, such as libuv handles. From db9b5ff7bc6d3c60739d42bfa9ae674496fd1167 Mon Sep 17 00:00:00 2001 From: Ali Ijaz Sheikh Date: Tue, 18 Sep 2018 14:39:17 -0700 Subject: [PATCH 2/2] [squash] lint fix --- src/tracing/agent.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/tracing/agent.h b/src/tracing/agent.h index 130734bd057dc5..e9f52abe647a2d 100644 --- a/src/tracing/agent.h +++ b/src/tracing/agent.h @@ -70,7 +70,9 @@ class Agent { Agent(); ~Agent(); - TracingController* GetTracingController() { return tracing_controller_.get(); } + TracingController* GetTracingController() { + return tracing_controller_.get(); + } enum UseDefaultCategoryMode { kUseDefaultCategories,