mathias-luedtke
commented
6 years ago This behavior is intended. It implements the very common RAII pattern.
Other examples are ros::NodeHandle, ros::Subscriber or std::shared_ptr.
But it is a weak spot and makes the usage of socketcan_interface more complex (because you must keep an instance of the shared pointer for the whole lifetime).
Actually, this makes it very easy to manage the lifetime. Otherwise it would be very hard to limit the lifetime of the listerner to the lifetime of object it refers to in the callback.
until the listener is removed via remove(Listener*) (or a similar method with better suited signature).
Such a remove function would be very error prone (exception safety, memory leaks, access violations).
maracuya-robotics
The dispatcher contains a method
createListener(DispatcherBaseSharedPtr, const Callable&)which is used to create and add listeners. A new listener is instantiated, wrapped in a shared pointer and later returned to the caller. But the internal list of the dispatcher contains only a pointer to the listener, bypassing the shared pointer. Thus if you do not store the returned shared pointer, the listener will be deleted and the dispatcher will try to call freed memory.https://github.com/ros-industrial/ros_canopen/blob/f37aa7b75503c6a880dcd17bf98844958984d04d/socketcan_interface/include/socketcan_interface/dispatcher.h#L53-L57
I'm not sure if the above description is correct or if I have missed something. I understand that the pointer is stored for faster dispatching. But it is a weak spot and makes the usage of socketcan_interface more complex (because you must keep an instance of the shared pointer for the whole lifetime).
IMHO the dispatcher should keep a copy of the shared pointer (in another list to retain the performance of direct indirection) until the listener is removed via
remove(Listener*)(or a similar method with better suited signature).