From 397cde7ee9c4df0e0caa9cef27261dcd14dc1e69 Mon Sep 17 00:00:00 2001
From: William Woodall <william@osrfoundation.org>
Date: Mon, 29 Jun 2015 13:42:54 -0700
Subject: [PATCH] Fix some lifecycle issues in the TimerBase class.

---
 rclcpp/include/rclcpp/client.hpp       |  8 +++++---
 rclcpp/include/rclcpp/node_impl.hpp    |  7 +++++--
 rclcpp/include/rclcpp/publisher.hpp    |  2 ++
 rclcpp/include/rclcpp/subscription.hpp |  5 ++++-
 rclcpp/include/rclcpp/timer.hpp        | 22 +++++++++++++++++++++-
 5 files changed, 37 insertions(+), 7 deletions(-)

diff --git a/rclcpp/include/rclcpp/client.hpp b/rclcpp/include/rclcpp/client.hpp
index 8b41b90..757bcb7 100644
--- a/rclcpp/include/rclcpp/client.hpp
+++ b/rclcpp/include/rclcpp/client.hpp
@@ -57,9 +57,11 @@ public:
   {
     if (client_handle_) {
       if (rmw_destroy_client(client_handle_) == RMW_RET_ERROR) {
-        std::cerr << "Error in destruction of rmw client handle: " <<
-        (rmw_get_error_string() ? rmw_get_error_string() : "") <<
-          std::endl;
+        // *INDENT-OFF*
+        std::cerr << "Error in destruction of rmw client handle: "
+                  << (rmw_get_error_string() ? rmw_get_error_string() : "")
+                  << std::endl;
+        // *INDENT-ON*
       }
     }
   }
diff --git a/rclcpp/include/rclcpp/node_impl.hpp b/rclcpp/include/rclcpp/node_impl.hpp
index 5f045ba..be2d241 100644
--- a/rclcpp/include/rclcpp/node_impl.hpp
+++ b/rclcpp/include/rclcpp/node_impl.hpp
@@ -50,14 +50,16 @@ Node::Node(std::string node_name, context::Context::SharedPtr context)
     if (node_handle_) {
       auto ret = rmw_destroy_node(node);
       if (ret != RMW_RET_OK) {
+        // *INDENT-OFF*
         std::cerr << "Error in destruction of rmw node handle: "
                   << (rmw_get_error_string() ? rmw_get_error_string() : "")
                   << std::endl;
+        // *INDENT-ON*
       }
     }
   });
   if (!node_handle_) {
-    // *INDENT-OFF* (prevent uncrustify from making unecessary indents here)
+    // *INDENT-OFF*
     throw std::runtime_error(
       std::string("could not create node: ") +
       (rmw_get_error_string() ? rmw_get_error_string() : ""));
@@ -127,7 +129,8 @@ Node::create_subscription(
   using rosidl_generator_cpp::get_message_type_support_handle;
   auto type_support_handle = get_message_type_support_handle<MessageT>();
   rmw_subscription_t * subscriber_handle = rmw_create_subscription(
-    node_handle_.get(), type_support_handle, topic_name.c_str(), queue_size, ignore_local_publications);
+    node_handle_.get(), type_support_handle,
+    topic_name.c_str(), queue_size, ignore_local_publications);
   if (!subscriber_handle) {
     // *INDENT-OFF* (prevent uncrustify from making unecessary indents here)
     throw std::runtime_error(
diff --git a/rclcpp/include/rclcpp/publisher.hpp b/rclcpp/include/rclcpp/publisher.hpp
index 9b414d6..42a8506 100644
--- a/rclcpp/include/rclcpp/publisher.hpp
+++ b/rclcpp/include/rclcpp/publisher.hpp
@@ -48,9 +48,11 @@ public:
   {
     if (publisher_handle_) {
       if (rmw_destroy_publisher(node_handle_.get(), publisher_handle_) == RMW_RET_ERROR) {
+        // *INDENT-OFF*
         std::cerr << "Error in destruction of rmw publisher handle: "
                   << (rmw_get_error_string() ? rmw_get_error_string() : "")
                   << std::endl;
+        // *INDENT-ON*
       }
     }
   }
diff --git a/rclcpp/include/rclcpp/subscription.hpp b/rclcpp/include/rclcpp/subscription.hpp
index 40fad86..de00b04 100644
--- a/rclcpp/include/rclcpp/subscription.hpp
+++ b/rclcpp/include/rclcpp/subscription.hpp
@@ -53,7 +53,10 @@ public:
     subscription_handle_(subscription_handle),
     topic_name_(topic_name),
     ignore_local_publications_(ignore_local_publications)
-  {}
+  {
+    // To avoid unused private member warnings.
+    (void)ignore_local_publications_;
+  }
 
   ~SubscriptionBase()
   {
diff --git a/rclcpp/include/rclcpp/timer.hpp b/rclcpp/include/rclcpp/timer.hpp
index d378669..d87c71e 100644
--- a/rclcpp/include/rclcpp/timer.hpp
+++ b/rclcpp/include/rclcpp/timer.hpp
@@ -20,6 +20,7 @@
 #include <memory>
 #include <thread>
 
+#include <rmw/error_handling.h>
 #include <rmw/rmw.h>
 
 #include <rclcpp/macros.hpp>
@@ -50,9 +51,27 @@ public:
   TimerBase(std::chrono::nanoseconds period, CallbackType callback)
   : period_(period),
     callback_(callback),
+    guard_condition_(rmw_create_guard_condition()),
     canceled_(false)
   {
-    guard_condition_ = rmw_create_guard_condition();
+    if (!guard_condition_) {
+      // TODO(wjwwood): use custom exception
+      throw std::runtime_error(
+              std::string("failed to create guard condition: ") +
+              (rmw_get_error_string() ? rmw_get_error_string() : "")
+      );
+    }
+  }
+
+  ~TimerBase()
+  {
+    if (guard_condition_) {
+      if (rmw_destroy_guard_condition(guard_condition_) == RMW_RET_ERROR) {
+        std::cerr << "Error in TimerBase destructor, rmw_destroy_guard_condition failed: " <<
+        (rmw_get_error_string() ? rmw_get_error_string() : "") <<
+          std::endl;
+      }
+    }
   }
 
   void
@@ -89,6 +108,7 @@ public:
   ~GenericTimer()
   {
     cancel();
+    thread_.join();
   }
 
   void