Skip to content

Add graph method to wait for publishers#1621

Draft
jacobperron wants to merge 2 commits intorollingfrom
jacob/wait_for
Draft

Add graph method to wait for publishers#1621
jacobperron wants to merge 2 commits intorollingfrom
jacob/wait_for

Conversation

@jacobperron
Copy link
Copy Markdown
Member

Wrapping the rcl API introduced in ros2/rcl#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:

// Add the graph guard condition for the node to the wait set.
auto graph_gc = node_ptr->get_graph_guard_condition();
if (!graph_gc) {
throw_from_rcl_error(RCL_RET_ERROR, "failed to get graph guard condition");
}
ret = rcl_wait_set_add_guard_condition(&wait_set_, graph_gc, &graph_gc_indexes[i]);

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.

Wrapping the rcl API.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Copy link
Copy Markdown
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall it looks good to me.

Comment on lines +309 to +310
* E.g. Do not call this after calling `rclcpp::GraphListener::get_graph_event()`.
* If this happens an exception is thrown.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

me neither.

if possible, could anyone elaborate what kind of exception we are going to get? i cannot see it precisely...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, this PR is not complete. I imagine that we should guard against concurrent usage in the implementation and throw an exception (see the TODO in node_graph.cpp).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants