Check for association with an executor before removing nodes in ComponentManager

[skyegalaxy]

Description

This PR changes the destructor of component_manager to check each node it's removing for if it is already associated with an executor.

A regression test has also been added to test_component_manager_api in rclcpp_components, which asserts that an exception isn't thrown during the double-removal case as outlined in #3186

Fixes #3186

Is this user-facing behavior change?

• Users will no longer get an exception on shutdown when using component managers with the callback group events executor

Did you use Generative AI?

Claude Opus 4.8

Additional Information

[mergify]

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request

[ahcorde]

Pulls: #3190
Gist: https://gist.githubusercontent.com/ahcorde/0921923647104ed12128ae08da3c7b80/raw/62b669e2d3ce5aba62ac68d53a57e0c095eb3cfa/ros2.repos
BUILD args: --packages-above-and-dependencies rclcpp rclcpp_components
TEST args: --packages-above rclcpp rclcpp_components
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/19824

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

[fujitatomoya]

i have 2 comments.

• rclcpp/rclcpp/include/rclcpp/executor.hpp

  Line 224 in d031217

  * \throw std::runtime_error if the node is not associated with an executor.

  should be also updated align with implementation.
• normal Ctrl+C shutdown will now WARN-spam once per node, every time. the double-remove sequence is still deterministic though. either the coordination should also be fixed (e.g., ~ComponentManager skips nodes whose association flag is already clear, or EventsCBGExecutor's shutdown notifies/clears differently)? warning on every clean shutdown trains users to ignore warnings?

[skyegalaxy]

I appreciate your review and feedback @fujitatomoya ! I updated the docstrings here and also added a check in the component manager destructor for if the node's already associated with an executor before removing. That gets rid of the warning for the test case I added on my end.

[skyegalaxy]

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

[jmachowinski]

Hm, so now we are anyway at the point, were we fixed the source of the exception. Should we leave it in then ?

[skyegalaxy]

yeah you're right @jmachowinski , I'll keep the check in component container and undo my other changes

[skyegalaxy]

I removed the commits which got rid of the exception, and updated the PR title and description