Node::remove_<subscription, publisher, service, client>

[jacquelinekay]

Node has several functions for creating entities (subscription, publisher, service, client, timer) but not for removing them. This seems like an oversight. I think we should add remove functions, and entities should also remove themselves from Node in their destructor.

[wjwwood]

The opposite of create is not remove... so at least I think the names of the functions should be destroy if you're going to have them.

Since they are scoped and the callback group objects which keep track of them store them as weak pointers:

https://github.com/ros2/rclcpp/blob/master/rclcpp/include/rclcpp/callback_group.hpp#L100

So when they are destroyed, they are effectively removed from any tracking.

[wjwwood]

Reading more of the source (and remembering more) I think the idea is that there should not be remove or destroy functions, but I think we are missing some clean up which should happen in the destructors of these things, for example they might ought to wake up any blocking wait calls so the wait set can be remade without the recently destroyed objects. Like here:

https://github.com/ros2/rclcpp/blob/master/rclcpp/src/rclcpp/subscription.cpp#L41-L60

[jacquelinekay]

Hmm, yeah, when I originally created this ticket I might have also been thinking of functions to remove a subscription/client/service from a node and manage the state in the node that tracks those entities (like this https://github.com/ros2/rclcpp/blob/master/rclcpp/include/rclcpp/node.hpp#L273-L276), without destroying the node itself.

[wjwwood]

Searching, those might be dead code now:

Searching 71 files for "number_of_subscriptions_" (case sensitive)

/Users/william/ros2_ws/src/ros2/rclcpp/rclcpp/include/rclcpp/node.hpp:
  271    CallbackGroupWeakPtrList callback_groups_;
  272  
  273:   size_t number_of_subscriptions_;
  274    size_t number_of_timers_;
  275    size_t number_of_services_;

/Users/william/ros2_ws/src/ros2/rclcpp/rclcpp/include/rclcpp/node_impl.hpp:
  243      default_callback_group_->add_subscription(sub_base_ptr);
  244    }
  245:   number_of_subscriptions_++;
  246    return sub;
  247  }

/Users/william/ros2_ws/src/ros2/rclcpp/rclcpp/src/rclcpp/node.cpp:
   36    bool use_intra_process_comms)
   37  : name_(node_name), context_(context),
   38:   number_of_subscriptions_(0), number_of_timers_(0), number_of_services_(0),
   39    use_intra_process_comms_(use_intra_process_comms)
   40  {

3 matches across 3 files

It is declared, initialized, and incremented, but otherwise never referenced or decremented anywhere in rclcpp.

[jacquelinekay]

oh, interesting. and they are private. We should definitely clean that up.

[Karsten1987]

@wjwwood is that still an ongoing discussion? It looks to me as if using shared_ptr/weak_ptr doesn't require any destroy functions.

[wjwwood]

Have the functions mentioned been removed? If not then I’d say the task in this issue is not done yet. I don’t think any more discussion needs to be done.

[clalancette]

It looks to me like everything has been done here (with destructors), and number_of_subscriptions was also removed. So closing this out.