-
Notifications
You must be signed in to change notification settings - Fork 522
Description
Bug report
In rclcpp, in rclcpp::ParameterEventHandler the functions add_parameter_event_callback and add_parameter_callback return a shared_ptr to a "handle" object. The shared_ptr to the handle is the only owning pointer to the handle, meaning if it's not captured, the handle is deallocated and the callback silently never fires (internally, the parameter_callbacks_ map holds only weak_ptrs to the callback handles which do get checked when the callback needs to fire. If the handle object no longer exists, it correctly doesn't fire the callback.)
Steps to reproduce issue
In the demos repository, in demo_nodes_cpp-> parameters->parameter_event_handler.cpp, modify this line:
https://github.com/ros2/demos/blob/rolling/demo_nodes_cpp/src/parameters/parameter_event_handler.cpp#L91
from:
auto handle1 = param_subscriber->add_parameter_callback(param_name, cb1);
to
param_subscriber->add_parameter_callback(param_name, cb1);
run the demo and set the parameter and observe the callback never fires.
Expected behavior
I would expect the callback to fire or if this design really is intentional get some sort of warning of what's happening.
It seems strange to me that the parameter_callbacks_ map and event_callbacks_ list hold only weak_ptrs. Only thing I can thing of is that its sort of an implicit way of ensuring whatever object registered the callback still exists when it's time to fire the callback. If this is the case it really should be documented better and at a minimum those functions should be marked [[no_discard]] so that if a user doesn't capture the handle they get a compiler warning.
I think it's perfectly valid to not capture the handle if I have no interest in deleting the callback later and so I think it's fine to change the weak_ptrs in parameter_callbacks_ and event_callbacks_ to shared_ptrs (actually I don't even think pointers need to be used here...just store the callback directly in the map). I'm trying to work through a scenerio in which you would run into ownership issues....if I come up with one, I'll post it.