rclcpp icon indicating copy to clipboard operation
rclcpp copied to clipboard

Add graph method to wait for publishers

Open jacobperron opened this issue 3 years ago • 0 comments

Wrapping the rcl API introduced in https://github.com/ros2/rcl/pull/907


There are some issues with this change. Opening as a draft for visibility.

The issue boils down to using the node's graph guard condition in multiple wait sets concurrently.

The rcl_wait_for_* API uses the graph guard condition in a wait set as part of it's implementation. The rclcpp::GraphListener also uses the graph guard condition, running in a separate thread:

https://github.com/ros2/rclcpp/blob/091a8bcf86ca2697df8b94f485d62fc5687cc19c/rclcpp/src/rclcpp/graph_listener.cpp#L175-L180

This means that if a users try to use the GraphListener, which is lazily started, and then try to call one of the rcl_wait_for_* functions we get undefined behavior.

Out of curiosity, I've tried to replicate this undefined behavior scenario and I didn't notice anything unexpected:

#include <rclcpp/rclcpp.hpp>

int main(void)
{
  rclcpp::init(0, nullptr);
  auto node = std::make_shared<rclcpp::Node>("my_node");
  auto graph = node->get_node_graph_interface();

  // This starts the GraphListener
  auto event = graph->get_graph_event();

  // Concurrently access the node's graph guard condition
  bool wait_result = graph->wait_for_publishers("/foo", 2, std::chrono::seconds(10));

  return 0;
}

I think rather than wrapping the rcl functions, we could add similar implementations based on the rclcpp::GraphListener. This way we avoid undefined behavior.

We could additionally expose the rcl functions, but we should "guard" against using the same guard condition as the GraphListener (e.g. detect that they are both being used concurrently and throw an informative exception).

Alternatively, we could add support to rcl (and rmw?) for multiple graph guard conditions. Then, the rcl functions could use a different guard condition than the rclcpp::GraphListener. I think this approach is more involved and is probably not worth the added complexity.

jacobperron avatar Apr 05 '21 19:04 jacobperron