Don't let exceptions escape destructors

[davetcoleman]

Space ROS runs clang-tidy over rclcpp. ament_lint_common does not, and there is no .clang-tidy in this repo, so bugprone-exception-escape has not been pointed at this code before. It flags three destructors that can reach std::terminate.

The bug

These three destructors call something that throws:

• TimeSource::NodeState::~NodeState calls detachNode(), which calls NodeParameters::remove_on_set_parameters_callback(). That constructs a ParameterMutationRecursionGuard, which throws ParameterModifiedInCallbackException while a set-parameters callback is running.
• EventsCBGExecutor::~EventsCBGExecutor calls shutdown(), then remove_all_nodes_and_callback_groups(), then remove_node(), which throws std::runtime_error when a node is no longer associated with the executor.
• WaitResult::~WaitResult calls wait_result_release(), which throws std::runtime_error when the wait set is not holding the result (wait_set_template.hpp:750).

The three are not equally reachable, and it is worth saying which is which. The first happens today and the added test hits it. For the second I have no reproducer: it needs the node disassociated from the executor by some other path first. The third is likely unreachable without prior misuse, since wait_result_holding_ is cleared only in that function and the move constructor nulls the moved-from pointer, so guarding it is defensive. Same defect and same fix in all three, so they go together.

On the noexcept point, since it came up on ros2#2948: a destructor's implicit exception specification is noexcept(true) unless a base or member destructor is itself potentially throwing, which none of these three have. The gdb trace below is the confirmation rather than the argument, since it lands in __cxa_call_terminate.

~Context already guards this case, and ros2#2953 did the same for ~ServerGoalHandle after ros2#2948 reported it as a throwing destructor. Those two picked different severities: ~ServerGoalHandle logs at RCLCPP_DEBUG, ~Context at RCLCPP_ERROR. I followed ~Context on the grounds that failed teardown is worth seeing by default, but I will happily drop these to DEBUG if you would rather they stay quiet.

Reproducing

The test added to test_time_source.cpp destroys a TimeSource from inside an on-set-parameters callback. Before the change it takes down the whole test binary instead of failing an assertion:

[ RUN      ] TestTimeSource.destructor_does_not_terminate_when_detach_throws
-- run_test.py: return code 245

Under gdb:

#2  std::terminate ()
#3  __cxa_call_terminate ()
#4  in ?? () from librclcpp.so                   <- ~TimeSource
#6  std::default_delete<rclcpp::TimeSource>::operator() (...)
#10 operator() (...) at test_time_source.cpp:216 <- inside the parameter callback

After:

[ERROR] [my_node]: unhandled exception in ~NodeState(): cannot set or declare a parameter, or change the callback from within set callback
[       OK ] TestTimeSource.destructor_does_not_terminate_when_detach_throws (27 ms)

Full rclcpp suite with this patch applied, Ubuntu 24.04 on aarch64: 3096 tests, 0 failures.

To see the warnings, inside docker run --rm -it ros:rolling-ros-base bash:

apt-get update && apt-get install -y clang-tidy git python3-colcon-common-extensions ros-rolling-rclcpp
mkdir -p /ws/src && cd /ws/src && git clone https://github.com/ros2/rclcpp.git
cd rclcpp && git checkout 29de98c   # rolling as of writing
cd /ws && . /opt/ros/rolling/setup.sh
colcon build --packages-select rclcpp --cmake-args -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DBUILD_TESTING=OFF
run-clang-tidy -p /ws/build/rclcpp -quiet \
  -header-filter=".*/ws/src/rclcpp/rclcpp/include/rclcpp/.*" \
  -checks="-*,bugprone-exception-escape"

Three warnings on that tree, zero with this branch checked out instead. The ros-rolling-rclcpp package is only there to pull the dependency closure; rosdep install --from-paths src -y does the same.

The fix

Catch and log, the way ~Context does.

Behavior change: these paths aborted the process and now log and continue. ~Context made the same tradeoff.

The guarded call is abandoned partway, which is the part worth checking. In ~NodeState that skips resets of members that are about to be destroyed anyway. In ~EventsCBGExecutor it skips context_->remove_on_shutdown_callback(), so the registration outlives the executor; that callback holds a weak_ptr to the guard condition, so it becomes a no-op rather than a dangling call, but the entry does stay on the context until shutdown. Both are better than terminating.

No API or ABI change: destructor bodies only, no members, bases, signatures or vtables. One of the three is an inline destructor in a public header (wait_result.hpp) and adds #include "rclcpp/logging.hpp" there, which that header already pulled in transitively via client.hpp and service.hpp.

Scope

The same clang-tidy run reports roughly a thousand other findings in this package and I left all of them out. The large ones are style rules whose fix would break the public API (google-explicit-constructor, google-default-arguments) or would change exported signatures and break ABI (misc-const-correctness, performance-unnecessary-value-param). cppcoreguidelines-owning-memory wants gsl::owner, which this project does not use.

Two more are worth fixing but not here:

• bugprone-use-after-move at node_logging.cpp:84. A SetLoggerLevelsResult declared outside the loop is moved into the response on every iteration, so later iterations write into a moved-from object and the success branch never clears reason. libstdc++ leaves the string empty, so you will not see it today, but the state is unspecified.
• bugprone-exception-escape on main() in rclcpp_components/src/component_container.cpp. Different call site, and aborting on a fatal startup error may be what is wanted there.

Backport

All three sites exist on jazzy, so this wants backport-jazzy. Space ROS pins rclcpp on the jazzy branch, so without the backport the fix does not reach it.

[davetcoleman]

@dv-picknik @JWhitleyWork tagging you both for review before this goes anywhere near ros2/rclcpp.

Short version: Space ROS runs clang-tidy over rclcpp, and bugprone-exception-escape flags three destructors that can reach std::terminate. The fix is three try/catch blocks matching what ~Context already does, plus a regression test that takes down the test binary without the fix and passes with it.

GitHub will not let me put you in the Reviewers field because this is my personal fork and you are not collaborators on it, so this mention is the tag. Happy to add you as collaborators if you would rather have the formal review request.

[davetcoleman]

Actually I think i will just open it on rclcpp

[davetcoleman]

This is now open upstream as ros2#3278, so review there rather than here: ros2#3278

Same commit (d870d7c). The upstream description follows the ros2 PR template, which adds a user-facing-behavior-change section and a generative-AI disclosure. @dv-picknik @JWhitleyWork you can comment directly on the upstream PR.