Don't shutdown contained entities.

[clalancette]

It is tempting to think of the entities in the RMW graph as a hierarchy, where an rmw_context_t contains zero or more rmw_node_t, and rmw_node_t contains zero or more publishers, subscriptions, clients, and services.

However, the reality is that the upper layers (particularly rclcpp and rclpy) don't exactly view the entities like that. More specifically, each entity is considered standalone, that happens to have linkage to other entities in the graph. For example, a publisher is considered to be a standalone entity that happens to be linked to a particular node.

Because of this, it is not proper to shutdown all nodes within a context when the context is shutdown. The node should continue to live on until it is shutdown. And a similar argument goes for the node shutdown; it should not shutdown the publishers, subscriptions, clients, and services that are contained within it.

This manifested itself as a exception that was being thrown in some of the tests in test_communication. Because it is using a loop with rclcpp::ok(), followed by a publisher->publish(), the test would sometimes throw an exception if Ctrl-C was hit between the rclcpp::ok() and the publish() call. And that's because even though the context has been shut down, the publisher is an independent entity that should continue to exist until the next rclcpp::ok().

The fix here is simple; don't shut "contained" entities down during a context or node shutdown.

[Yadunund]

Hmm this contradicts the documentation for rcl_node_fini seen here. Could it be that the test expecation needs to be fixed instead?

[clalancette]

Could it be that the test expecation needs to be fixed instead?

I don't think so; the pattern of while (rclcpp::ok()) { pub->publish() } is pretty well established.

Hmm this contradicts the documentation for rcl_node_fini seen here.

I read the documentation for rcl_node_fini as saying "it is the users responsibility to shut down all entities before shutting down the node".

Further, I'll point out that none of the rest of the current RMWs clean up entities when calling rmw_destroy_node:

• rmw_cyclonedds_cpp - removes this node from the graph, frees memory associated with the node, then decrements the references to the context
• rmw_fastrtps_cpp - calls __rmw_destroy_node, which removes this node from the graph and frees memory associated with the node, then decrements the references to the context
• rmw_connextdds - removes this node from the graph and frees the memory associated with the node, then decrements the references to the context

So I think that we should do the same as all of the rest, and not actually destroy all of the associated entities here.

[Yadunund]

Thanks for clarifying!