Skip to content

[Humble] Remove a callback group's guard condition when the group is removed - #3411

Open
Rahul-cs50 wants to merge 1 commit into
ros2:humblefrom
Rahul-cs50:fix/3410-remove-group-guard-condition
Open

Rahul-cs50 wants to merge 1 commit into
ros2:humblefrom
Rahul-cs50:fix/3410-remove-group-guard-condition

Conversation

@Rahul-cs50

Copy link
Copy Markdown

Description

On Humble, removing a node that has more than one callback group in an executor leaves a dangling guard condition behind (#3410).

Executor::remove_callback_group_from_map() removed the group's guard condition from the memory strategy, and from weak_groups_to_guard_conditions_, only inside the "this node has no other group left in the executor" branch. For a node with several groups, that is true only for the last group that gets removed. The guard conditions of the other groups stayed in the memory strategy as raw pointers. The guard conditions are owned by the callback groups, so the pointers dangle once the node is destroyed, and the next wait uses them. The expired-group cleanup in wait_for_work() can't catch this, because the group was already erased from weak_groups_to_nodes_.

This PR removes the guard condition whenever a group is removed. The interrupt of the wait is unchanged: it still happens only when the node's last group is removed.

StaticExecutorEntitiesCollector is not affected. It waits on one guard condition per node, not per group.

This is a Humble-only change. The code was replaced by the executor rewrite in Jazzy, so there is nothing to forward-port.

Fixes #3410

Is this user-facing behavior change?

No API or ABI change (the change is inside one function in executor.cpp). Unloading a component, or calling remove_node(), for a node with an extra automatically added callback group no longer crashes the executor afterwards.

Did you use Generative AI?

Yes. The analysis, the change and the tests were produced with Claude (Claude Code), which also ran the tests below. I reviewed the diff before opening this pull request.

Additional Information

Two new tests in test/rclcpp/test_executor.cpp:

  • remove_node_removes_guard_conditions_of_all_callback_groups: adds a node with the default group and one more automatically added group, removes it, and checks the memory strategy has as many guard conditions as before. It then destroys the node and calls spin_some().
  • remove_callback_group_removes_guard_condition: adds two groups of one node with add_callback_group() and checks the count drops by one for each remove_callback_group().

On the unpatched humble branch both fail: one guard condition is left over in each, and spin_some() after destroying the node throws.

Tested in a ros:humble-ros-base container:

  • test_executor: 37 passed (35 existing + the 2 new ones).
  • test_executors (rmw_fastrtps_cpp): 47 passed.
  • test_add_callback_groups_to_executor__rmw_fastrtps_cpp: 24 passed.
  • test_multi_threaded_executor, test_static_single_threaded_executor, test_static_executor_entities_collector, test_memory_strategy, test_allocator_memory_strategy: all passed.
  • ament_uncrustify and ament_cpplint on the two changed files: no problems.

I did not run the component_container_mt load/unload steps from the issue; the new tests cover the same path through remove_node().

Executor::remove_callback_group_from_map() only removed the group's
guard condition from the memory strategy when the node had no other
callback group left in the executor. For a node with more than one
group, the guard conditions of all but the last removed group stayed in
the memory strategy as raw pointers. They dangle once the node and its
groups are destroyed, and the next wait uses them.

Remove the guard condition whenever the group is removed. The interrupt
of the wait still only happens when the node's last group is removed.

Signed-off-by: Rahul-cs50 <rahulseervi.dev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant