Skip to content

Parameter and parameter event callbacks silently fail if handle is not captured #1998

@jasonbeach

Description

@jasonbeach

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    help wantedExtra attention is needed

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions