From 46da8348a880d8701f66ee49eb06169b0064f8fc Mon Sep 17 00:00:00 2001 From: Steve Wolter Date: Tue, 22 Sep 2020 08:18:37 +0000 Subject: [PATCH 1/3] 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 Addressed code style test failures. Signed-off-by: Steve Wolter 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 Also update recently added tests. Signed-off-by: Steve Wolter Added reference to bug number. Signed-off-by: Steve Wolter Extend allocator lifetime in options. Signed-off-by: Steve Wolter Drop the generic version of get_rcl_allocator. This allows us to simplify a bunch of code as well. Signed-off-by: Steve Wolter Update rclcpp/include/rclcpp/allocator/allocator_common.hpp Co-authored-by: William Woodall Signed-off-by: Steve Wolter Revert accidental deletion of allocator creation. Signed-off-by: Steve Wolter --- .../rclcpp/allocator/allocator_common.hpp | 74 +++-------------- .../rclcpp/message_memory_strategy.hpp | 5 +- rclcpp/include/rclcpp/publisher_options.hpp | 9 ++- .../strategies/allocator_memory_strategy.hpp | 2 +- .../include/rclcpp/subscription_options.hpp | 9 ++- .../allocator/test_allocator_common.cpp | 79 +++---------------- 6 files changed, 37 insertions(+), 141 deletions(-) diff --git a/rclcpp/include/rclcpp/allocator/allocator_common.hpp b/rclcpp/include/rclcpp/allocator/allocator_common.hpp index 12b2f383b6..f1d411d25f 100644 --- a/rclcpp/include/rclcpp/allocator/allocator_common.hpp +++ b/rclcpp/include/rclcpp/allocator/allocator_common.hpp @@ -30,14 +30,19 @@ namespace allocator template using AllocRebind = typename std::allocator_traits::template rebind_traits; -template -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 +rcl_allocator_t get_rcl_allocator(std::allocator allocator) { - auto typed_allocator = static_cast(untyped_allocator); - if (!typed_allocator) { - throw std::runtime_error("Received incorrect allocator type"); - } - return std::allocator_traits::allocate(*typed_allocator, size); + (void)allocator; // Remove warning + return rcl_get_default_allocator(); } template @@ -56,61 +61,6 @@ void * retyped_zero_allocate(size_t number_of_elem, size_t size_of_elem, void * return allocated_memory; } -template -void retyped_deallocate(void * untyped_pointer, void * untyped_allocator) -{ - auto typed_allocator = static_cast(untyped_allocator); - if (!typed_allocator) { - throw std::runtime_error("Received incorrect allocator type"); - } - auto typed_ptr = static_cast(untyped_pointer); - std::allocator_traits::deallocate(*typed_allocator, typed_ptr, 1); -} - -template -void * retyped_reallocate(void * untyped_pointer, size_t size, void * untyped_allocator) -{ - auto typed_allocator = static_cast(untyped_allocator); - if (!typed_allocator) { - throw std::runtime_error("Received incorrect allocator type"); - } - auto typed_ptr = static_cast(untyped_pointer); - std::allocator_traits::deallocate(*typed_allocator, typed_ptr, 1); - return std::allocator_traits::allocate(*typed_allocator, size); -} - - -// Convert a std::allocator_traits-formatted Allocator into an rcl allocator -template< - typename T, - typename Alloc, - typename std::enable_if>::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; - rcl_allocator.zero_allocate = &retyped_zero_allocate; - rcl_allocator.deallocate = &retyped_deallocate; - rcl_allocator.reallocate = &retyped_reallocate; - rcl_allocator.state = &allocator; -#else - (void)allocator; // Remove warning -#endif - return rcl_allocator; -} - -// TODO(jacquelinekay) Workaround for an incomplete implementation of std::allocator -template< - typename T, - typename Alloc, - typename std::enable_if>::value>::type * = nullptr> -rcl_allocator_t get_rcl_allocator(Alloc & allocator) -{ - (void)allocator; - return rcl_get_default_allocator(); -} - } // namespace allocator } // namespace rclcpp diff --git a/rclcpp/include/rclcpp/message_memory_strategy.hpp b/rclcpp/include/rclcpp/message_memory_strategy.hpp index f548d953c2..d664239b25 100644 --- a/rclcpp/include/rclcpp/message_memory_strategy.hpp +++ b/rclcpp/include/rclcpp/message_memory_strategy.hpp @@ -18,6 +18,7 @@ #include #include +#include "rcl/allocator.h" #include "rcl/types.h" #include "rclcpp/allocator/allocator_common.hpp" @@ -61,7 +62,7 @@ class MessageMemoryStrategy message_allocator_ = std::make_shared(); serialized_message_allocator_ = std::make_shared(); buffer_allocator_ = std::make_shared(); - rcutils_allocator_ = allocator::get_rcl_allocator(*buffer_allocator_.get()); + rcutils_allocator_ = rcl_get_default_allocator(); } explicit MessageMemoryStrategy(std::shared_ptr allocator) @@ -69,7 +70,7 @@ class MessageMemoryStrategy message_allocator_ = std::make_shared(*allocator.get()); serialized_message_allocator_ = std::make_shared(*allocator.get()); buffer_allocator_ = std::make_shared(*allocator.get()); - rcutils_allocator_ = allocator::get_rcl_allocator(*buffer_allocator_.get()); + rcutils_allocator_ = rcl_get_default_allocator(); } virtual ~MessageMemoryStrategy() = default; diff --git a/rclcpp/include/rclcpp/publisher_options.hpp b/rclcpp/include/rclcpp/publisher_options.hpp index 01fd314f49..7b741809ea 100644 --- a/rclcpp/include/rclcpp/publisher_options.hpp +++ b/rclcpp/include/rclcpp/publisher_options.hpp @@ -20,6 +20,7 @@ #include #include +#include "rcl/allocator.h" #include "rcl/publisher.h" #include "rclcpp/allocator/allocator_common.hpp" @@ -81,7 +82,8 @@ 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 @@ -89,7 +91,7 @@ struct PublisherOptionsWithAllocator : public PublisherOptionsBase 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; @@ -102,7 +104,6 @@ struct PublisherOptionsWithAllocator : public PublisherOptionsBase return result; } - /// Get the allocator, creating one if needed. std::shared_ptr get_allocator() const @@ -127,7 +128,7 @@ struct PublisherOptionsWithAllocator : public PublisherOptionsBase plain_allocator_storage_ = std::make_shared(*this->get_allocator()); } - return rclcpp::allocator::get_rcl_allocator(*plain_allocator_storage_); + return rcl_get_default_allocator(); } // This is a temporal workaround, to make sure that get_allocator() diff --git a/rclcpp/include/rclcpp/strategies/allocator_memory_strategy.hpp b/rclcpp/include/rclcpp/strategies/allocator_memory_strategy.hpp index d3dcfab21c..c55d78cceb 100644 --- a/rclcpp/include/rclcpp/strategies/allocator_memory_strategy.hpp +++ b/rclcpp/include/rclcpp/strategies/allocator_memory_strategy.hpp @@ -515,7 +515,7 @@ class AllocatorMemoryStrategy : public memory_strategy::MemoryStrategy rcl_allocator_t get_allocator() override { - return rclcpp::allocator::get_rcl_allocator(*allocator_.get()); + return rcl_get_default_allocator(); } size_t number_of_ready_subscriptions() const override diff --git a/rclcpp/include/rclcpp/subscription_options.hpp b/rclcpp/include/rclcpp/subscription_options.hpp index 0dd738ee60..513b15d999 100644 --- a/rclcpp/include/rclcpp/subscription_options.hpp +++ b/rclcpp/include/rclcpp/subscription_options.hpp @@ -21,6 +21,8 @@ #include #include +#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" @@ -108,7 +110,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. /** @@ -119,7 +122,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 = @@ -171,7 +174,7 @@ struct SubscriptionOptionsWithAllocator : public SubscriptionOptionsBase plain_allocator_storage_ = std::make_shared(*this->get_allocator()); } - return rclcpp::allocator::get_rcl_allocator(*plain_allocator_storage_); + return rcl_get_default_allocator(); } // This is a temporal workaround, to make sure that get_allocator() diff --git a/rclcpp/test/rclcpp/allocator/test_allocator_common.cpp b/rclcpp/test/rclcpp/allocator/test_allocator_common.cpp index 4619b7665d..5e1d8bc90c 100644 --- a/rclcpp/test/rclcpp/allocator/test_allocator_common.cpp +++ b/rclcpp/test/rclcpp/allocator/test_allocator_common.cpp @@ -18,54 +18,17 @@ #include "rclcpp/allocator/allocator_common.hpp" -TEST(TestAllocatorCommon, retyped_allocate) { - std::allocator allocator; - void * untyped_allocator = &allocator; - void * allocated_mem = - rclcpp::allocator::retyped_allocate>(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>( - 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>( - 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>( - reallocated_mem, untyped_allocator); - }; - EXPECT_NO_THROW(code2()); -} - -TEST(TestAllocatorCommon, retyped_zero_allocate_basic) { +TEST(TestAllocatorCommon, retyped_zero_allocate_basic) +{ std::allocator allocator; void * untyped_allocator = &allocator; void * allocated_mem = rclcpp::allocator::retyped_zero_allocate>(20u, 1u, untyped_allocator); ASSERT_TRUE(nullptr != allocated_mem); - - auto code = [&untyped_allocator, allocated_mem]() { - rclcpp::allocator::retyped_deallocate>( - allocated_mem, untyped_allocator); - }; - EXPECT_NO_THROW(code()); } -TEST(TestAllocatorCommon, retyped_zero_allocate) { +TEST(TestAllocatorCommon, retyped_zero_allocate) +{ std::allocator allocator; void * untyped_allocator = &allocator; void * allocated_mem = @@ -73,34 +36,12 @@ TEST(TestAllocatorCommon, retyped_zero_allocate) { // 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>( - 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>( - 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>( - reallocated_mem, untyped_allocator); - }; - EXPECT_NO_THROW(code2()); } -TEST(TestAllocatorCommon, get_rcl_allocator) { +TEST(TestAllocatorCommon, get_rcl_allocator) +{ std::allocator allocator; - auto rcl_allocator = rclcpp::allocator::get_rcl_allocator(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); @@ -108,10 +49,10 @@ TEST(TestAllocatorCommon, get_rcl_allocator) { // 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 allocator; - auto rcl_allocator = - rclcpp::allocator::get_rcl_allocator>(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); From aab8c9ed7bd9646e26a6e0321abb990adaa6fdf2 Mon Sep 17 00:00:00 2001 From: Janosch Machowinski Date: Fri, 6 Feb 2026 18:34:00 +0100 Subject: [PATCH 2/3] fix: Remove now dead plain_allocator_storage_ Signed-off-by: Janosch Machowinski --- rclcpp/include/rclcpp/publisher_options.hpp | 35 ++++++------------- .../include/rclcpp/subscription_options.hpp | 32 +++++------------ rclcpp/test/rclcpp/test_node_options.cpp | 2 +- 3 files changed, 21 insertions(+), 48 deletions(-) diff --git a/rclcpp/include/rclcpp/publisher_options.hpp b/rclcpp/include/rclcpp/publisher_options.hpp index 7b741809ea..9c85832683 100644 --- a/rclcpp/include/rclcpp/publisher_options.hpp +++ b/rclcpp/include/rclcpp/publisher_options.hpp @@ -75,13 +75,17 @@ struct PublisherOptionsWithAllocator : public PublisherOptionsBase "Publisher allocator value type must be void"); /// Optional custom allocator. - std::shared_ptr allocator = nullptr; + std::shared_ptr allocator; + std::shared_ptr rcl_allocator; - PublisherOptionsWithAllocator() {} + PublisherOptionsWithAllocator() + :allocator(std::make_shared()) + {} /// Constructor using base class as input. explicit PublisherOptionsWithAllocator(const PublisherOptionsBase & publisher_options_base) - : PublisherOptionsBase(publisher_options_base) + : PublisherOptionsBase(publisher_options_base), + allocator(std::make_shared()) { } @@ -91,7 +95,7 @@ struct PublisherOptionsWithAllocator : public PublisherOptionsBase to_rcl_publisher_options(const rclcpp::QoS & qos) const { rcl_publisher_options_t result = rcl_publisher_get_default_options(); - result.allocator = rcl_get_default_allocator(); + result.allocator = get_rcl_allocator(); result.qos = qos.get_rmw_qos_profile(); result.rmw_publisher_options.require_unique_network_flow_endpoints = this->require_unique_network_flow_endpoints; @@ -108,38 +112,21 @@ struct PublisherOptionsWithAllocator : public PublisherOptionsBase std::shared_ptr get_allocator() const { - if (!this->allocator) { - if (!allocator_storage_) { - allocator_storage_ = std::make_shared(); - } - return allocator_storage_; - } return this->allocator; } private: - using PlainAllocator = - typename std::allocator_traits::template rebind_alloc; - rcl_allocator_t get_rcl_allocator() const { - if (!plain_allocator_storage_) { - plain_allocator_storage_ = - std::make_shared(*this->get_allocator()); + if(rcl_allocator) { + return *rcl_allocator; } return rcl_get_default_allocator(); } - - // This is a temporal workaround, to make sure that get_allocator() - // always returns a copy of the same allocator. - mutable std::shared_ptr allocator_storage_; - - // This is a temporal workaround, to keep the plain allocator that backs - // up the rcl allocator returned in rcl_publisher_options_t alive. - mutable std::shared_ptr plain_allocator_storage_; }; + using PublisherOptions = PublisherOptionsWithAllocator>; } // namespace rclcpp diff --git a/rclcpp/include/rclcpp/subscription_options.hpp b/rclcpp/include/rclcpp/subscription_options.hpp index 513b15d999..70701aec93 100644 --- a/rclcpp/include/rclcpp/subscription_options.hpp +++ b/rclcpp/include/rclcpp/subscription_options.hpp @@ -102,14 +102,18 @@ struct SubscriptionOptionsWithAllocator : public SubscriptionOptionsBase "Subscription allocator value type must be void"); /// Optional custom allocator. - std::shared_ptr allocator = nullptr; + std::shared_ptr allocator; + std::shared_ptr rcl_allocator; - SubscriptionOptionsWithAllocator() {} + SubscriptionOptionsWithAllocator() + :allocator(std::make_shared()) + {} /// Constructor using base class as input. explicit SubscriptionOptionsWithAllocator( const SubscriptionOptionsBase & subscription_options_base) - : SubscriptionOptionsBase(subscription_options_base) + : SubscriptionOptionsBase(subscription_options_base), + allocator(std::make_shared()) { } @@ -154,36 +158,18 @@ struct SubscriptionOptionsWithAllocator : public SubscriptionOptionsBase std::shared_ptr get_allocator() const { - if (!this->allocator) { - if (!allocator_storage_) { - allocator_storage_ = std::make_shared(); - } - return allocator_storage_; - } return this->allocator; } private: - using PlainAllocator = - typename std::allocator_traits::template rebind_alloc; - rcl_allocator_t get_rcl_allocator() const { - if (!plain_allocator_storage_) { - plain_allocator_storage_ = - std::make_shared(*this->get_allocator()); + if(rcl_allocator) { + return *rcl_allocator; } return rcl_get_default_allocator(); } - - // This is a temporal workaround, to make sure that get_allocator() - // always returns a copy of the same allocator. - mutable std::shared_ptr allocator_storage_; - - // This is a temporal workaround, to keep the plain allocator that backs - // up the rcl allocator returned in rcl_subscription_options_t alive. - mutable std::shared_ptr plain_allocator_storage_; }; using SubscriptionOptions = SubscriptionOptionsWithAllocator>; diff --git a/rclcpp/test/rclcpp/test_node_options.cpp b/rclcpp/test/rclcpp/test_node_options.cpp index 07a522f497..14e32f2304 100644 --- a/rclcpp/test/rclcpp/test_node_options.cpp +++ b/rclcpp/test/rclcpp/test_node_options.cpp @@ -372,7 +372,7 @@ TEST(TestNodeOptions, set_get_allocator) { EXPECT_EQ(fake_allocator.state, options.allocator().state); // Check invalid allocator - EXPECT_THROW(options.get_rcl_node_options(), std::bad_alloc); + EXPECT_THROW(options.get_rcl_node_options(), rclcpp::exceptions::RCLBadAlloc); } TEST(TestNodeOptions, clock_type) { From 80ffafa018ea873a807791858abeed22e7c08a35 Mon Sep 17 00:00:00 2001 From: Janosch Machowinski Date: Wed, 11 Feb 2026 13:30:20 +0100 Subject: [PATCH 3/3] feat: experimented with automatic warnings for users of custom allocators --- rclcpp/include/rclcpp/publisher_options.hpp | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/rclcpp/include/rclcpp/publisher_options.hpp b/rclcpp/include/rclcpp/publisher_options.hpp index 9c85832683..8a9bb4ea00 100644 --- a/rclcpp/include/rclcpp/publisher_options.hpp +++ b/rclcpp/include/rclcpp/publisher_options.hpp @@ -122,10 +122,22 @@ struct PublisherOptionsWithAllocator : public PublisherOptionsBase if(rcl_allocator) { return *rcl_allocator; } + + RCUTILS_LOG_ERROR_NAMED("rclcpp", + "Warning, custom C++ allocator specified, but custom rcl allocator was set. Note, " \ + "that due to a bug fix rcl allocators are not automatically generated from the C++ " \ + "allocator any more."); + return rcl_get_default_allocator(); } }; +template<> +inline rcl_allocator_t +PublisherOptionsWithAllocator>::get_rcl_allocator() const +{ + return rcl_get_default_allocator(); +} using PublisherOptions = PublisherOptionsWithAllocator>;