fix TestTimeSource.ROS_time_valid_attach_detach.

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>

.github/workflows/mirror-rolling-to-master.yaml

@@ -1,13 +0,0 @@
-name: Mirror rolling to master
-
-on:
-  push:
-    branches: [ rolling ]
-
-jobs:
-  mirror-to-master:
-    runs-on: ubuntu-latest
-    steps:
-    - uses: zofrex/mirror-branch@v1
-      with:
-        target-branch: master

CODEOWNERS

@@ -1,2 +0,0 @@
-# This file was generated by https://github.com/audrow/update-ros2-repos
-* @ivanpauno @hidmic @wjwwood

rclcpp/src/rclcpp/executor.cpp

@@ -729,13 +729,33 @@ Executor::wait_for_work(std::chrono::nanoseconds timeout)
   // Clear any previous wait result
   this->wait_result_.reset();
 
+  // we need to make sure that callback groups don't get out of scope
+  // during the wait. As in jazzy, they are not covered by the DynamicStorage,
+  // we explicitly hold them here as a bugfix
+  std::vector<rclcpp::CallbackGroup::SharedPtr> cbgs;
+
   {
     std::lock_guard<std::mutex> guard(mutex_);
+
     if (this->entities_need_rebuild_.exchange(false) || current_collection_.empty()) {
       this->collect_entities();
     }
+
+    auto callback_groups = this->collector_.get_all_callback_groups();
+    cbgs.resize(callback_groups.size());
+    for(const auto & w_ptr : callback_groups) {
+      auto shr_ptr = w_ptr.lock();
+      if(shr_ptr) {
+        cbgs.push_back(std::move(shr_ptr));
+      }
+    }
   }
+
   this->wait_result_.emplace(wait_set_.wait(timeout));
+
+  // drop references to the callback groups, before trying to execute anything
+  cbgs.clear();
+
   if (!this->wait_result_ || this->wait_result_->kind() == WaitResultKind::Empty) {
     RCUTILS_LOG_WARN_NAMED(
       "rclcpp",

rclcpp/src/rclcpp/executors/static_single_threaded_executor.cpp

@@ -110,10 +110,29 @@ StaticSingleThreadedExecutor::spin_once_impl(std::chrono::nanoseconds timeout)
 std::optional<rclcpp::WaitResult<rclcpp::WaitSet>>
 StaticSingleThreadedExecutor::collect_and_wait(std::chrono::nanoseconds timeout)
 {
+  // we need to make sure that callback groups don't get out of scope
+  // during the wait. As in jazzy, they are not covered by the DynamicStorage,
+  // we explicitly hold them here as a bugfix
+  std::vector<rclcpp::CallbackGroup::SharedPtr> cbgs;
+
   if (this->entities_need_rebuild_.exchange(false) || current_collection_.empty()) {
     this->collect_entities();
   }
+
+  auto callback_groups = this->collector_.get_all_callback_groups();
+  cbgs.resize(callback_groups.size());
+  for(const auto & w_ptr : callback_groups) {
+    auto shr_ptr = w_ptr.lock();
+    if(shr_ptr) {
+      cbgs.push_back(std::move(shr_ptr));
+    }
+  }
+
   auto wait_result = wait_set_.wait(std::chrono::nanoseconds(timeout));
+
+  // drop references to the callback groups, before trying to execute anything
+  cbgs.clear();
+
   if (wait_result.kind() == WaitResultKind::Empty) {
     RCUTILS_LOG_WARN_NAMED(
       "rclcpp",

rclcpp/test/rclcpp/executors/test_executors.cpp

@@ -388,7 +388,7 @@ to_nanoseconds_helper(DurationT duration)
 //   - works nominally (it can execute entities)
 //   - it can execute multiple items at once
 //   - it does not wait for work to be available before returning
-TYPED_TEST(TestExecutors, spin_some)
+TYPED_TEST(TestExecutors, spinSome)
 {
   using ExecutorType = TypeParam;
 
@@ -484,7 +484,7 @@ TYPED_TEST(TestExecutors, spin_some)
 //   do not properly implement max_duration (it seems), so disable this test
 //   for them in the meantime.
 //   see: https://github.com/ros2/rclcpp/issues/2462
-TYPED_TEST(TestExecutorsStable, spin_some_max_duration)
+TYPED_TEST(TestExecutorsStable, spinSomeMaxDuration)
 {
   using ExecutorType = TypeParam;
 
@@ -714,6 +714,13 @@ TYPED_TEST(TestExecutors, notifyTwiceWhileSpinning)
       sub1_msg_count++;
     });
 
+  // Wait for the subscription to be matched
+  size_t tries = 10000;
+  while (this->publisher->get_subscription_count() < 2 && tries-- > 0) {
+    std::this_thread::sleep_for(std::chrono::milliseconds(1));
+  }
+  ASSERT_EQ(this->publisher->get_subscription_count(), 2);
+
   // Publish a message and verify it's received
   this->publisher->publish(test_msgs::msg::Empty());
   auto start = std::chrono::steady_clock::now();
@@ -731,6 +738,13 @@ TYPED_TEST(TestExecutors, notifyTwiceWhileSpinning)
       sub2_msg_count++;
     });
 
+  // Wait for the subscription to be matched
+  tries = 10000;
+  while (this->publisher->get_subscription_count() < 3 && tries-- > 0) {
+    std::this_thread::sleep_for(std::chrono::milliseconds(1));
+  }
+  ASSERT_EQ(this->publisher->get_subscription_count(), 3);
+
   // Publish a message and verify it's received by both subscriptions
   this->publisher->publish(test_msgs::msg::Empty());
   start = std::chrono::steady_clock::now();
@@ -820,7 +834,7 @@ TEST(TestExecutors, testSpinWithNonDefaultContext)
   rclcpp::shutdown(non_default_context);
 }
 
-TYPED_TEST(TestExecutors, release_ownership_entity_after_spinning_cancel)
+TYPED_TEST(TestExecutors, releaseOwnershipEntityAfterSpinningCancel)
 {
   using ExecutorType = TypeParam;
   ExecutorType executor;
@@ -843,3 +857,36 @@ TYPED_TEST(TestExecutors, release_ownership_entity_after_spinning_cancel)
 
   EXPECT_EQ(server.use_count(), 1);
 }
+
+TYPED_TEST(TestExecutors, testRaceDropCallbackGroupFromSecondThread)
+{
+  using ExecutorType = TypeParam;
+
+  // Create an executor
+  ExecutorType executor;
+  executor.add_node(this->node);
+
+  // Start spinning
+  auto executor_thread = std::thread(
+    [&executor]() {
+      executor.spin();
+    });
+
+  // As the problem is a race, we do this multiple times,
+  // to raise our chances of hitting the problem
+  for (size_t i = 0; i < 10; i++) {
+    auto cg = this->node->create_callback_group(
+      rclcpp::CallbackGroupType::MutuallyExclusive);
+    auto timer = this->node->create_timer(1s, [] {}, cg);
+    // sleep a bit, so that the spin thread can pick up the callback group
+    // and add it to the executor
+    std::this_thread::sleep_for(5ms);
+
+    // At this point the callbackgroup should be used within the waitset of the executor
+    // as we leave the scope, the reference to cg will be dropped.
+    // If the executor has a race, we will experience a segfault at this point.
+  }
+
+  executor.cancel();
+  executor_thread.join();
+}

rclcpp/test/rclcpp/test_client_common.cpp

@@ -431,7 +431,7 @@ TYPED_TEST(TestAllClientTypesWithServer, client_qos)
 
   rclcpp::ServicesQoS qos_profile;
   qos_profile.liveliness(rclcpp::LivelinessPolicy::Automatic);
-  rclcpp::Duration duration(std::chrono::nanoseconds(1));
+  rclcpp::Duration duration(std::chrono::milliseconds(1));
   qos_profile.deadline(duration);
   qos_profile.lifespan(duration);
   qos_profile.liveliness_lease_duration(duration);

rclcpp/test/rclcpp/test_generic_service.cpp

@@ -320,7 +320,7 @@ TEST_F(TestGenericService, rcl_service_request_subscription_get_actual_qos_error
 TEST_F(TestGenericService, generic_service_qos) {
   rclcpp::ServicesQoS qos_profile;
   qos_profile.liveliness(rclcpp::LivelinessPolicy::Automatic);
-  rclcpp::Duration duration(std::chrono::nanoseconds(1));
+  rclcpp::Duration duration(std::chrono::milliseconds(1));
   qos_profile.deadline(duration);
   qos_profile.lifespan(duration);
   qos_profile.liveliness_lease_duration(duration);

rclcpp/test/rclcpp/test_service.cpp

@@ -335,7 +335,7 @@ TEST_F(TestService, rcl_service_request_subscription_get_actual_qos_error) {
 TEST_F(TestService, server_qos) {
   rclcpp::ServicesQoS qos_profile;
   qos_profile.liveliness(rclcpp::LivelinessPolicy::Automatic);
-  rclcpp::Duration duration(std::chrono::nanoseconds(1));
+  rclcpp::Duration duration(std::chrono::milliseconds(1));
   qos_profile.deadline(duration);
   qos_profile.lifespan(duration);
   qos_profile.liveliness_lease_duration(duration);

rclcpp/test/rclcpp/test_service_introspection.cpp

@@ -92,9 +92,16 @@ TEST_F(TestServiceIntrospection, service_introspection_nominal)
   request->set__int64_value(42);
 
   client->configure_introspection(
-    node->get_clock(), rclcpp::SystemDefaultsQoS(), RCL_SERVICE_INTROSPECTION_CONTENTS);
+    node->get_clock(), rclcpp::ServicesQoS(), RCL_SERVICE_INTROSPECTION_CONTENTS);
   service->configure_introspection(
-    node->get_clock(), rclcpp::SystemDefaultsQoS(), RCL_SERVICE_INTROSPECTION_CONTENTS);
+    node->get_clock(), rclcpp::ServicesQoS(), RCL_SERVICE_INTROSPECTION_CONTENTS);
+
+  // Wait for the introspection to attach to our subscription
+  size_t tries = 1000;
+  while (this->sub->get_publisher_count() < 2 && tries-- > 0) {
+    std::this_thread::sleep_for(std::chrono::milliseconds(1));
+  }
+  ASSERT_EQ(sub->get_publisher_count(), 2u);
 
   auto future = client->async_send_request(request);
   ASSERT_EQ(
@@ -163,9 +170,11 @@ TEST_F(TestServiceIntrospection, service_introspection_nominal)
 TEST_F(TestServiceIntrospection, service_introspection_enable_disable_events)
 {
   client->configure_introspection(
-    node->get_clock(), rclcpp::SystemDefaultsQoS(), RCL_SERVICE_INTROSPECTION_OFF);
+    node->get_clock(), rclcpp::ServicesQoS(), RCL_SERVICE_INTROSPECTION_OFF);
   service->configure_introspection(
-    node->get_clock(), rclcpp::SystemDefaultsQoS(), RCL_SERVICE_INTROSPECTION_OFF);
+    node->get_clock(), rclcpp::ServicesQoS(), RCL_SERVICE_INTROSPECTION_OFF);
+
+  ASSERT_EQ(sub->get_publisher_count(), 0);
 
   auto request = std::make_shared<BasicTypes::Request>();
   request->set__bool_value(true);
@@ -183,9 +192,16 @@ TEST_F(TestServiceIntrospection, service_introspection_enable_disable_events)
   events.clear();
 
   client->configure_introspection(
-    node->get_clock(), rclcpp::SystemDefaultsQoS(), RCL_SERVICE_INTROSPECTION_METADATA);
+    node->get_clock(), rclcpp::ServicesQoS(), RCL_SERVICE_INTROSPECTION_METADATA);
   service->configure_introspection(
-    node->get_clock(), rclcpp::SystemDefaultsQoS(), RCL_SERVICE_INTROSPECTION_OFF);
+    node->get_clock(), rclcpp::ServicesQoS(), RCL_SERVICE_INTROSPECTION_OFF);
+
+  // Wait for the introspection to attach to our subscription
+  size_t tries = 1000;
+  while (this->sub->get_publisher_count() < 1 && tries-- > 0) {
+    std::this_thread::sleep_for(std::chrono::milliseconds(1));
+  }
+  ASSERT_EQ(sub->get_publisher_count(), 1u);
 
   future = client->async_send_request(request);
   ASSERT_EQ(
@@ -200,9 +216,16 @@ TEST_F(TestServiceIntrospection, service_introspection_enable_disable_events)
   events.clear();
 
   client->configure_introspection(
-    node->get_clock(), rclcpp::SystemDefaultsQoS(), RCL_SERVICE_INTROSPECTION_OFF);
+    node->get_clock(), rclcpp::ServicesQoS(), RCL_SERVICE_INTROSPECTION_OFF);
   service->configure_introspection(
-    node->get_clock(), rclcpp::SystemDefaultsQoS(), RCL_SERVICE_INTROSPECTION_METADATA);
+    node->get_clock(), rclcpp::ServicesQoS(), RCL_SERVICE_INTROSPECTION_METADATA);
+
+  // Wait for the introspection to attach to our subscription
+  tries = 1000;
+  while (this->sub->get_publisher_count() < 1 && tries-- > 0) {
+    std::this_thread::sleep_for(std::chrono::milliseconds(1));
+  }
+  ASSERT_EQ(sub->get_publisher_count(), 1u);
 
   future = client->async_send_request(request);
   ASSERT_EQ(
@@ -217,9 +240,16 @@ TEST_F(TestServiceIntrospection, service_introspection_enable_disable_events)
   events.clear();
 
   client->configure_introspection(
-    node->get_clock(), rclcpp::SystemDefaultsQoS(), RCL_SERVICE_INTROSPECTION_METADATA);
+    node->get_clock(), rclcpp::ServicesQoS(), RCL_SERVICE_INTROSPECTION_METADATA);
   service->configure_introspection(
-    node->get_clock(), rclcpp::SystemDefaultsQoS(), RCL_SERVICE_INTROSPECTION_METADATA);
+    node->get_clock(), rclcpp::ServicesQoS(), RCL_SERVICE_INTROSPECTION_METADATA);
+
+  // Wait for the introspection to attach to our subscription
+  tries = 1000;
+  while (this->sub->get_publisher_count() < 2 && tries-- > 0) {
+    std::this_thread::sleep_for(std::chrono::milliseconds(1));
+  }
+  ASSERT_EQ(sub->get_publisher_count(), 2u);
 
   future = client->async_send_request(request);
   ASSERT_EQ(
@@ -235,9 +265,16 @@ TEST_F(TestServiceIntrospection, service_introspection_enable_disable_events)
 TEST_F(TestServiceIntrospection, service_introspection_enable_disable_event_content)
 {
   client->configure_introspection(
-    node->get_clock(), rclcpp::SystemDefaultsQoS(), RCL_SERVICE_INTROSPECTION_METADATA);
+    node->get_clock(), rclcpp::ServicesQoS(), RCL_SERVICE_INTROSPECTION_METADATA);
   service->configure_introspection(
-    node->get_clock(), rclcpp::SystemDefaultsQoS(), RCL_SERVICE_INTROSPECTION_METADATA);
+    node->get_clock(), rclcpp::ServicesQoS(), RCL_SERVICE_INTROSPECTION_METADATA);
+
+  // Wait for the introspection to attach to our subscription
+  size_t tries = 1000;
+  while (this->sub->get_publisher_count() < 2 && tries-- > 0) {
+    std::this_thread::sleep_for(std::chrono::milliseconds(1));
+  }
+  ASSERT_EQ(sub->get_publisher_count(), 2u);
 
   auto request = std::make_shared<BasicTypes::Request>();
   request->set__bool_value(true);
@@ -259,9 +296,16 @@ TEST_F(TestServiceIntrospection, service_introspection_enable_disable_event_cont
   events.clear();
 
   client->configure_introspection(
-    node->get_clock(), rclcpp::SystemDefaultsQoS(), RCL_SERVICE_INTROSPECTION_CONTENTS);
+    node->get_clock(), rclcpp::ServicesQoS(), RCL_SERVICE_INTROSPECTION_CONTENTS);
   service->configure_introspection(
-    node->get_clock(), rclcpp::SystemDefaultsQoS(), RCL_SERVICE_INTROSPECTION_METADATA);
+    node->get_clock(), rclcpp::ServicesQoS(), RCL_SERVICE_INTROSPECTION_METADATA);
+
+  // Wait for the introspection to attach to our subscription
+  tries = 1000;
+  while (this->sub->get_publisher_count() < 2 && tries-- > 0) {
+    std::this_thread::sleep_for(std::chrono::milliseconds(1));
+  }
+  ASSERT_EQ(sub->get_publisher_count(), 2u);
 
   future = client->async_send_request(request);
   ASSERT_EQ(
@@ -292,9 +336,16 @@ TEST_F(TestServiceIntrospection, service_introspection_enable_disable_event_cont
   events.clear();
 
   client->configure_introspection(
-    node->get_clock(), rclcpp::SystemDefaultsQoS(), RCL_SERVICE_INTROSPECTION_METADATA);
+    node->get_clock(), rclcpp::ServicesQoS(), RCL_SERVICE_INTROSPECTION_METADATA);
   service->configure_introspection(
-    node->get_clock(), rclcpp::SystemDefaultsQoS(), RCL_SERVICE_INTROSPECTION_CONTENTS);
+    node->get_clock(), rclcpp::ServicesQoS(), RCL_SERVICE_INTROSPECTION_CONTENTS);
+
+  // Wait for the introspection to attach to our subscription
+  tries = 1000;
+  while (this->sub->get_publisher_count() < 2 && tries-- > 0) {
+    std::this_thread::sleep_for(std::chrono::milliseconds(1));
+  }
+  ASSERT_EQ(sub->get_publisher_count(), 2u);
 
   future = client->async_send_request(request);
   ASSERT_EQ(
@@ -325,9 +376,16 @@ TEST_F(TestServiceIntrospection, service_introspection_enable_disable_event_cont
   events.clear();
 
   client->configure_introspection(
-    node->get_clock(), rclcpp::SystemDefaultsQoS(), RCL_SERVICE_INTROSPECTION_CONTENTS);
+    node->get_clock(), rclcpp::ServicesQoS(), RCL_SERVICE_INTROSPECTION_CONTENTS);
   service->configure_introspection(
-    node->get_clock(), rclcpp::SystemDefaultsQoS(), RCL_SERVICE_INTROSPECTION_CONTENTS);
+    node->get_clock(), rclcpp::ServicesQoS(), RCL_SERVICE_INTROSPECTION_CONTENTS);
+
+  // Wait for the introspection to attach to our subscription
+  tries = 1000;
+  while (this->sub->get_publisher_count() < 2 && tries-- > 0) {
+    std::this_thread::sleep_for(std::chrono::milliseconds(1));
+  }
+  ASSERT_EQ(sub->get_publisher_count(), 2u);
 
   future = client->async_send_request(request);
   ASSERT_EQ(

rclcpp/test/rclcpp/test_time_source.cpp

@@ -212,6 +212,9 @@ TEST_F(TestTimeSource, ROS_time_valid_attach_detach) {
   ts.attachNode(node);
   EXPECT_FALSE(ros_clock->ros_time_is_active());
 
+  ts.attachClock(ros_clock);
+  EXPECT_FALSE(ros_clock->ros_time_is_active());
+
   ts.detachClock(ros_clock);
   EXPECT_FALSE(ros_clock->ros_time_is_active());
 }

rclcpp_action/package.xml

@@ -26,8 +26,6 @@
   <depend>rcl</depend>
   <depend>rcpputils</depend>
 
-  <depend>ament_cmake</depend>
-
   <test_depend>ament_cmake_gtest</test_depend>
   <test_depend>ament_lint_auto</test_depend>
   <test_depend>ament_lint_common</test_depend>