Don't let exceptions escape destructors

TimeSource::NodeState, EventsCBGExecutor and WaitResult each call, from their
destructor, a function that throws by contract. A destructor is implicitly
noexcept, so such a throw calls std::terminate instead of unwinding.

Guard each call the way ~Context already does: log the exception through
RCLCPP_ERROR and let destruction finish.

Found with clang-tidy bugprone-exception-escape.

Signed-off-by: Dave Coleman <dave@picknik.ai>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: davetcoleman

rclcpp/include/rclcpp/wait_result.hpp

@@ -23,6 +23,7 @@
 
 #include "rcl/wait.h"
 
+#include "rclcpp/logging.hpp"
 #include "rclcpp/macros.hpp"
 #include "rclcpp/wait_result_kind.hpp"
 
@@ -137,7 +138,16 @@ class WaitResult final
   ~WaitResult()
   {
     if (wait_set_pointer_) {
-      wait_set_pointer_->wait_result_release();
+      // wait_result_release() throws if the wait set is no longer holding this result.
+      // A destructor is noexcept, so letting that escape would terminate the process.
+      try {
+        wait_set_pointer_->wait_result_release();
+      } catch (const std::exception & exc) {
+        RCLCPP_ERROR(
+          rclcpp::get_logger("rclcpp"), "unhandled exception in ~WaitResult(): %s", exc.what());
+      } catch (...) {
+        RCLCPP_ERROR(rclcpp::get_logger("rclcpp"), "unhandled exception in ~WaitResult()");
+      }
     }
   }
 

rclcpp/src/rclcpp/executors/events_cbg_executor/events_cbg_executor.cpp

@@ -116,7 +116,18 @@ EventsCBGExecutor::EventsCBGExecutor(
 
 EventsCBGExecutor::~EventsCBGExecutor()
 {
-  shutdown();
+  // shutdown() removes nodes and callback groups, which throws when an entity is no longer
+  // associated with this executor. A destructor is noexcept, so letting that escape would
+  // terminate the process.
+  try {
+    shutdown();
+  } catch (const std::exception & exc) {
+    RCLCPP_ERROR(
+      rclcpp::get_logger("rclcpp"),
+      "unhandled exception in ~EventsCBGExecutor(): %s", exc.what());
+  } catch (...) {
+    RCLCPP_ERROR(rclcpp::get_logger("rclcpp"), "unhandled exception in ~EventsCBGExecutor()");
+  }
 }
 
 void EventsCBGExecutor::shutdown()

rclcpp/src/rclcpp/time_source.cpp

@@ -203,7 +203,16 @@ class TimeSource::NodeState final
       node_base_ || node_topics_ || node_graph_ || node_services_ ||
       node_logging_ || node_clock_ || node_parameters_)
     {
-      detachNode();
+      // detachNode() removes the parameter callbacks, which throws while a set-parameters
+      // callback is running. A destructor is noexcept, so letting that escape would terminate
+      // the process.
+      try {
+        detachNode();
+      } catch (const std::exception & exc) {
+        RCLCPP_ERROR(logger_, "unhandled exception in ~NodeState(): %s", exc.what());
+      } catch (...) {
+        RCLCPP_ERROR(logger_, "unhandled exception in ~NodeState()");
+      }
     }
   }
 

rclcpp/test/rclcpp/test_time_source.cpp

@@ -24,6 +24,7 @@
 #include <stdexcept>
 #include <string>
 #include <thread>
+#include <vector>
 
 #include "rcl/error_handling.h"
 #include "rcl/time.h"
@@ -203,6 +204,27 @@ TEST_F(TestTimeSource, detachUnattached) {
   ASSERT_NO_THROW(ts.detachNode());
 }
 
+TEST_F(TestTimeSource, destructor_does_not_terminate_when_detach_throws) {
+  auto time_source = std::make_unique<rclcpp::TimeSource>(node);
+
+  // Destroying the time source from inside a set-parameters callback makes its destructor call
+  // NodeParameters::remove_on_set_parameters_callback(), which throws because parameter
+  // modification is disabled while such a callback runs. A destructor is implicitly noexcept, so
+  // an unguarded throw here terminates the process instead of failing this expectation.
+  auto handle = node->add_on_set_parameters_callback(
+    [&time_source](const std::vector<rclcpp::Parameter> &) {
+      time_source.reset();
+      rcl_interfaces::msg::SetParametersResult result;
+      result.successful = true;
+      return result;
+    });
+
+  EXPECT_NO_THROW(node->set_parameter({"use_sim_time", true}));
+  EXPECT_EQ(nullptr, time_source);
+
+  node->remove_on_set_parameters_callback(handle.get());
+}
+
 TEST_F(TestTimeSource, reattach) {
   rclcpp::TimeSource ts;
   // Try reattach