Repository navigation
[Humble] Remove a callback group's guard condition when the group is removed - #3411
Open
Rahul-cs50 wants to merge 1 commit into
Open
Rahul-cs50 wants to merge 1 commit into
Rahul-cs50 wants to merge 1 commit into
Conversation
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 fromweak_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 inwait_for_work()can't catch this, because the group was already erased fromweak_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.
StaticExecutorEntitiesCollectoris 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 callingremove_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 callsspin_some().remove_callback_group_removes_guard_condition: adds two groups of one node withadd_callback_group()and checks the count drops by one for eachremove_callback_group().On the unpatched
humblebranch both fail: one guard condition is left over in each, andspin_some()after destroying the node throws.Tested in a
ros:humble-ros-basecontainer: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_uncrustifyandament_cpplinton the two changed files: no problems.I did not run the
component_container_mtload/unload steps from the issue; the new tests cover the same path throughremove_node().