Make get_rcl_allocator a function family

As explained in ros2#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 ros2#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>

Addressed code style test failures.

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

Update comments.

Incidentally, this also reruns the flaky test suite, which
seems to be using the real DDS middleware and to be prone
to cross-talk.

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

Also update recently added tests.

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

Added reference to bug number.

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

Extend allocator lifetime in options.

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

Drop the generic version of get_rcl_allocator.

This allows us to simplify a bunch of code as well.

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

Update rclcpp/include/rclcpp/allocator/allocator_common.hpp

Co-authored-by: William Woodall <william+github@osrfoundation.org>
Signed-off-by: Steve Wolter <swolter@google.com>

Revert accidental deletion of allocator creation.

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

rclcpp/include/rclcpp/allocator/allocator_common.hpp

@@ -30,14 +30,19 @@ 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);
+  (void)allocator;     // Remove warning
+  return rcl_get_default_allocator();
 }
 
 template<typename Alloc>
@@ -56,61 +61,6 @@ void * retyped_zero_allocate(size_t number_of_elem, size_t size_of_elem, void *
   return allocated_memory;
 }
 
-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.zero_allocate = &retyped_zero_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
 

rclcpp/include/rclcpp/message_memory_strategy.hpp

@@ -18,6 +18,7 @@
 #include <memory>
 #include <stdexcept>
 
+#include "rcl/allocator.h"
 #include "rcl/types.h"
 
 #include "rclcpp/allocator/allocator_common.hpp"
@@ -61,15 +62,15 @@ class MessageMemoryStrategy
     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_ = rcl_get_default_allocator();
   }
 
   explicit MessageMemoryStrategy(std::shared_ptr<Alloc> allocator)
   {
     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_ = rcl_get_default_allocator();
   }
 
   virtual ~MessageMemoryStrategy() = default;

rclcpp/include/rclcpp/publisher_options.hpp

@@ -20,6 +20,7 @@
 #include <type_traits>
 #include <vector>
 
+#include "rcl/allocator.h"
 #include "rcl/publisher.h"
 
 #include "rclcpp/allocator/allocator_common.hpp"
@@ -77,15 +78,16 @@ struct PublisherOptionsWithAllocator : public PublisherOptionsBase
   /// Constructor using base class as input.
   explicit PublisherOptionsWithAllocator(const PublisherOptionsBase & publisher_options_base)
   : PublisherOptionsBase(publisher_options_base)
-  {}
+  {
+  }
 
   /// Convert this class, and a rclcpp::QoS, into an rcl_publisher_options_t.
   template<typename MessageT>
   rcl_publisher_options_t
   to_rcl_publisher_options(const rclcpp::QoS & qos) const
   {
     rcl_publisher_options_t result = rcl_publisher_get_default_options();
-    result.allocator = this->get_rcl_allocator();
+    result.allocator = rcl_get_default_allocator();
     result.qos = qos.get_rmw_qos_profile();
     result.rmw_publisher_options.require_unique_network_flow_endpoints =
       this->require_unique_network_flow_endpoints;
@@ -98,7 +100,6 @@ struct PublisherOptionsWithAllocator : public PublisherOptionsBase
     return result;
   }
 
-
   /// Get the allocator, creating one if needed.
   std::shared_ptr<Allocator>
   get_allocator() const
@@ -123,7 +124,7 @@ struct PublisherOptionsWithAllocator : public PublisherOptionsBase
       plain_allocator_storage_ =
         std::make_shared<PlainAllocator>(*this->get_allocator());
     }
-    return rclcpp::allocator::get_rcl_allocator<char>(*plain_allocator_storage_);
+    return rcl_get_default_allocator();
   }
 
   // This is a temporal workaround, to make sure that get_allocator()

rclcpp/include/rclcpp/strategies/allocator_memory_strategy.hpp

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

rclcpp/include/rclcpp/subscription_options.hpp

@@ -21,6 +21,8 @@
 #include <type_traits>
 #include <vector>
 
+#include "rcl/allocator.h"
+
 #include "rclcpp/callback_group.hpp"
 #include "rclcpp/detail/rmw_implementation_specific_subscription_payload.hpp"
 #include "rclcpp/intra_process_buffer_type.hpp"
@@ -103,7 +105,8 @@ struct SubscriptionOptionsWithAllocator : public SubscriptionOptionsBase
   explicit SubscriptionOptionsWithAllocator(
     const SubscriptionOptionsBase & subscription_options_base)
   : SubscriptionOptionsBase(subscription_options_base)
-  {}
+  {
+  }
 
   /// Convert this class, with a rclcpp::QoS, into an rcl_subscription_options_t.
   /**
@@ -115,7 +118,7 @@ struct SubscriptionOptionsWithAllocator : public SubscriptionOptionsBase
   to_rcl_subscription_options(const rclcpp::QoS & qos) const
   {
     rcl_subscription_options_t result = rcl_subscription_get_default_options();
-    result.allocator = this->get_rcl_allocator();
+    result.allocator = rcl_get_default_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 =
@@ -167,7 +170,7 @@ struct SubscriptionOptionsWithAllocator : public SubscriptionOptionsBase
       plain_allocator_storage_ =
         std::make_shared<PlainAllocator>(*this->get_allocator());
     }
-    return rclcpp::allocator::get_rcl_allocator<char>(*plain_allocator_storage_);
+    return rcl_get_default_allocator();
   }
 
   // This is a temporal workaround, to make sure that get_allocator()

rclcpp/test/rclcpp/allocator/test_allocator_common.cpp

@@ -18,100 +18,41 @@
 
 #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, retyped_zero_allocate_basic) {
+TEST(TestAllocatorCommon, retyped_zero_allocate_basic)
+{
   std::allocator<int> allocator;
   void * untyped_allocator = &allocator;
   void * allocated_mem =
     rclcpp::allocator::retyped_zero_allocate<std::allocator<char>>(20u, 1u, untyped_allocator);
   ASSERT_TRUE(nullptr != allocated_mem);
-
-  auto code = [&untyped_allocator, allocated_mem]() {
-      rclcpp::allocator::retyped_deallocate<char, std::allocator<char>>(
-        allocated_mem, untyped_allocator);
-    };
-  EXPECT_NO_THROW(code());
 }
 
-TEST(TestAllocatorCommon, retyped_zero_allocate) {
+TEST(TestAllocatorCommon, retyped_zero_allocate)
+{
   std::allocator<int> allocator;
   void * untyped_allocator = &allocator;
   void * allocated_mem =
     rclcpp::allocator::retyped_zero_allocate<std::allocator<char>>(20u, 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) {
+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);
   EXPECT_NE(nullptr, rcl_allocator.zero_allocate);
   // Not testing state as that may or may not be null depending on platform
 }
 
-TEST(TestAllocatorCommon, get_void_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);