rclcpp_lifecycle does not handle service de-registration the same as rclcpp

[JWhitleyWork]

Generated by Generative AI

No response

Operating System:

Linux pop-os 6.16.3-76061603-generic #202508231538175856113522.04~171c8de SMP PREEMPT_DYNAMIC Mon S x86_64 x86_64 x86_64 GNU/Linux

ROS version or commit hash:

humble

RMW implementation (if applicable):

rmw_cyclonedds_cpp

RMW Configuration (if applicable):

No response

Client library (if applicable):

rclcppp and rclcpp_lifecycle

'ros2 doctor --report' output

ros2 doctor --report

<COPY OUTPUT HERE>

Steps to reproduce issue

Test for the existence of a service (using a client) which was created by a node which inherits from rclcpp_lifecycle::LifeCycleNodeInterface after the last reference to an owned rclcpp::Client::SharedPtr has been removed and/or the node has been transitioned to UNCONFIGURED. Example below of a test which reproduces the issue (and also tests similar/expected behavior from an rclcpp::Node):

#include <gtest/gtest.h>
#include <rclcpp/rclcpp.hpp>
#include <rclcpp_lifecycle/lifecycle_node.hpp>
#include <std_srvs/srv/trigger.hpp>

#include <chrono>
#include <memory>
#include <thread>

using namespace std::chrono_literals;

class TestLifecycleNode : public rclcpp_lifecycle::LifecycleNode
{
public:
  TestLifecycleNode() : rclcpp_lifecycle::LifecycleNode("test_lifecycle") {}

  CallbackReturn on_configure(const rclcpp_lifecycle::State& /*previous_state*/) override
  {
    service_ = create_service<std_srvs::srv::Trigger>("test_lifecycle_service", std::bind(&TestLifecycleNode::service_callback, this, std::placeholders::_1, std::placeholders::_2));
    return CallbackReturn::SUCCESS;
  }

  CallbackReturn on_activate(const rclcpp_lifecycle::State& /*previous_state*/) override
  {
    return CallbackReturn::SUCCESS;
  }

  CallbackReturn on_deactivate(const rclcpp_lifecycle::State& /*previous_state*/) override
  {
    return CallbackReturn::SUCCESS;
  }

  CallbackReturn on_cleanup(const rclcpp_lifecycle::State& /*previous_state*/) override
  {
    service_.reset();
    return CallbackReturn::SUCCESS;
  }
private:
  void service_callback(const std::shared_ptr<std_srvs::srv::Trigger::Request> /*request*/, std::shared_ptr<std_srvs::srv::Trigger::Response> response)
  {
    response->success = true;
  }

  rclcpp::Service<std_srvs::srv::Trigger>::SharedPtr service_;
};

class TestRegularNode : public rclcpp::Node
{
public:
  TestRegularNode() : rclcpp::Node("test_regular")
  {
    service_ = create_service<std_srvs::srv::Trigger>("test_regular_service", std::bind(&TestRegularNode::service_callback, this, std::placeholders::_1, std::placeholders::_2));
  }

  void cleanup()
  {
    service_.reset();
  }

private:
  void service_callback(const std::shared_ptr<std_srvs::srv::Trigger::Request> /*request*/, std::shared_ptr<std_srvs::srv::Trigger::Response> response)
  {
    response->success = true;
  }

  rclcpp::Service<std_srvs::srv::Trigger>::SharedPtr service_;
};

class TestLifecycle : public ::testing::Test
{
public:
  static void SetUpTestSuite()
  {
    rclcpp::init(0, nullptr);
  }

  static void TearDownTestSuite()
  {
    rclcpp::shutdown();
  }

  void SetUp()
  {
    node_ = std::make_shared<TestLifecycleNode>();
  }

  void TearDown()
  {
    node_.reset();
  }

  std::shared_ptr<TestLifecycleNode> node_;
};

class TestRegular : public ::testing::Test
{
public:
  static void SetUpTestSuite()
  {
    rclcpp::init(0, nullptr);
  }

  static void TearDownTestSuite()
  {
    rclcpp::shutdown();
  }

  void SetUp()
  {
    node_ = std::make_shared<TestRegularNode>();
  }

  void TearDown()
  {
    node_.reset();
  }

  std::shared_ptr<TestRegularNode> node_;
};

TEST_F(TestRegular, test_regular)
{
  auto test_node = rclcpp::Node::make_shared("test_node");

  auto exec = std::make_shared<rclcpp::executors::SingleThreadedExecutor>();
  exec->add_node(node_->get_node_base_interface());
  exec->add_node(test_node->get_node_base_interface());

  std::thread ros_thread([exec]() { exec->spin(); });
  while (!exec->is_spinning()) {}

  node_->cleanup();

  std::this_thread::sleep_for(100ms);

  auto test_client = test_node->create_client<std_srvs::srv::Trigger>("test_regular_service");
  ASSERT_FALSE(test_client->service_is_ready()) << "Test client service should not be active after cleanup";

  exec->cancel();
  if (ros_thread.joinable()) {
    ros_thread.join();
  }
}

TEST_F(TestLifecycle, test_lifecycle)
{
  auto test_node = rclcpp::Node::make_shared("test_node");

  auto exec = std::make_shared<rclcpp::executors::SingleThreadedExecutor>();

  exec->add_node(node_->get_node_base_interface());
  exec->add_node(test_node->get_node_base_interface());

  // Use shared_ptr capture to ensure executor lives as long as the thread
  std::thread ros_thread([exec]() { exec->spin(); });
  while (!exec->is_spinning()) {}

  node_->configure();
  node_->activate();
  node_->deactivate();
  node_->cleanup();

  std::this_thread::sleep_for(100ms);

  auto test_client = test_node->create_client<std_srvs::srv::Trigger>("test_lifecycle_service");
  ASSERT_FALSE(test_client->service_is_ready()) << "Test client service should not be active after cleanup";

  exec->cancel();
  if (ros_thread.joinable()) {
    ros_thread.join();
  }
}

Expected behavior

rclcpp::Service::SharedPtr instances which have their last std::shared_ptr reference removed should destruct, eventually calling rcl_service_fini/rmw_destroy_service, removing the service from DDS registration, making rclcpp::Client::service_is_ready() and rclcpp::Client::wait_for_service() return False immediately (or nearly immediately). I would expect this to happen no matter the LifeCycle state of the parent node but theoretically, based on the design doc, I would expect the service to be unavailable as soon as the node transitions out of CONFIGURED as well (when in CONFIGURED, I would expect the service to be discoverable but not respond to any requests).

Actual behavior

rclcpp::Client::service_is_ready() and rclcpp::Client::wait_for_service() sometimes return True (~10% of the time) after the rclcpp::Service::SharedPtr has been destructed and the owning class instance inheriting from rclcpp_lifecycle::LifeCycleNodeInterface has been transitioned to UNCONFIGURED. In my testing, this does not ever happen with the same conditions in an rclcpp::Node.

Additional information

No response

[wjwwood]

This seems like a race condition to me. What happens if you remove the std::this_thread::sleep_for(100ms); in both cases? Is it still only in the lifecycle case or now in both? What happens to the rate of failure for the lifecycle case? I would guess both have the problem and the rate increases from ~10%.

---

Something else to consider, the ownership of the service you created is shared, meaning some other part of the system may have an instance of it for an indeterminate amount of time. The "contract" with the user is that when it is no longer needed the service shared pointer will be allowed to go out of scope, but there's no guarantees on when that will happen. For example, if you have an executor running with the service as a part of it, it may keep the shared pointer in scope while servicing on-going requests which are asynchronous to your .reset(). I'm not aware of anything in the LifecycleNode that should be extending the lifespan of that pointer, but there may be something I'm not aware of. You could check the state of the shared pointer by creating a weak_ptr and checking its use count (https://en.cppreference.com/w/cpp/memory/weak_ptr/use_count.html or https://en.cppreference.com/w/cpp/memory/weak_ptr/expired.html).

There is also a shared pointer for the rcl handle, which may have a different lifespan from the c++ object too, though we try to avoid those getting out of sync, see:

rclcpp/rclcpp/include/rclcpp/service.hpp

Line 275 in 715a983

std::shared_ptr<rcl_service_t> service_handle_;

It should be released at the same time as the c++ object as we use the default destructor:

rclcpp/rclcpp/include/rclcpp/service.hpp

Lines 59 to 60 in 715a983

	RCLCPP_PUBLIC
	virtual ~ServiceBase() = default;

It's possible that this rcl handle could have its lifespan extended as well, but you could also create weak_ptr to it and check its status during debugging:

rclcpp/rclcpp/include/rclcpp/service.hpp

Lines 68 to 75 in 715a983

	/// Return the rcl_service_t service handle in a std::shared_ptr.
	/**
	* This handle remains valid after the Service is destroyed.
	* The actual rcl service is not finalized until it is out of scope everywhere.
	*/
	RCLCPP_PUBLIC
	std::shared_ptr<rcl_service_t>
	get_service_handle();

---

Separately, once the pointer is released there may also be a delay in the time between the rcl object being destroyed and the middleware communicating that to other entities. Now locally (which is the case here, since the client and service are in the same process/rclcpp::Context) it is likely very fast and needs no "communication", but it may be the case that this information still has to propagate through the discovery traffic as if it were in a different process of machine on the network. So there's still no guarantee on how long it will take the client to notice the server has disappeared. In practice it probably should be instant, but you cannot rely on that as it is not a guaranteed.

So while I understand why you're checking this the way you are (it is reasonable to assume that this test case should work as written), it's not something we guarantee at all, much less within 100ms (which is arbitrary and may not be long enough on non-realtime OSes).

Instead I would suggest you change the logic to poll until the predicate is true (the service is not ready) and allow that polling to go on for a comparatively large amount of time (e.g. 60s or 5m or something). While this may seem strange, I would ask you what it is you're trying to test here (I assume it's a simplification of a more complex test), are you trying to assert that the service disappears in a given amount of time? or that it disappears eventually (within some reasonable amount of time for a human)? or some thing else?