Make get_rcl_allocator a function family

As explained in #1254, there's conceptually no way to implement RCL
allocators in terms of C++ allocators. In order to fix this behavior,
we have to delete the generic version of get_rcl_allocator. Since some
ROS code depends on this, we need to allow users to write their own
version of get_rcl_allocator for allocators that support the C-style
interface (most do).

So this CL changes get_rcl_allocator from a template function
into a family of (potentially templated) functions, which allows
users to add their own overloads and rely on the "most specialized"
mechanism for function specialization to select the right one. See
http://www.gotw.ca/publications/mill17.htm for details. This also
allows us to return get_rcl_default_allocator for all specializations
of std::allocator (previously, only for std::allocator), which will
already fix #1254 for pretty much all clients. I'll continue to work
on deleting the generic version, though, to make sure that nobody is
accidentally bitten by it.

I've tried to test this by doing a full ROS compilation following the
Dockerfile of the source Docker image, and all packages compile.

Signed-off-by: Steve Wolter <swolter@google.com>

rclcpp/CHANGELOG.rst

@@ -2,14 +2,6 @@
 Changelog for package rclcpp
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
-9.0.3 (2021-05-10)
-------------------
-* Use OnShutdown callback handle instead of OnShutdown callback (`#1639 <https://github.com/ros2/rclcpp/issues/1639>`_) (`#1650 <https://github.com/ros2/rclcpp/issues/1650>`_)
-* use dynamic_pointer_cast to detect allocator mismatch in intra process manager (`#1643 <https://github.com/ros2/rclcpp/issues/1643>`_) (`#1644 <https://github.com/ros2/rclcpp/issues/1644>`_)
-* Increase cppcheck timeout to 500s (`#1634 <https://github.com/ros2/rclcpp/issues/1634>`_)
-* Clarify node parameters docs (`#1631 <https://github.com/ros2/rclcpp/issues/1631>`_)
-* Contributors: Barry Xu, Jacob Perron, Michel Hidalgo, Shane Loretz, William Woodall
-
 9.0.2 (2021-04-14)
 ------------------
 * Avoid returning loan when none was obtained. (`#1629 <https://github.com/ros2/rclcpp/issues/1629>`_)

rclcpp/include/rclcpp/allocator/allocator_common.hpp

@@ -29,71 +29,23 @@ namespace allocator
 template<typename T, typename Alloc>
 using AllocRebind = typename std::allocator_traits<Alloc>::template rebind_traits<T>;
 
-template<typename Alloc>
-void * retyped_allocate(size_t size, void * untyped_allocator)
+/// Return the equivalent rcl_allocator_t for the C++ standard allocator.
+/**
+ * This assumes that the user intent behind both allocators is the
+ * same: Using system malloc for allocation.
+ *
+ * If you're using a custom allocator in ROS, you'll need to provide
+ * your own overload for this function.
+ */
+template<typename T>
+rcl_allocator_t get_rcl_allocator(std::allocator<T> allocator)
 {
-  auto typed_allocator = static_cast<Alloc *>(untyped_allocator);
-  if (!typed_allocator) {
-    throw std::runtime_error("Received incorrect allocator type");
-  }
-  return std::allocator_traits<Alloc>::allocate(*typed_allocator, size);
-}
-
-template<typename T, typename Alloc>
-void retyped_deallocate(void * untyped_pointer, void * untyped_allocator)
-{
-  auto typed_allocator = static_cast<Alloc *>(untyped_allocator);
-  if (!typed_allocator) {
-    throw std::runtime_error("Received incorrect allocator type");
-  }
-  auto typed_ptr = static_cast<T *>(untyped_pointer);
-  std::allocator_traits<Alloc>::deallocate(*typed_allocator, typed_ptr, 1);
-}
-
-template<typename T, typename Alloc>
-void * retyped_reallocate(void * untyped_pointer, size_t size, void * untyped_allocator)
-{
-  auto typed_allocator = static_cast<Alloc *>(untyped_allocator);
-  if (!typed_allocator) {
-    throw std::runtime_error("Received incorrect allocator type");
-  }
-  auto typed_ptr = static_cast<T *>(untyped_pointer);
-  std::allocator_traits<Alloc>::deallocate(*typed_allocator, typed_ptr, 1);
-  return std::allocator_traits<Alloc>::allocate(*typed_allocator, size);
-}
-
-
-// Convert a std::allocator_traits-formatted Allocator into an rcl allocator
-template<
-  typename T,
-  typename Alloc,
-  typename std::enable_if<!std::is_same<Alloc, std::allocator<void>>::value>::type * = nullptr>
-rcl_allocator_t get_rcl_allocator(Alloc & allocator)
-{
-  rcl_allocator_t rcl_allocator = rcl_get_default_allocator();
-#ifndef _WIN32
-  rcl_allocator.allocate = &retyped_allocate<Alloc>;
-  rcl_allocator.deallocate = &retyped_deallocate<T, Alloc>;
-  rcl_allocator.reallocate = &retyped_reallocate<T, Alloc>;
-  rcl_allocator.state = &allocator;
-#else
   (void)allocator;  // Remove warning
-#endif
-  return rcl_allocator;
-}
-
-// TODO(jacquelinekay) Workaround for an incomplete implementation of std::allocator<void>
-template<
-  typename T,
-  typename Alloc,
-  typename std::enable_if<std::is_same<Alloc, std::allocator<void>>::value>::type * = nullptr>
-rcl_allocator_t get_rcl_allocator(Alloc & allocator)
-{
-  (void)allocator;
   return rcl_get_default_allocator();
 }
 
 }  // namespace allocator
+
 }  // namespace rclcpp
 
 #endif  // RCLCPP__ALLOCATOR__ALLOCATOR_COMMON_HPP_

rclcpp/include/rclcpp/context.hpp

@@ -23,7 +23,6 @@
 #include <typeindex>
 #include <typeinfo>
 #include <unordered_map>
-#include <unordered_set>
 #include <utility>
 #include <vector>
 
@@ -48,17 +47,6 @@ public:
 /// Forward declare WeakContextsWrapper
 class WeakContextsWrapper;
 
-class OnShutdownCallbackHandle
-{
-  friend class Context;
-
-public:
-  using OnShutdownCallbackType = std::function<void ()>;
-
-private:
-  std::weak_ptr<OnShutdownCallbackType> callback;
-};
-
 /// Context which encapsulates shared state between nodes and other similar entities.
 /**
  * A context also represents the lifecycle between init and shutdown of rclcpp.
@@ -189,7 +177,7 @@ public:
   bool
   shutdown(const std::string & reason);
 
-  using OnShutdownCallback = OnShutdownCallbackHandle::OnShutdownCallbackType;
+  using OnShutdownCallback = std::function<void ()>;
 
   /// Add a on_shutdown callback to be called when shutdown is called for this context.
   /**
@@ -215,47 +203,23 @@ public:
   OnShutdownCallback
   on_shutdown(OnShutdownCallback callback);
 
-  /// Add a on_shutdown callback to be called when shutdown is called for this context.
+  /// Return the shutdown callbacks as const.
   /**
-   * These callbacks will be called in the order they are added as the second
-   * to last step in shutdown().
-   *
-   * When shutdown occurs due to the signal handler, these callbacks are run
-   * asynchronously in the dedicated singal handling thread.
-   *
-   * Also, shutdown() may be called from the destructor of this function.
-   * Therefore, it is not safe to throw exceptions from these callbacks.
-   * Instead, log errors or use some other mechanism to indicate an error has
-   * occurred.
-   *
-   * On shutdown callbacks may be registered before init and after shutdown,
-   * and persist on repeated init's.
-   *
-   * \param[in] callback the on_shutdown callback to be registered
-   * \return the created callback handle
+   * Using the returned reference is not thread-safe with calls that modify
+   * the list of "on shutdown" callbacks, i.e. on_shutdown().
    */
   RCLCPP_PUBLIC
-  virtual
-  OnShutdownCallbackHandle
-  add_on_shutdown_callback(OnShutdownCallback callback);
-
-  /// Remove an registered on_shutdown callbacks.
-  /**
-   * \param[in] callback_handle the on_shutdown callback handle to be removed.
-   * \return true if the callback is found and removed, otherwise false.
-   */
-  RCLCPP_PUBLIC
-  virtual
-  bool
-  remove_on_shutdown_callback(const OnShutdownCallbackHandle & callback_handle);
+  const std::vector<OnShutdownCallback> &
+  get_on_shutdown_callbacks() const;
 
   /// Return the shutdown callbacks.
   /**
-   * Returned callbacks are a copy of the registered callbacks.
+   * Using the returned reference is not thread-safe with calls that modify
+   * the list of "on shutdown" callbacks, i.e. on_shutdown().
    */
   RCLCPP_PUBLIC
-  std::vector<OnShutdownCallback>
-  get_on_shutdown_callbacks() const;
+  std::vector<OnShutdownCallback> &
+  get_on_shutdown_callbacks();
 
   /// Return the internal rcl context.
   RCLCPP_PUBLIC
@@ -335,8 +299,8 @@ private:
   // attempt to acquire another sub context.
   std::recursive_mutex sub_contexts_mutex_;
 
-  std::unordered_set<std::shared_ptr<OnShutdownCallback>> on_shutdown_callbacks_;
-  mutable std::mutex on_shutdown_callbacks_mutex_;
+  std::vector<OnShutdownCallback> on_shutdown_callbacks_;
+  std::mutex on_shutdown_callbacks_mutex_;
 
   /// Condition variable for timed sleep (see sleep_for).
   std::condition_variable interrupt_condition_variable_;

rclcpp/include/rclcpp/executor.hpp

@@ -30,7 +30,6 @@
 #include "rcl/guard_condition.h"
 #include "rcl/wait.h"
 
-#include "rclcpp/context.hpp"
 #include "rclcpp/contexts/default_context.hpp"
 #include "rclcpp/guard_condition.hpp"
 #include "rclcpp/executor_options.hpp"
@@ -572,9 +571,6 @@ protected:
   /// nodes that are associated with the executor
   std::list<rclcpp::node_interfaces::NodeBaseInterface::WeakPtr>
   weak_nodes_ RCPPUTILS_TSA_GUARDED_BY(mutex_);
-
-  /// shutdown callback handle registered to Context
-  rclcpp::OnShutdownCallbackHandle shutdown_callback_handle_;
 };
 
 }  // namespace rclcpp

rclcpp/include/rclcpp/experimental/intra_process_manager.hpp

@@ -202,8 +202,7 @@ public:
       // None of the buffers require ownership, so we promote the pointer
       std::shared_ptr<MessageT> msg = std::move(message);
 
-      this->template add_shared_msg_to_buffers<MessageT, Alloc, Deleter>(
-        msg, sub_ids.take_shared_subscriptions);
+      this->template add_shared_msg_to_buffers<MessageT>(msg, sub_ids.take_shared_subscriptions);
     } else if (!sub_ids.take_ownership_subscriptions.empty() && // NOLINT
       sub_ids.take_shared_subscriptions.size() <= 1)
     {
@@ -228,7 +227,7 @@ public:
       // for the buffers that do not require ownership
       auto shared_msg = std::allocate_shared<MessageT, MessageAllocatorT>(*allocator, *message);
 
-      this->template add_shared_msg_to_buffers<MessageT, Alloc, Deleter>(
+      this->template add_shared_msg_to_buffers<MessageT>(
         shared_msg, sub_ids.take_shared_subscriptions);
       this->template add_owned_msg_to_buffers<MessageT, Alloc, Deleter>(
         std::move(message), sub_ids.take_ownership_subscriptions, allocator);
@@ -264,7 +263,7 @@ public:
       // If there are no owning, just convert to shared.
       std::shared_ptr<MessageT> shared_msg = std::move(message);
       if (!sub_ids.take_shared_subscriptions.empty()) {
-        this->template add_shared_msg_to_buffers<MessageT, Alloc, Deleter>(
+        this->template add_shared_msg_to_buffers<MessageT>(
           shared_msg, sub_ids.take_shared_subscriptions);
       }
       return shared_msg;
@@ -274,7 +273,7 @@ public:
       auto shared_msg = std::allocate_shared<MessageT, MessageAllocatorT>(*allocator, *message);
 
       if (!sub_ids.take_shared_subscriptions.empty()) {
-        this->template add_shared_msg_to_buffers<MessageT, Alloc, Deleter>(
+        this->template add_shared_msg_to_buffers<MessageT>(
           shared_msg,
           sub_ids.take_shared_subscriptions);
       }
@@ -351,10 +350,7 @@ private:
   bool
   can_communicate(PublisherInfo pub_info, SubscriptionInfo sub_info) const;
 
-  template<
-    typename MessageT,
-    typename Alloc,
-    typename Deleter>
+  template<typename MessageT>
   void
   add_shared_msg_to_buffers(
     std::shared_ptr<const MessageT> message,
@@ -367,16 +363,9 @@ private:
       }
       auto subscription_base = subscription_it->second.subscription.lock();
       if (subscription_base) {
-        auto subscription = std::dynamic_pointer_cast<
-          rclcpp::experimental::SubscriptionIntraProcess<MessageT, Alloc, Deleter>
+        auto subscription = std::static_pointer_cast<
+          rclcpp::experimental::SubscriptionIntraProcess<MessageT>
           >(subscription_base);
-        if (nullptr == subscription) {
-          throw std::runtime_error(
-                  "failed to dynamic cast SubscriptionIntraProcessBase to "
-                  "SubscriptionIntraProcess<MessageT, Alloc, Deleter>, which "
-                  "can happen when the publisher and subscription use different "
-                  "allocator types, which is not supported");
-        }
 
         subscription->provide_intra_process_message(message);
       } else {
@@ -405,16 +394,9 @@ private:
       }
       auto subscription_base = subscription_it->second.subscription.lock();
       if (subscription_base) {
-        auto subscription = std::dynamic_pointer_cast<
-          rclcpp::experimental::SubscriptionIntraProcess<MessageT, Alloc, Deleter>
+        auto subscription = std::static_pointer_cast<
+          rclcpp::experimental::SubscriptionIntraProcess<MessageT>
           >(subscription_base);
-        if (nullptr == subscription) {
-          throw std::runtime_error(
-                  "failed to dynamic cast SubscriptionIntraProcessBase to "
-                  "SubscriptionIntraProcess<MessageT, Alloc, Deleter>, which "
-                  "can happen when the publisher and subscription use different "
-                  "allocator types, which is not supported");
-        }
 
         if (std::next(it) == subscription_ids.end()) {
           // If this is the last subscription, give up ownership

rclcpp/include/rclcpp/message_memory_strategy.hpp

@@ -61,7 +61,7 @@ public:
     message_allocator_ = std::make_shared<MessageAlloc>();
     serialized_message_allocator_ = std::make_shared<SerializedMessageAlloc>();
     buffer_allocator_ = std::make_shared<BufferAlloc>();
-    rcutils_allocator_ = allocator::get_rcl_allocator<char, BufferAlloc>(*buffer_allocator_.get());
+    rcutils_allocator_ = rclcpp::allocator::get_rcl_allocator(*buffer_allocator_.get());
   }
 
   explicit MessageMemoryStrategy(std::shared_ptr<Alloc> allocator)
@@ -69,7 +69,7 @@ public:
     message_allocator_ = std::make_shared<MessageAlloc>(*allocator.get());
     serialized_message_allocator_ = std::make_shared<SerializedMessageAlloc>(*allocator.get());
     buffer_allocator_ = std::make_shared<BufferAlloc>(*allocator.get());
-    rcutils_allocator_ = allocator::get_rcl_allocator<char, BufferAlloc>(*buffer_allocator_.get());
+    rcutils_allocator_ = rclcpp::allocator::get_rcl_allocator(*buffer_allocator_.get());
   }
 
   virtual ~MessageMemoryStrategy() = default;

rclcpp/include/rclcpp/publisher_options.hpp

@@ -80,10 +80,7 @@ struct PublisherOptionsWithAllocator : public PublisherOptionsBase
   to_rcl_publisher_options(const rclcpp::QoS & qos) const
   {
     rcl_publisher_options_t result = rcl_publisher_get_default_options();
-    using AllocatorTraits = std::allocator_traits<Allocator>;
-    using MessageAllocatorT = typename AllocatorTraits::template rebind_alloc<MessageT>;
-    auto message_alloc = std::make_shared<MessageAllocatorT>(*this->get_allocator().get());
-    result.allocator = rclcpp::allocator::get_rcl_allocator<MessageT>(*message_alloc);
+    result.allocator = rclcpp::allocator::get_rcl_allocator(*this->get_allocator());
     result.qos = qos.get_rmw_qos_profile();
     result.rmw_publisher_options.require_unique_network_flow_endpoints =
       this->require_unique_network_flow_endpoints;

rclcpp/include/rclcpp/strategies/allocator_memory_strategy.hpp

@@ -437,7 +437,7 @@ public:
 
   rcl_allocator_t get_allocator() override
   {
-    return rclcpp::allocator::get_rcl_allocator<void *, VoidAlloc>(*allocator_.get());
+    return rclcpp::allocator::get_rcl_allocator(*allocator_.get());
   }
 
   size_t number_of_ready_subscriptions() const override

rclcpp/include/rclcpp/subscription_options.hpp

@@ -107,10 +107,7 @@ struct SubscriptionOptionsWithAllocator : public SubscriptionOptionsBase
   to_rcl_subscription_options(const rclcpp::QoS & qos) const
   {
     rcl_subscription_options_t result = rcl_subscription_get_default_options();
-    using AllocatorTraits = std::allocator_traits<Allocator>;
-    using MessageAllocatorT = typename AllocatorTraits::template rebind_alloc<MessageT>;
-    auto message_alloc = std::make_shared<MessageAllocatorT>(*this->get_allocator().get());
-    result.allocator = allocator::get_rcl_allocator<MessageT>(*message_alloc);
+    result.allocator = rclcpp::allocator::get_rcl_allocator(*this->get_allocator());
     result.qos = qos.get_rmw_qos_profile();
     result.rmw_subscription_options.ignore_local_publications = this->ignore_local_publications;
     result.rmw_subscription_options.require_unique_network_flow_endpoints =

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>9.0.3</version>
+  <version>9.0.2</version>
   <description>The ROS client library in C++.</description>
   <maintainer email="ivanpauno@ekumenlabs.com">Ivan Paunovic</maintainer>
   <maintainer email="mabel@openrobotics.org">Mabel Zhang</maintainer>

rclcpp/src/rclcpp/context.cpp

@@ -315,13 +315,9 @@ Context::shutdown(const std::string & reason)
   // set shutdown reason
   shutdown_reason_ = reason;
   // call each shutdown callback
-  {
-    std::lock_guard<std::mutex> lock(on_shutdown_callbacks_mutex_);
-    for (const auto & callback : on_shutdown_callbacks_) {
-      (*callback)();
-    }
+  for (const auto & callback : on_shutdown_callbacks_) {
+    callback();
   }
-
   // interrupt all blocking sleep_for() and all blocking executors or wait sets
   this->interrupt_all_sleep_for();
   // remove self from the global contexts
@@ -348,48 +344,20 @@ Context::shutdown(const std::string & reason)
 rclcpp::Context::OnShutdownCallback
 Context::on_shutdown(OnShutdownCallback callback)
 {
-  add_on_shutdown_callback(callback);
+  on_shutdown_callbacks_.push_back(callback);
   return callback;
 }
 
-rclcpp::OnShutdownCallbackHandle
-Context::add_on_shutdown_callback(OnShutdownCallback callback)
-{
-  auto callback_shared_ptr = std::make_shared<OnShutdownCallback>(callback);
-  {
-    std::lock_guard<std::mutex> lock(on_shutdown_callbacks_mutex_);
-    on_shutdown_callbacks_.emplace(callback_shared_ptr);
-  }
-
-  OnShutdownCallbackHandle callback_handle;
-  callback_handle.callback = callback_shared_ptr;
-  return callback_handle;
-}
-
-bool
-Context::remove_on_shutdown_callback(const OnShutdownCallbackHandle & callback_handle)
-{
-  std::lock_guard<std::mutex> lock(on_shutdown_callbacks_mutex_);
-  auto callback_shared_ptr = callback_handle.callback.lock();
-  if (callback_shared_ptr == nullptr) {
-    return false;
-  }
-  return on_shutdown_callbacks_.erase(callback_shared_ptr) == 1;
-}
-
-std::vector<rclcpp::Context::OnShutdownCallback>
+const std::vector<rclcpp::Context::OnShutdownCallback> &
 Context::get_on_shutdown_callbacks() const
 {
-  std::vector<OnShutdownCallback> callbacks;
+  return on_shutdown_callbacks_;
+}
 
-  {
-    std::lock_guard<std::mutex> lock(on_shutdown_callbacks_mutex_);
-    for (auto & iter : on_shutdown_callbacks_) {
-      callbacks.emplace_back(*iter);
-    }
-  }
-
-  return callbacks;
+std::vector<rclcpp::Context::OnShutdownCallback> &
+Context::get_on_shutdown_callbacks()
+{
+  return on_shutdown_callbacks_;
 }
 
 std::shared_ptr<rcl_context_t>

rclcpp/src/rclcpp/executor.cpp

@@ -56,7 +56,7 @@ Executor::Executor(const rclcpp::ExecutorOptions & options)
     throw_from_rcl_error(ret, "Failed to create interrupt guard condition in Executor constructor");
   }
 
-  shutdown_callback_handle_ = context_->add_on_shutdown_callback(
+  context_->on_shutdown(
     [weak_gc = std::weak_ptr<rclcpp::GuardCondition>{shutdown_guard_condition_}]() {
       auto strong_gc = weak_gc.lock();
       if (strong_gc) {
@@ -138,14 +138,6 @@ Executor::~Executor()
   }
   // Remove and release the sigint guard condition
   memory_strategy_->remove_guard_condition(&shutdown_guard_condition_->get_rcl_guard_condition());
-
-  // Remove shutdown callback handle registered to Context
-  if (!context_->remove_on_shutdown_callback(shutdown_callback_handle_)) {
-    RCUTILS_LOG_ERROR_NAMED(
-      "rclcpp",
-      "failed to remove registered on_shutdown callback");
-    rcl_reset_error();
-  }
 }
 
 std::vector<rclcpp::CallbackGroup::WeakPtr>

rclcpp/test/rclcpp/CMakeLists.txt

@@ -143,13 +143,6 @@ if(TARGET test_intra_process_manager)
   )
   target_link_libraries(test_intra_process_manager ${PROJECT_NAME})
 endif()
-ament_add_gmock(test_intra_process_manager_with_allocators test_intra_process_manager_with_allocators.cpp)
-if(TARGET test_intra_process_manager_with_allocators)
-  ament_target_dependencies(test_intra_process_manager_with_allocators
-    "test_msgs"
-  )
-  target_link_libraries(test_intra_process_manager_with_allocators ${PROJECT_NAME})
-endif()
 ament_add_gtest(test_ring_buffer_implementation test_ring_buffer_implementation.cpp)
 if(TARGET test_ring_buffer_implementation)
   ament_target_dependencies(test_ring_buffer_implementation

rclcpp/test/rclcpp/allocator/test_allocator_common.cpp

@@ -18,42 +18,9 @@
 
 #include "rclcpp/allocator/allocator_common.hpp"
 
-TEST(TestAllocatorCommon, retyped_allocate) {
-  std::allocator<int> allocator;
-  void * untyped_allocator = &allocator;
-  void * allocated_mem =
-    rclcpp::allocator::retyped_allocate<std::allocator<char>>(1u, untyped_allocator);
-  // The more natural check here is ASSERT_NE(nullptr, ptr), but clang static
-  // analysis throws a false-positive memory leak warning.  Use ASSERT_TRUE instead.
-  ASSERT_TRUE(nullptr != allocated_mem);
-
-  auto code = [&untyped_allocator, allocated_mem]() {
-      rclcpp::allocator::retyped_deallocate<int, std::allocator<int>>(
-        allocated_mem, untyped_allocator);
-    };
-  EXPECT_NO_THROW(code());
-
-  allocated_mem = allocator.allocate(1);
-  // The more natural check here is ASSERT_NE(nullptr, ptr), but clang static
-  // analysis throws a false-positive memory leak warning.  Use ASSERT_TRUE instead.
-  ASSERT_TRUE(nullptr != allocated_mem);
-  void * reallocated_mem =
-    rclcpp::allocator::retyped_reallocate<int, std::allocator<int>>(
-    allocated_mem, 2u, untyped_allocator);
-  // The more natural check here is ASSERT_NE(nullptr, ptr), but clang static
-  // analysis throws a false-positive memory leak warning.  Use ASSERT_TRUE instead.
-  ASSERT_TRUE(nullptr != reallocated_mem);
-
-  auto code2 = [&untyped_allocator, reallocated_mem]() {
-      rclcpp::allocator::retyped_deallocate<int, std::allocator<int>>(
-        reallocated_mem, untyped_allocator);
-    };
-  EXPECT_NO_THROW(code2());
-}
-
 TEST(TestAllocatorCommon, get_rcl_allocator) {
   std::allocator<int> allocator;
-  auto rcl_allocator = rclcpp::allocator::get_rcl_allocator<int>(allocator);
+  auto rcl_allocator = rclcpp::allocator::get_rcl_allocator(allocator);
   EXPECT_NE(nullptr, rcl_allocator.allocate);
   EXPECT_NE(nullptr, rcl_allocator.deallocate);
   EXPECT_NE(nullptr, rcl_allocator.reallocate);
@@ -63,8 +30,7 @@ TEST(TestAllocatorCommon, get_rcl_allocator) {
 
 TEST(TestAllocatorCommon, get_void_rcl_allocator) {
   std::allocator<void> allocator;
-  auto rcl_allocator =
-    rclcpp::allocator::get_rcl_allocator<void, std::allocator<void>>(allocator);
+  auto rcl_allocator = rclcpp::allocator::get_rcl_allocator(allocator);
   EXPECT_NE(nullptr, rcl_allocator.allocate);
   EXPECT_NE(nullptr, rcl_allocator.deallocate);
   EXPECT_NE(nullptr, rcl_allocator.reallocate);

rclcpp/test/rclcpp/test_intra_process_manager.cpp

@@ -216,10 +216,7 @@ public:
   const char * topic_name;
 };
 
-template<
-  typename MessageT,
-  typename Alloc = std::allocator<void>,
-  typename Deleter = std::default_delete<MessageT>>
+template<typename MessageT>
 class SubscriptionIntraProcess : public SubscriptionIntraProcessBase
 {
 public:

rclcpp/test/rclcpp/test_intra_process_manager_with_allocators.cpp

@@ -1,276 +0,0 @@
-// Copyright 2021 Open Source Robotics Foundation, Inc.
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//     http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-#include <gmock/gmock.h>
-
-#include <chrono>
-#include <list>
-#include <memory>
-#include <string>
-#include <utility>
-
-#include "test_msgs/msg/empty.hpp"
-
-#include "rclcpp/rclcpp.hpp"
-#include "rclcpp/allocator/allocator_common.hpp"
-#include "rclcpp/strategies/allocator_memory_strategy.hpp"
-
-// For demonstration purposes only, not necessary for allocator_traits
-static uint32_t num_allocs = 0;
-static uint32_t num_deallocs = 0;
-// A very simple custom allocator. Counts calls to allocate and deallocate.
-template<typename T = void>
-struct MyAllocator
-{
-public:
-  using value_type = T;
-  using size_type = std::size_t;
-  using pointer = T *;
-  using const_pointer = const T *;
-  using difference_type = typename std::pointer_traits<pointer>::difference_type;
-
-  MyAllocator() noexcept
-  {
-  }
-
-  ~MyAllocator() noexcept {}
-
-  template<typename U>
-  MyAllocator(const MyAllocator<U> &) noexcept
-  {
-  }
-
-  T * allocate(size_t size, const void * = 0)
-  {
-    if (size == 0) {
-      return nullptr;
-    }
-    num_allocs++;
-    return static_cast<T *>(std::malloc(size * sizeof(T)));
-  }
-
-  void deallocate(T * ptr, size_t size)
-  {
-    (void)size;
-    if (!ptr) {
-      return;
-    }
-    num_deallocs++;
-    std::free(ptr);
-  }
-
-  template<typename U>
-  struct rebind
-  {
-    typedef MyAllocator<U> other;
-  };
-};
-
-template<typename T, typename U>
-constexpr bool operator==(
-  const MyAllocator<T> &,
-  const MyAllocator<U> &) noexcept
-{
-  return true;
-}
-
-template<typename T, typename U>
-constexpr bool operator!=(
-  const MyAllocator<T> &,
-  const MyAllocator<U> &) noexcept
-{
-  return false;
-}
-
-template<
-  typename PublishedMessageAllocatorT,
-  typename PublisherAllocatorT,
-  typename SubscribedMessageAllocatorT,
-  typename SubscriptionAllocatorT,
-  typename MessageMemoryStrategyAllocatorT,
-  typename MemoryStrategyAllocatorT,
-  typename ExpectedExceptionT
->
-void
-do_custom_allocator_test(
-  PublishedMessageAllocatorT published_message_allocator,
-  PublisherAllocatorT publisher_allocator,
-  SubscribedMessageAllocatorT /* subscribed_message_allocator */,  // intentionally unused
-  SubscriptionAllocatorT subscription_allocator,
-  MessageMemoryStrategyAllocatorT message_memory_strategy,
-  MemoryStrategyAllocatorT memory_strategy_allocator)
-{
-  using PublishedMessageAllocTraits =
-    rclcpp::allocator::AllocRebind<test_msgs::msg::Empty, PublishedMessageAllocatorT>;
-  using PublishedMessageAlloc = typename PublishedMessageAllocTraits::allocator_type;
-  using PublishedMessageDeleter =
-    rclcpp::allocator::Deleter<PublishedMessageAlloc, test_msgs::msg::Empty>;
-
-  using SubscribedMessageAllocTraits =
-    rclcpp::allocator::AllocRebind<test_msgs::msg::Empty, SubscribedMessageAllocatorT>;
-  using SubscribedMessageAlloc = typename SubscribedMessageAllocTraits::allocator_type;
-  using SubscribedMessageDeleter =
-    rclcpp::allocator::Deleter<SubscribedMessageAlloc, test_msgs::msg::Empty>;
-
-  // init and node
-  auto context = std::make_shared<rclcpp::Context>();
-  context->init(0, nullptr);
-  auto node = std::make_shared<rclcpp::Node>(
-    "custom_allocator_test",
-    rclcpp::NodeOptions().context(context).use_intra_process_comms(true));
-
-  // publisher
-  auto shared_publisher_allocator = std::make_shared<PublisherAllocatorT>(publisher_allocator);
-  rclcpp::PublisherOptionsWithAllocator<PublisherAllocatorT> publisher_options;
-  publisher_options.allocator = shared_publisher_allocator;
-  auto publisher =
-    node->create_publisher<test_msgs::msg::Empty>("custom_allocator_test", 10, publisher_options);
-
-  // callback for subscription
-  uint32_t counter = 0;
-  std::promise<std::unique_ptr<test_msgs::msg::Empty, SubscribedMessageDeleter>> received_message;
-  auto received_message_future = received_message.get_future();
-  auto callback =
-    [&counter, &received_message](
-    std::unique_ptr<test_msgs::msg::Empty, SubscribedMessageDeleter> msg)
-    {
-      ++counter;
-      received_message.set_value(std::move(msg));
-    };
-
-  // subscription
-  auto shared_subscription_allocator =
-    std::make_shared<SubscriptionAllocatorT>(subscription_allocator);
-  rclcpp::SubscriptionOptionsWithAllocator<SubscriptionAllocatorT> subscription_options;
-  subscription_options.allocator = shared_subscription_allocator;
-  auto shared_message_strategy_allocator =
-    std::make_shared<MessageMemoryStrategyAllocatorT>(message_memory_strategy);
-  auto msg_mem_strat = std::make_shared<
-    rclcpp::message_memory_strategy::MessageMemoryStrategy<
-      test_msgs::msg::Empty,
-      MessageMemoryStrategyAllocatorT
-    >
-    >(shared_message_strategy_allocator);
-  using CallbackMessageT =
-    typename rclcpp::subscription_traits::has_message_type<decltype(callback)>::type;
-  auto subscriber = node->create_subscription<
-    test_msgs::msg::Empty,
-    decltype(callback),
-    SubscriptionAllocatorT,
-    CallbackMessageT,
-    rclcpp::Subscription<CallbackMessageT, SubscriptionAllocatorT>,
-    rclcpp::message_memory_strategy::MessageMemoryStrategy<
-      CallbackMessageT,
-      MessageMemoryStrategyAllocatorT
-    >
-    >(
-    "custom_allocator_test",
-    10,
-    std::forward<decltype(callback)>(callback),
-    subscription_options,
-    msg_mem_strat);
-
-  // executor memory strategy
-  using rclcpp::memory_strategies::allocator_memory_strategy::AllocatorMemoryStrategy;
-  auto shared_memory_strategy_allocator = std::make_shared<MemoryStrategyAllocatorT>(
-    memory_strategy_allocator);
-  std::shared_ptr<rclcpp::memory_strategy::MemoryStrategy> memory_strategy =
-    std::make_shared<AllocatorMemoryStrategy<MemoryStrategyAllocatorT>>(
-    shared_memory_strategy_allocator);
-
-  // executor
-  rclcpp::ExecutorOptions options;
-  options.memory_strategy = memory_strategy;
-  options.context = context;
-  rclcpp::executors::SingleThreadedExecutor executor(options);
-
-  executor.add_node(node);
-
-  // rebind message_allocator to ensure correct type
-  PublishedMessageDeleter message_deleter;
-  PublishedMessageAlloc rebound_message_allocator = published_message_allocator;
-  rclcpp::allocator::set_allocator_for_deleter(&message_deleter, &rebound_message_allocator);
-
-  // allocate a message
-  auto ptr = PublishedMessageAllocTraits::allocate(rebound_message_allocator, 1);
-  PublishedMessageAllocTraits::construct(rebound_message_allocator, ptr);
-  std::unique_ptr<test_msgs::msg::Empty, PublishedMessageDeleter> msg(ptr, message_deleter);
-
-  // publisher and receive
-  if constexpr (std::is_same_v<ExpectedExceptionT, void>) {
-    // no exception expected
-    EXPECT_NO_THROW(
-    {
-      publisher->publish(std::move(msg));
-      executor.spin_until_future_complete(received_message_future, std::chrono::seconds(10));
-    });
-    EXPECT_EQ(ptr, received_message_future.get().get());
-    EXPECT_EQ(1u, counter);
-  } else {
-    // exception expected
-    EXPECT_THROW(
-    {
-      publisher->publish(std::move(msg));
-      executor.spin_until_future_complete(received_message_future, std::chrono::seconds(10));
-    }, ExpectedExceptionT);
-  }
-}
-
-/*
-   This tests the case where a custom allocator is used correctly, i.e. the same
-   custom allocator on both sides.
- */
-TEST(TestIntraProcessManagerWithAllocators, custom_allocator) {
-  using PublishedMessageAllocatorT = std::allocator<void>;
-  using PublisherAllocatorT = std::allocator<void>;
-  using SubscribedMessageAllocatorT = std::allocator<void>;
-  using SubscriptionAllocatorT = std::allocator<void>;
-  using MessageMemoryStrategyAllocatorT = std::allocator<void>;
-  using MemoryStrategyAllocatorT = std::allocator<void>;
-  auto allocator = std::allocator<void>();
-  do_custom_allocator_test<
-    PublishedMessageAllocatorT,
-    PublisherAllocatorT,
-    SubscribedMessageAllocatorT,
-    SubscriptionAllocatorT,
-    MessageMemoryStrategyAllocatorT,
-    MemoryStrategyAllocatorT,
-    void  // no exception expected
-  >(allocator, allocator, allocator, allocator, allocator, allocator);
-}
-
-/*
-   This tests the case where a custom allocator is used incorrectly, i.e. different
-   custom allocators on both sides.
- */
-TEST(TestIntraProcessManagerWithAllocators, custom_allocator_wrong) {
-  // explicitly use a different allocator here to provoke a failure
-  using PublishedMessageAllocatorT = std::allocator<void>;
-  using PublisherAllocatorT = std::allocator<void>;
-  using SubscribedMessageAllocatorT = MyAllocator<void>;
-  using SubscriptionAllocatorT = MyAllocator<void>;
-  using MessageMemoryStrategyAllocatorT = MyAllocator<void>;
-  using MemoryStrategyAllocatorT = std::allocator<void>;
-  auto allocator = std::allocator<void>();
-  auto my_allocator = MyAllocator<void>();
-  do_custom_allocator_test<
-    PublishedMessageAllocatorT,
-    PublisherAllocatorT,
-    SubscribedMessageAllocatorT,
-    SubscriptionAllocatorT,
-    MessageMemoryStrategyAllocatorT,
-    MemoryStrategyAllocatorT,
-    std::runtime_error  // expected exception
-  >(allocator, allocator, my_allocator, my_allocator, my_allocator, allocator);
-}

rclcpp_action/CHANGELOG.rst

@@ -3,12 +3,6 @@ Changelog for package rclcpp_action
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 
-9.0.3 (2021-05-10)
-------------------
-* Returns CancelResponse::REJECT while goal handle failed to transit to CANCELING state (`#1641 <https://github.com/ros2/rclcpp/issues/1641>`_) (`#1653 <https://github.com/ros2/rclcpp/issues/1653>`_)
-* Fix action server deadlock issue that caused by other mutexes locked in CancelCallback (`#1635 <https://github.com/ros2/rclcpp/issues/1635>`_) (`#1646 <https://github.com/ros2/rclcpp/issues/1646>`_)
-* Contributors: Jacob Perron, Kaven Yau, William Woodall
-
 9.0.2 (2021-04-14)
 ------------------
 

rclcpp_action/include/rclcpp_action/server.hpp

@@ -356,26 +356,15 @@ protected:
   CancelResponse
   call_handle_cancel_callback(const GoalUUID & uuid) override
   {
-    std::shared_ptr<ServerGoalHandle<ActionT>> goal_handle;
-    {
-      std::lock_guard<std::mutex> lock(goal_handles_mutex_);
-      auto element = goal_handles_.find(uuid);
-      if (element != goal_handles_.end()) {
-        goal_handle = element->second.lock();
-      }
-    }
-
+    std::lock_guard<std::mutex> lock(goal_handles_mutex_);
     CancelResponse resp = CancelResponse::REJECT;
-    if (goal_handle) {
-      resp = handle_cancel_(goal_handle);
-      if (CancelResponse::ACCEPT == resp) {
-        try {
+    auto element = goal_handles_.find(uuid);
+    if (element != goal_handles_.end()) {
+      std::shared_ptr<ServerGoalHandle<ActionT>> goal_handle = element->second.lock();
+      if (goal_handle) {
+        resp = handle_cancel_(goal_handle);
+        if (CancelResponse::ACCEPT == resp) {
           goal_handle->_cancel_goal();
-        } catch (const rclcpp::exceptions::RCLError & ex) {
-          RCLCPP_DEBUG(
-            rclcpp::get_logger("rclcpp_action"),
-            "Failed to cancel goal in call_handle_cancel_callback: %s", ex.what());
-          return CancelResponse::REJECT;
         }
       }
     }

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>9.0.3</version>
+  <version>9.0.2</version>
   <description>Adds action APIs for C++.</description>
   <maintainer email="ivanpauno@ekumenlabs.com">Ivan Paunovic</maintainer>
   <maintainer email="mabel@openrobotics.org">Mabel Zhang</maintainer>

rclcpp_action/test/test_server.cpp

@@ -1306,15 +1306,3 @@ TEST_F(TestDeadlockServer, deadlock_while_canceled)
   send_goal_request(node_, uuid2_);  // deadlock here
   t.join();
 }
-
-TEST_F(TestDeadlockServer, deadlock_while_succeed_and_canceled)
-{
-  send_goal_request(node_, uuid1_);
-  std::thread t(&TestDeadlockServer::GoalSucceeded, this);
-  rclcpp::sleep_for(std::chrono::milliseconds(50));
-  auto response_ptr = send_cancel_request(node_, uuid1_);
-
-  // current goal handle is not cancelable, so it returns ERROR_REJECTED
-  EXPECT_EQ(CancelResponse::ERROR_REJECTED, response_ptr->return_code);
-  t.join();
-}

rclcpp_components/CHANGELOG.rst

@@ -2,9 +2,6 @@
 Changelog for package rclcpp_components
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
-9.0.3 (2021-05-10)
-------------------
-
 9.0.2 (2021-04-14)
 ------------------
 

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>9.0.3</version>
+  <version>9.0.2</version>
   <description>Package containing tools for dynamically loadable components</description>
   <maintainer email="ivanpauno@ekumenlabs.com">Ivan Paunovic</maintainer>
   <maintainer email="mabel@openrobotics.org">Mabel Zhang</maintainer>

rclcpp_lifecycle/CHANGELOG.rst

@@ -3,9 +3,6 @@ Changelog for package rclcpp_lifecycle
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 
-9.0.3 (2021-05-10)
-------------------
-
 9.0.2 (2021-04-14)
 ------------------
 

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>9.0.3</version>
+  <version>9.0.2</version>
   <description>Package containing a prototype for lifecycle implementation</description>
   <maintainer email="ivanpauno@ekumenlabs.com">Ivan Paunovic</maintainer>
   <maintainer email="mabel@openrobotics.org">Mabel Zhang</maintainer>