From 60efdfbb581a3bf0ee56a1834f669a3b10dc4339 Mon Sep 17 00:00:00 2001 From: "Tomoya.Fujita" Date: Wed, 22 Apr 2020 17:27:35 +0900 Subject: [PATCH 1/3] subscriber_statistics_collectors_ should be protected with mutex. Signed-off-by: Tomoya.Fujita --- .../topic_statistics/subscription_topic_statistics.hpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/rclcpp/include/rclcpp/topic_statistics/subscription_topic_statistics.hpp b/rclcpp/include/rclcpp/topic_statistics/subscription_topic_statistics.hpp index 711b04998c..41867f918b 100644 --- a/rclcpp/include/rclcpp/topic_statistics/subscription_topic_statistics.hpp +++ b/rclcpp/include/rclcpp/topic_statistics/subscription_topic_statistics.hpp @@ -101,6 +101,7 @@ class SubscriptionTopicStatistics const CallbackMessageT & received_message, const rclcpp::Time now_nanoseconds) const { + std::lock_guard lock(mutex_); for (const auto & collector : subscriber_statistics_collectors_) { collector->OnMessageReceived(received_message, now_nanoseconds.nanoseconds()); } @@ -120,6 +121,7 @@ class SubscriptionTopicStatistics { rclcpp::Time window_end{get_current_nanoseconds_since_epoch()}; + std::lock_guard lock(mutex_); for (auto & collector : subscriber_statistics_collectors_) { const auto collected_stats = collector->GetStatisticsResults(); @@ -143,6 +145,7 @@ class SubscriptionTopicStatistics std::vector get_current_collector_data() const { std::vector data; + std::lock_guard lock(mutex_); for (const auto & collector : subscriber_statistics_collectors_) { data.push_back(collector->GetStatisticsResults()); } @@ -155,6 +158,7 @@ class SubscriptionTopicStatistics { auto received_message_period = std::make_unique(); received_message_period->Start(); + std::lock_guard lock(mutex_); subscriber_statistics_collectors_.emplace_back(std::move(received_message_period)); window_start_ = rclcpp::Time(get_current_nanoseconds_since_epoch()); @@ -163,6 +167,7 @@ class SubscriptionTopicStatistics /// Stop all collectors, clear measurements, stop publishing timer, and reset publisher. void tear_down() { + std::lock_guard lock(mutex_); for (auto & collector : subscriber_statistics_collectors_) { collector->Stop(); } @@ -187,6 +192,8 @@ class SubscriptionTopicStatistics return std::chrono::duration_cast(now.time_since_epoch()).count(); } + /// Mutex to protect the subsequence vectors + mutable std::mutex mutex_; /// Collection of statistics collectors std::vector> subscriber_statistics_collectors_{}; /// Node name used to generate topic statistics messages to be published From 7150067854f681a3aa4d6e69c9098bd6aaa6574d Mon Sep 17 00:00:00 2001 From: "Tomoya.Fujita" Date: Thu, 23 Apr 2020 11:00:50 +0900 Subject: [PATCH 2/3] reduce the scope of the lock for statistics collectors list. Signed-off-by: Tomoya.Fujita --- .../subscription_topic_statistics.hpp | 62 +++++++++++++------ 1 file changed, 43 insertions(+), 19 deletions(-) diff --git a/rclcpp/include/rclcpp/topic_statistics/subscription_topic_statistics.hpp b/rclcpp/include/rclcpp/topic_statistics/subscription_topic_statistics.hpp index 41867f918b..c6d0697b3b 100644 --- a/rclcpp/include/rclcpp/topic_statistics/subscription_topic_statistics.hpp +++ b/rclcpp/include/rclcpp/topic_statistics/subscription_topic_statistics.hpp @@ -94,6 +94,8 @@ class SubscriptionTopicStatistics /// Handle a message received by the subscription to collect statistics. /** + * - This method acquires a lock to prevent race conditions to collectors list. + * * \param received_message the message received by the subscription * \param now_nanoseconds current time in nanoseconds */ @@ -117,22 +119,32 @@ class SubscriptionTopicStatistics } /// Publish a populated MetricsStatisticsMessage. + /** + * - This method acquires a lock to prevent race conditions to collectors list. + */ virtual void publish_message() { + std::vector msgs; rclcpp::Time window_end{get_current_nanoseconds_since_epoch()}; - std::lock_guard lock(mutex_); - for (auto & collector : subscriber_statistics_collectors_) { - const auto collected_stats = collector->GetStatisticsResults(); - - auto message = libstatistics_collector::collector::GenerateStatisticMessage( - node_name_, - collector->GetMetricName(), - collector->GetMetricUnit(), - window_start_, - window_end, - collected_stats); - publisher_->publish(message); + { + std::lock_guard lock(mutex_); + for (auto & collector : subscriber_statistics_collectors_) { + const auto collected_stats = collector->GetStatisticsResults(); + + auto message = libstatistics_collector::collector::GenerateStatisticMessage( + node_name_, + collector->GetMetricName(), + collector->GetMetricUnit(), + window_start_, + window_end, + collected_stats); + msgs.push_back(message); + } + } + + for (auto & msg : msgs) { + publisher_->publish(msg); } window_start_ = window_end; } @@ -140,6 +152,8 @@ class SubscriptionTopicStatistics protected: /// Return a vector of all the currently collected data. /** + * - This method acquires a lock to prevent race conditions to collectors list. + * * \return a vector of all the collected data */ std::vector get_current_collector_data() const @@ -154,25 +168,35 @@ class SubscriptionTopicStatistics private: /// Construct and start all collectors and set window_start_. + /** + * - This method acquires a lock to prevent race conditions to collectors list. + */ void bring_up() { auto received_message_period = std::make_unique(); received_message_period->Start(); - std::lock_guard lock(mutex_); - subscriber_statistics_collectors_.emplace_back(std::move(received_message_period)); + { + std::lock_guard lock(mutex_); + subscriber_statistics_collectors_.emplace_back(std::move(received_message_period)); + } window_start_ = rclcpp::Time(get_current_nanoseconds_since_epoch()); } /// Stop all collectors, clear measurements, stop publishing timer, and reset publisher. + /** + * - This method acquires a lock to prevent race conditions to collectors list. + */ void tear_down() { - std::lock_guard lock(mutex_); - for (auto & collector : subscriber_statistics_collectors_) { - collector->Stop(); - } + { + std::lock_guard lock(mutex_); + for (auto & collector : subscriber_statistics_collectors_) { + collector->Stop(); + } - subscriber_statistics_collectors_.clear(); + subscriber_statistics_collectors_.clear(); + } if (publisher_timer_) { publisher_timer_->cancel(); From 4498d24d835679d0dba1d284cf898df664a39b4c Mon Sep 17 00:00:00 2001 From: tomoya Date: Thu, 23 Apr 2020 12:55:18 +0900 Subject: [PATCH 3/3] Apply suggestions from code review Co-Authored-By: William Woodall Signed-off-by: Tomoya.Fujita --- .../topic_statistics/subscription_topic_statistics.hpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/rclcpp/include/rclcpp/topic_statistics/subscription_topic_statistics.hpp b/rclcpp/include/rclcpp/topic_statistics/subscription_topic_statistics.hpp index c6d0697b3b..058ed45818 100644 --- a/rclcpp/include/rclcpp/topic_statistics/subscription_topic_statistics.hpp +++ b/rclcpp/include/rclcpp/topic_statistics/subscription_topic_statistics.hpp @@ -94,7 +94,7 @@ class SubscriptionTopicStatistics /// Handle a message received by the subscription to collect statistics. /** - * - This method acquires a lock to prevent race conditions to collectors list. + * This method acquires a lock to prevent race conditions to collectors list. * * \param received_message the message received by the subscription * \param now_nanoseconds current time in nanoseconds @@ -120,7 +120,7 @@ class SubscriptionTopicStatistics /// Publish a populated MetricsStatisticsMessage. /** - * - This method acquires a lock to prevent race conditions to collectors list. + * This method acquires a lock to prevent race conditions to collectors list. */ virtual void publish_message() { @@ -152,7 +152,7 @@ class SubscriptionTopicStatistics protected: /// Return a vector of all the currently collected data. /** - * - This method acquires a lock to prevent race conditions to collectors list. + * This method acquires a lock to prevent race conditions to collectors list. * * \return a vector of all the collected data */ @@ -169,7 +169,7 @@ class SubscriptionTopicStatistics private: /// Construct and start all collectors and set window_start_. /** - * - This method acquires a lock to prevent race conditions to collectors list. + * This method acquires a lock to prevent race conditions to collectors list. */ void bring_up() { @@ -185,7 +185,7 @@ class SubscriptionTopicStatistics /// Stop all collectors, clear measurements, stop publishing timer, and reset publisher. /** - * - This method acquires a lock to prevent race conditions to collectors list. + * This method acquires a lock to prevent race conditions to collectors list. */ void tear_down() {