Change which variable we are using.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

rclcpp/CHANGELOG.rst

@@ -2,19 +2,6 @@
 Changelog for package rclcpp
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
-0.8.5 (2020-12-04)
-------------------
-* Warn about unused result of add_on_set_parameters_callback (`#1238 <https://github.com/ros2/rclcpp/issues/1238>`_) (`#1243 <https://github.com/ros2/rclcpp/issues/1243>`_)
-* fix exception message on rcl_clock_init. (`#1194 <https://github.com/ros2/rclcpp/issues/1194>`_)
-* Check if context is valid when looping in spin_some (`#1167 <https://github.com/ros2/rclcpp/issues/1167>`_)
-* Fix spin_until_future_complete: check spinning value (`#1023 <https://github.com/ros2/rclcpp/issues/1023>`_)
-* Fix lock-order-inversion (potential deadlock) (`#1135 <https://github.com/ros2/rclcpp/issues/1135>`_) (`#1137 <https://github.com/ros2/rclcpp/issues/1137>`_)
-* Don't specify calling convention in std::_Binder template (`#952 <https://github.com/ros2/rclcpp/issues/952>`_) (`#1006 <https://github.com/ros2/rclcpp/issues/1006>`_)
-* Add missing service callback registration tracepoint (`#986 <https://github.com/ros2/rclcpp/issues/986>`_) (`#1004 <https://github.com/ros2/rclcpp/issues/1004>`_)
-* Allow node clock use in logging macros (`#969 <https://github.com/ros2/rclcpp/issues/969>`_) (`#970 <https://github.com/ros2/rclcpp/issues/970>`_) (`#981 <https://github.com/ros2/rclcpp/issues/981>`_)
-* Complete published event message when declaring a parameter (`#928 <https://github.com/ros2/rclcpp/issues/928>`_) (`#966 <https://github.com/ros2/rclcpp/issues/966>`_)
-* Contributors: Christophe Bedard, DongheeYe, Ivan Santiago Paunovic, Jacob Perron, Sean Kelly, Shane Loretz, tomoya
-
 0.8.4 (2020-01-17)
 ------------------
 * Intra-process subscriber should use RMW actual qos (ros2`#913 <https://github.com/ros2/rclcpp/issues/913>`_) (`#914 <https://github.com/ros2/rclcpp/issues/914>`_) (`#965 <https://github.com/ros2/rclcpp/issues/965>`_)

rclcpp/CMakeLists.txt

@@ -21,7 +21,7 @@ if(NOT CMAKE_CXX_STANDARD)
   set(CMAKE_CXX_STANDARD 14)
 endif()
 if(CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "Clang")
-  add_compile_options(-Wall -Wextra -Wpedantic)
+  add_compile_options(-Wall -Wextra -Wpedantic -rdynamic -g)
 endif()
 
 include_directories(include)

rclcpp/include/rclcpp/clock.hpp

@@ -16,6 +16,9 @@
 #define RCLCPP__CLOCK_HPP_
 
 #include <functional>
+#include <mutex>
+#include <shared_mutex>  // NOLINT
+#include <unordered_map>
 
 #include "rclcpp/macros.hpp"
 #include "rclcpp/time.hpp"
@@ -138,6 +141,19 @@ private:
   rcl_allocator_t allocator_;
 };
 
+// This code is an ABI-compatible version of the code that went in for
+// https://github.com/ros2/rclcpp/pull/999.  The way that this works is to have
+// a global map of pointers to mutexes, one per clock class that is created.
+// During the rclcpp::Clock constructor, it adds a new one to this map, and
+// during the rclcpp::Clock destructor, it removes it from this map.  When
+// it needs to lock it, it just looks it up in the map.  The map itself has
+// to be protected when adding a new mutex, removing one, or looking it up,
+// hence the g_clock_map_mutex.
+extern std::shared_timed_mutex g_clock_map_mutex;
+extern std::unordered_map<rclcpp::Clock *, std::mutex> g_clock_mutex_map;
+
+std::mutex & get_clock_mutex(rclcpp::Clock * clock);
+
 }  // namespace rclcpp
 
 #endif  // RCLCPP__CLOCK_HPP_

rclcpp/include/rclcpp/executor.hpp

@@ -35,7 +35,6 @@
 #include "rclcpp/node_interfaces/node_base_interface.hpp"
 #include "rclcpp/utilities.hpp"
 #include "rclcpp/visibility_control.hpp"
-#include "rclcpp/scope_exit.hpp"
 
 namespace rclcpp
 {
@@ -245,14 +244,9 @@ public:
     }
     std::chrono::nanoseconds timeout_left = timeout_ns;
 
-    if (spinning.exchange(true)) {
-      throw std::runtime_error("spin_until_future_complete() called while already spinning");
-    }
-    RCLCPP_SCOPE_EXIT(this->spinning.store(false); );
-    while (rclcpp::ok(this->context_) && spinning.load()) {
+    while (rclcpp::ok(this->context_)) {
       // Do one item of work.
-      spin_once_impl(timeout_left);
-
+      spin_once(timeout_left);
       // Check if the future is set, return SUCCESS if it is.
       status = future.wait_for(std::chrono::seconds(0));
       if (status == std::future_status::ready) {
@@ -366,10 +360,6 @@ protected:
 private:
   RCLCPP_DISABLE_COPY(Executor)
 
-  RCLCPP_PUBLIC
-  void
-  spin_once_impl(std::chrono::nanoseconds timeout);
-
   std::list<rclcpp::node_interfaces::NodeBaseInterface::WeakPtr> weak_nodes_;
   std::list<const rcl_guard_condition_t *> guard_conditions_;
 };

rclcpp/include/rclcpp/node.hpp

@@ -26,8 +26,6 @@
 #include <utility>
 #include <vector>
 
-#include "rcutils/macros.h"
-
 #include "rcl/error_handling.h"
 #include "rcl/node.h"
 
@@ -796,7 +794,6 @@ public:
    * \throws std::bad_alloc if the allocation of the OnSetParametersCallbackHandle fails.
    */
   RCLCPP_PUBLIC
-  RCUTILS_WARN_UNUSED
   OnSetParametersCallbackHandle::SharedPtr
   add_on_set_parameters_callback(OnParametersSetCallbackType callback);
 

rclcpp/include/rclcpp/node_interfaces/node_parameters.hpp

@@ -21,8 +21,6 @@
 #include <string>
 #include <vector>
 
-#include "rcutils/macros.h"
-
 #include "rcl_interfaces/msg/list_parameters_result.hpp"
 #include "rcl_interfaces/msg/parameter_descriptor.hpp"
 #include "rcl_interfaces/msg/parameter_event.hpp"
@@ -161,7 +159,6 @@ public:
   list_parameters(const std::vector<std::string> & prefixes, uint64_t depth) const override;
 
   RCLCPP_PUBLIC
-  RCUTILS_WARN_UNUSED
   OnSetParametersCallbackHandle::SharedPtr
   add_on_set_parameters_callback(OnParametersSetCallbackType callback) override;
 

rclcpp/package.xml

@@ -2,7 +2,7 @@
 <?xml-model href="http://download.ros.org/schema/package_format2.xsd" schematypens="http://www.w3.org/2001/XMLSchema"?>
 <package format="2">
   <name>rclcpp</name>
-  <version>0.8.5</version>
+  <version>0.8.4</version>
   <description>The ROS client library in C++.</description>
   <maintainer email="dthomas@osrfoundation.org">Dirk Thomas</maintainer>
   <license>Apache License 2.0</license>

rclcpp/src/rclcpp/clock.cpp

@@ -12,6 +12,15 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+#include <mutex>
+#include <shared_mutex>  // NOLINT
+#include <unordered_map>
+
+#include <cstdio>
+#include <cstdlib>
+#include <execinfo.h>
+#include <cxxabi.h>
+
 #include "rclcpp/clock.hpp"
 
 #include "rclcpp/exceptions.hpp"
@@ -21,6 +30,93 @@
 namespace rclcpp
 {
 
+std::shared_timed_mutex g_clock_map_mutex;
+std::unordered_map<rclcpp::Clock *, std::mutex> g_clock_mutex_map;
+
+/** Print a demangled stack backtrace of the caller function to FILE* out. */
+void print_backtrace(FILE *out = stderr, unsigned int max_frames = 63)
+{
+  fprintf(out, "stack trace:\n");
+
+  // storage array for stack trace address data
+  void* addrlist[max_frames+1];
+
+  // retrieve current stack addresses
+  int addrlen = backtrace(addrlist, sizeof(addrlist) / sizeof(void*));
+
+  if (addrlen == 0) {
+    fprintf(out, "  <empty, possibly corrupt>\n");
+    return;
+  }
+
+  // resolve addresses into strings containing "filename(function+address)",
+  // this array must be free()-ed
+  char** symbollist = backtrace_symbols(addrlist, addrlen);
+
+  // allocate string which will be filled with the demangled function name
+  size_t funcnamesize = 256;
+  char* funcname = (char*)malloc(funcnamesize);
+
+  // iterate over the returned symbol lines. skip the first, it is the
+  // address of this function.
+  for (int i = 1; i < addrlen; i++) {
+    char *begin_name = 0, *begin_offset = 0, *end_offset = 0;
+
+    // find parentheses and +address offset surrounding the mangled name:
+    // ./module(function+0x15c) [0x8048a6d]
+    for (char *p = symbollist[i]; *p; ++p) {
+      if (*p == '(') {
+        begin_name = p;
+      } else if (*p == '+') {
+        begin_offset = p;
+      } else if (*p == ')' && begin_offset) {
+        end_offset = p;
+        break;
+      }
+    }
+
+    if (begin_name && begin_offset && end_offset && begin_name < begin_offset) {
+      *begin_name++ = '\0';
+      *begin_offset++ = '\0';
+      *end_offset = '\0';
+
+      // mangled name is now in [begin_name, begin_offset) and caller
+      // offset in [begin_offset, end_offset). now apply
+      // __cxa_demangle():
+
+      int status;
+      char* ret = abi::__cxa_demangle(begin_name, funcname, &funcnamesize, &status);
+      if (status == 0) {
+        funcname = ret; // use possibly realloc()-ed string
+        fprintf(out, "  %s : %s+%s\n", symbollist[i], funcname, begin_offset);
+      }
+      else {
+        // demangling failed. Output function name as a C function with
+        // no arguments.
+        fprintf(out, "  %s : %s()+%s\n", symbollist[i], begin_name, begin_offset);
+      }
+    }
+    else {
+      // couldn't parse the line? print the whole line.
+      fprintf(out, "  %s\n", symbollist[i]);
+    }
+  }
+
+  free(funcname);
+  free(symbollist);
+}
+
+std::mutex & get_clock_mutex(rclcpp::Clock * clock)
+{
+  std::shared_lock<std::shared_timed_mutex> lk(g_clock_map_mutex);
+  try {
+    return g_clock_mutex_map.at(clock);
+  } catch (const std::out_of_range &) {
+    print_backtrace();
+    throw;
+  }
+}
+
 JumpHandler::JumpHandler(
   pre_callback_t pre_callback,
   post_callback_t post_callback,
@@ -35,12 +131,26 @@ Clock::Clock(rcl_clock_type_t clock_type)
   allocator_ = rcl_get_default_allocator();
   auto ret = rcl_clock_init(clock_type, &rcl_clock_, &allocator_);
   if (ret != RCL_RET_OK) {
-    exceptions::throw_from_rcl_error(ret, "failed to initialize rcl clock");
+    exceptions::throw_from_rcl_error(ret, "could not get current time stamp");
+  }
+
+  {
+    std::lock_guard<std::shared_timed_mutex> lg(g_clock_map_mutex);
+    // Add a new default-constructed mutex, keyed off of this pointer.
+    fprintf(stderr, "Adding clock mutex for %p\n", static_cast<void *>(this));
+    g_clock_mutex_map[this];
   }
 }
 
 Clock::~Clock()
 {
+  {
+    std::lock_guard<std::shared_timed_mutex> lg(g_clock_map_mutex);
+    // Remove the mutex corresponding to this clock object.
+    g_clock_mutex_map.erase(this);
+    fprintf(stderr, "Removed clock mutex for %p\n", static_cast<void *>(this));
+  }
+
   auto ret = rcl_clock_fini(&rcl_clock_);
   if (ret != RCL_RET_OK) {
     RCUTILS_LOG_ERROR("Failed to fini rcl clock.");
@@ -118,18 +228,25 @@ Clock::create_jump_callback(
     throw std::bad_alloc{};
   }
 
-  // Try to add the jump callback to the clock
-  rcl_ret_t ret = rcl_clock_add_jump_callback(
-    &rcl_clock_, threshold, Clock::on_time_jump,
-    handler.get());
-  if (RCL_RET_OK != ret) {
-    exceptions::throw_from_rcl_error(ret, "Failed to add time jump callback");
+  {
+    fprintf(stderr, "create_jump_callback() getting clock mutex for %p\n", static_cast<void *>(this));
+    std::lock_guard<std::mutex> clock_guard(get_clock_mutex(this));
+    // Try to add the jump callback to the clock
+    rcl_ret_t ret = rcl_clock_add_jump_callback(
+      &rcl_clock_, threshold, Clock::on_time_jump,
+      handler.get());
+    if (RCL_RET_OK != ret) {
+      exceptions::throw_from_rcl_error(ret, "Failed to add time jump callback");
+    }
   }
 
   // *INDENT-OFF*
   // create shared_ptr that removes the callback automatically when all copies are destructed
   // TODO(dorezyuk) UB, if the clock leaves scope before the JumpHandler
   return JumpHandler::SharedPtr(handler.release(), [this](JumpHandler * handler) noexcept {
+    fprintf(stderr, "create_jump_callback release lambda getting clock mutex for %p\n", static_cast<void *>(this));
+    std::lock_guard<std::mutex> clock_guard(get_clock_mutex(this));
+
     rcl_ret_t ret = rcl_clock_remove_jump_callback(&rcl_clock_, Clock::on_time_jump,
         handler);
     delete handler;

rclcpp/src/rclcpp/executor.cpp

@@ -234,7 +234,7 @@ Executor::spin_some(std::chrono::nanoseconds max_duration)
     throw std::runtime_error("spin_some() called while already spinning");
   }
   RCLCPP_SCOPE_EXIT(this->spinning.store(false); );
-  while (rclcpp::ok(context_) && spinning.load() && max_duration_not_elapsed()) {
+  while (spinning.load() && max_duration_not_elapsed()) {
     AnyExecutable any_exec;
     if (get_next_executable(any_exec, std::chrono::milliseconds::zero())) {
       execute_any_executable(any_exec);
@@ -244,15 +244,6 @@ Executor::spin_some(std::chrono::nanoseconds max_duration)
   }
 }
 
-void
-Executor::spin_once_impl(std::chrono::nanoseconds timeout)
-{
-  AnyExecutable any_exec;
-  if (get_next_executable(any_exec, timeout)) {
-    execute_any_executable(any_exec);
-  }
-}
-
 void
 Executor::spin_once(std::chrono::nanoseconds timeout)
 {
@@ -260,7 +251,10 @@ Executor::spin_once(std::chrono::nanoseconds timeout)
     throw std::runtime_error("spin_once() called while already spinning");
   }
   RCLCPP_SCOPE_EXIT(this->spinning.store(false); );
-  spin_once_impl(timeout);
+  AnyExecutable any_exec;
+  if (get_next_executable(any_exec, timeout)) {
+    execute_any_executable(any_exec);
+  }
 }
 
 void

rclcpp/src/rclcpp/timer.cpp

@@ -17,7 +17,9 @@
 #include <chrono>
 #include <string>
 #include <memory>
+#include <mutex>
 
+#include "rclcpp/clock.hpp"
 #include "rclcpp/contexts/default_context.hpp"
 #include "rclcpp/exceptions.hpp"
 
@@ -40,11 +42,15 @@ TimerBase::TimerBase(
   timer_handle_ = std::shared_ptr<rcl_timer_t>(
     new rcl_timer_t, [ = ](rcl_timer_t * timer) mutable
     {
-      if (rcl_timer_fini(timer) != RCL_RET_OK) {
-        RCUTILS_LOG_ERROR_NAMED(
-          "rclcpp",
-          "Failed to clean up rcl timer handle: %s", rcl_get_error_string().str);
-        rcl_reset_error();
+      {
+        fprintf(stderr, "TimerBase handle lambda getting clock mutex for %p\n", static_cast<void *>(clock.get()));
+        std::lock_guard<std::mutex> clock_guard(get_clock_mutex(clock.get()));
+        if (rcl_timer_fini(timer) != RCL_RET_OK) {
+          RCUTILS_LOG_ERROR_NAMED(
+            "rclcpp",
+            "Failed to clean up rcl timer handle: %s", rcl_get_error_string().str);
+          rcl_reset_error();
+        }
       }
       delete timer;
       // Captured shared pointers by copy, reset to make sure timer is finalized before clock
@@ -55,15 +61,19 @@ TimerBase::TimerBase(
   *timer_handle_.get() = rcl_get_zero_initialized_timer();
 
   rcl_clock_t * clock_handle = clock_->get_clock_handle();
-  if (
-    rcl_timer_init(
-      timer_handle_.get(), clock_handle, rcl_context.get(), period.count(), nullptr,
-      rcl_get_default_allocator()) != RCL_RET_OK)
   {
-    RCUTILS_LOG_ERROR_NAMED(
-      "rclcpp",
-      "Couldn't initialize rcl timer handle: %s\n", rcl_get_error_string().str);
-    rcl_reset_error();
+    fprintf(stderr, "TimerBase constructor getting clock mutex for %p\n", static_cast<void *>(this->clock_.get()));
+    std::lock_guard<std::mutex> clock_guard(get_clock_mutex(this->clock_.get()));
+    if (
+      rcl_timer_init(
+        timer_handle_.get(), clock_handle, rcl_context.get(), period.count(), nullptr,
+        rcl_get_default_allocator()) != RCL_RET_OK)
+    {
+      RCUTILS_LOG_ERROR_NAMED(
+        "rclcpp",
+        "Couldn't initialize rcl timer handle: %s\n", rcl_get_error_string().str);
+      rcl_reset_error();
+    }
   }
 }
 

rclcpp_action/CHANGELOG.rst

@@ -3,9 +3,6 @@ Changelog for package rclcpp_action
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 
-0.8.5 (2020-12-04)
-------------------
-
 0.8.4 (2020-01-17)
 ------------------
 

rclcpp_action/package.xml

@@ -2,7 +2,7 @@
 <?xml-model href="http://download.ros.org/schema/package_format2.xsd" schematypens="http://www.w3.org/2001/XMLSchema"?>
 <package format="2">
   <name>rclcpp_action</name>
-  <version>0.8.5</version>
+  <version>0.8.4</version>
   <description>Adds action APIs for C++.</description>
   <maintainer email="dthomas@osrfoundation.org">Dirk Thomas</maintainer>
   <license>Apache License 2.0</license>

rclcpp_components/CHANGELOG.rst

@@ -2,11 +2,6 @@
 Changelog for package rclcpp_components
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
-0.8.5 (2020-12-04)
-------------------
-* Added rclcpp_components Doxyfile (`#1091 <https://github.com/ros2/rclcpp/issues/1091>`_) (`#1200 <https://github.com/ros2/rclcpp/issues/1200>`_)
-* Contributors: Alejandro Hernández Cordero
-
 0.8.4 (2020-01-17)
 ------------------
 * Remove absolute path from installed CMake code (`#948 <https://github.com/ros2/rclcpp/issues/948>`_) (`#950 <https://github.com/ros2/rclcpp/issues/950>`_)

rclcpp_components/Doxyfile

@@ -1,33 +0,0 @@
-# All settings not listed here will use the Doxygen default values.
-
-PROJECT_NAME           = "rclcpp_components"
-PROJECT_NUMBER         = master
-PROJECT_BRIEF          = "Package containing tools for dynamically loadable components"
-
-# Use these lines to include the generated logging.hpp (update install path if needed)
-# Otherwise just generate for the local (non-generated header files)
-INPUT                  = ./include
-
-RECURSIVE              = YES
-OUTPUT_DIRECTORY       = doc_output
-
-EXTRACT_ALL            = YES
-SORT_MEMBER_DOCS       = NO
-
-GENERATE_LATEX         = NO
-
-ENABLE_PREPROCESSING   = YES
-MACRO_EXPANSION        = YES
-EXPAND_ONLY_PREDEF     = YES
-PREDEFINED             = RCLCPP_COMPONENTS_PUBLIC=
-
-# Tag files that do not exist will produce a warning and cross-project linking will not work.
-TAGFILES += "../../../../doxygen_tag_files/cppreference-doxygen-web.tag.xml=http://en.cppreference.com/w/"
-# Consider changing "latest" to the version you want to reference (e.g. beta1 or 1.0.0)
-TAGFILES += "../../../../doxygen_tag_files/class_loader.tag=http://docs.ros2.org/latest/api/class_loader/"
-TAGFILES += "../../../../doxygen_tag_files/rclcpp.tag=http://docs.ros2.org/latest/api/rclcpp/"
-TAGFILES += "../../../../doxygen_tag_files/rcl.tag=http://docs.ros2.org/latest/api/rcl/"
-TAGFILES += "../../../../doxygen_tag_files/rcutils.tag=http://docs.ros2.org/latest/api/rcutils/"
-TAGFILES += "../../../../doxygen_tag_files/rmw.tag=http://docs.ros2.org/latest/api/rmw/"
-# Uncomment to generate tag files for cross-project linking.
-GENERATE_TAGFILE = "../../../../doxygen_tag_files/rclcpp_components.tag"

rclcpp_components/package.xml

@@ -2,7 +2,7 @@
 <?xml-model href="http://download.ros.org/schema/package_format2.xsd" schematypens="http://www.w3.org/2001/XMLSchema"?>
 <package format="2">
   <name>rclcpp_components</name>
-  <version>0.8.5</version>
+  <version>0.8.4</version>
   <description>Package containing tools for dynamically loadable components</description>
   <maintainer email="michael@openrobotics.org">Michael Carroll</maintainer>
   <license>Apache License 2.0</license>

rclcpp_lifecycle/CHANGELOG.rst

@@ -3,9 +3,6 @@ Changelog for package rclcpp_lifecycle
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 
-0.8.5 (2020-12-04)
-------------------
-
 0.8.4 (2020-01-17)
 ------------------
 

rclcpp_lifecycle/package.xml

@@ -2,7 +2,7 @@
 <?xml-model href="http://download.ros.org/schema/package_format2.xsd" schematypens="http://www.w3.org/2001/XMLSchema"?>
 <package format="2">
   <name>rclcpp_lifecycle</name>
-  <version>0.8.5</version>
+  <version>0.8.4</version>
   <description>Package containing a prototype for lifecycle implementation</description>
   <maintainer email="karsten@osrfoundation.org">Karsten Knese</maintainer>
   <license>Apache License 2.0</license>