From f0f44ed2626403ec826f382fa54afe570a1daf6d Mon Sep 17 00:00:00 2001 From: William Woodall Date: Tue, 27 Apr 2021 13:12:56 -0700 Subject: [PATCH 1/7] use dynamic_pointer_cast to detect allocator mismatch in intra process manager Signed-off-by: William Woodall --- .../rclcpp/experimental/intra_process_manager.hpp | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/rclcpp/include/rclcpp/experimental/intra_process_manager.hpp b/rclcpp/include/rclcpp/experimental/intra_process_manager.hpp index 9d4b62654c..cd703d5759 100644 --- a/rclcpp/include/rclcpp/experimental/intra_process_manager.hpp +++ b/rclcpp/include/rclcpp/experimental/intra_process_manager.hpp @@ -363,9 +363,13 @@ class IntraProcessManager } auto subscription_base = subscription_it->second.subscription.lock(); if (subscription_base) { - auto subscription = std::static_pointer_cast< + auto subscription = std::dynamic_pointer_cast< rclcpp::experimental::SubscriptionIntraProcess >(subscription_base); + if (nullptr == subscription) { + throw std::runtime_error( + "failed to dynamic cast SubscriptionIntraProcessBase to SubscriptionIntraProcess"); + } subscription->provide_intra_process_message(message); } else { @@ -394,9 +398,13 @@ class IntraProcessManager } auto subscription_base = subscription_it->second.subscription.lock(); if (subscription_base) { - auto subscription = std::static_pointer_cast< + auto subscription = std::dynamic_pointer_cast< rclcpp::experimental::SubscriptionIntraProcess >(subscription_base); + if (nullptr == subscription) { + throw std::runtime_error( + "failed to dynamic cast SubscriptionIntraProcessBase to SubscriptionIntraProcess"); + } if (std::next(it) == subscription_ids.end()) { // If this is the last subscription, give up ownership From 5c539d1e4a6a14cf10f67733517b9ad8d032061c Mon Sep 17 00:00:00 2001 From: William Woodall Date: Tue, 27 Apr 2021 14:08:23 -0700 Subject: [PATCH 2/7] add test case for mismatched allocators Signed-off-by: William Woodall --- .../experimental/intra_process_manager.hpp | 8 +- rclcpp/test/rclcpp/CMakeLists.txt | 7 + ..._intra_process_manager_with_allocators.cpp | 218 ++++++++++++++++++ 3 files changed, 231 insertions(+), 2 deletions(-) create mode 100644 rclcpp/test/rclcpp/test_intra_process_manager_with_allocators.cpp diff --git a/rclcpp/include/rclcpp/experimental/intra_process_manager.hpp b/rclcpp/include/rclcpp/experimental/intra_process_manager.hpp index cd703d5759..cae722acaf 100644 --- a/rclcpp/include/rclcpp/experimental/intra_process_manager.hpp +++ b/rclcpp/include/rclcpp/experimental/intra_process_manager.hpp @@ -368,7 +368,9 @@ class IntraProcessManager >(subscription_base); if (nullptr == subscription) { throw std::runtime_error( - "failed to dynamic cast SubscriptionIntraProcessBase to SubscriptionIntraProcess"); + "failed to dynamic cast SubscriptionIntraProcessBase to " + "SubscriptionIntraProcess, which can happen when the publisher " + "and subscription use different allocator types, which is not supported"); } subscription->provide_intra_process_message(message); @@ -403,7 +405,9 @@ class IntraProcessManager >(subscription_base); if (nullptr == subscription) { throw std::runtime_error( - "failed to dynamic cast SubscriptionIntraProcessBase to SubscriptionIntraProcess"); + "failed to dynamic cast SubscriptionIntraProcessBase to " + "SubscriptionIntraProcess, which can happen when the publisher " + "and subscription use different allocator types, which is not supported"); } if (std::next(it) == subscription_ids.end()) { diff --git a/rclcpp/test/rclcpp/CMakeLists.txt b/rclcpp/test/rclcpp/CMakeLists.txt index 010a911889..f5206c1d05 100644 --- a/rclcpp/test/rclcpp/CMakeLists.txt +++ b/rclcpp/test/rclcpp/CMakeLists.txt @@ -143,6 +143,13 @@ 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 diff --git a/rclcpp/test/rclcpp/test_intra_process_manager_with_allocators.cpp b/rclcpp/test/rclcpp/test_intra_process_manager_with_allocators.cpp new file mode 100644 index 0000000000..398efcdc7f --- /dev/null +++ b/rclcpp/test/rclcpp/test_intra_process_manager_with_allocators.cpp @@ -0,0 +1,218 @@ +// Copyright 2015 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 + +#include +#include +#include +#include +#include + +#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 +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::difference_type; + + MyAllocator() noexcept + { + } + + ~MyAllocator() noexcept {} + + template + MyAllocator(const MyAllocator &) noexcept + { + } + + T * allocate(size_t size, const void * = 0) + { + if (size == 0) { + return nullptr; + } + num_allocs++; + return static_cast(std::malloc(size * sizeof(T))); + } + + void deallocate(T * ptr, size_t size) + { + (void)size; + if (!ptr) { + return; + } + num_deallocs++; + std::free(ptr); + } + + template + struct rebind + { + typedef MyAllocator other; + }; +}; + +template +constexpr bool operator==( + const MyAllocator &, + const MyAllocator &) noexcept +{ + return true; +} + +template +constexpr bool operator!=( + const MyAllocator &, + const MyAllocator &) noexcept +{ + return false; +} + +/* + This tests the case where a custom allocator is used correctly, i.e. the same + custom allocator on both sides. + */ +TEST(TestIntraProcessManagerWithAllocators, custom_allocator) { + auto context = std::make_shared(); + context->init(0, nullptr); + auto node = std::make_shared( + "custom_allocator_test", + rclcpp::NodeOptions().context(context).use_intra_process_comms(true)); + + uint32_t counter = 0; + auto callback = + [&counter](std::shared_ptr) { + ++counter; + }; + + using Alloc = MyAllocator; + auto alloc = std::make_shared(); + rclcpp::PublisherOptionsWithAllocator publisher_options; + publisher_options.allocator = alloc; + auto publisher = + node->create_publisher("custom_allocator_test", 10, publisher_options); + + rclcpp::SubscriptionOptionsWithAllocator subscription_options; + subscription_options.allocator = alloc; + auto msg_mem_strat = + std::make_shared< + rclcpp::message_memory_strategy::MessageMemoryStrategy + >(alloc); + auto subscriber = node->create_subscription( + "custom_allocator_test", 10, callback, subscription_options, msg_mem_strat); + + using rclcpp::memory_strategies::allocator_memory_strategy::AllocatorMemoryStrategy; + std::shared_ptr memory_strategy = + std::make_shared>(alloc); + + rclcpp::ExecutorOptions options; + options.memory_strategy = memory_strategy; + options.context = context; + rclcpp::executors::SingleThreadedExecutor executor(options); + + executor.add_node(node); + + using MessageAllocTraits = + rclcpp::allocator::AllocRebind; + using MessageAlloc = MessageAllocTraits::allocator_type; + using MessageDeleter = rclcpp::allocator::Deleter; + MessageDeleter message_deleter; + MessageAlloc message_alloc = *alloc; + rclcpp::allocator::set_allocator_for_deleter(&message_deleter, &message_alloc); + + auto ptr = MessageAllocTraits::allocate(message_alloc, 1); + MessageAllocTraits::construct(message_alloc, ptr); + std::unique_ptr msg(ptr, message_deleter); + EXPECT_NO_THROW({ + publisher->publish(std::move(msg)); + executor.spin_some(); + }); +} + +/* + This tests the case where a custom allocator is used incorrectly, i.e. different + custom allocators on both sides. + */ +TEST(TestIntraProcessManagerWithAllocators, custom_allocator_wrong) { + auto context = std::make_shared(); + context->init(0, nullptr); + auto node = std::make_shared( + "custom_allocator_test", + rclcpp::NodeOptions().context(context).use_intra_process_comms(true)); + + uint32_t counter = 0; + auto callback = + [&counter](std::shared_ptr) { + ++counter; + }; + + using Alloc = MyAllocator; + auto alloc = std::make_shared(); + rclcpp::PublisherOptionsWithAllocator publisher_options; + publisher_options.allocator = alloc; + auto publisher = + node->create_publisher("custom_allocator_test", 10, publisher_options); + + // Explicitly use std::allocator + rclcpp::SubscriptionOptionsWithAllocator> subscription_options; + auto std_alloc = std::make_shared>(); + subscription_options.allocator = std_alloc; + auto msg_mem_strat = + std::make_shared< + rclcpp::message_memory_strategy::MessageMemoryStrategy> + >(std_alloc); + auto subscriber = node->create_subscription( + "custom_allocator_test", 10, callback, subscription_options, msg_mem_strat); + + using rclcpp::memory_strategies::allocator_memory_strategy::AllocatorMemoryStrategy; + std::shared_ptr memory_strategy = + std::make_shared>(alloc); + + rclcpp::ExecutorOptions options; + options.memory_strategy = memory_strategy; + options.context = context; + rclcpp::executors::SingleThreadedExecutor executor(options); + + executor.add_node(node); + + using MessageAllocTraits = + rclcpp::allocator::AllocRebind; + using MessageAlloc = MessageAllocTraits::allocator_type; + using MessageDeleter = rclcpp::allocator::Deleter; + MessageDeleter message_deleter; + MessageAlloc message_alloc = *alloc; + rclcpp::allocator::set_allocator_for_deleter(&message_deleter, &message_alloc); + + auto ptr = MessageAllocTraits::allocate(message_alloc, 1); + MessageAllocTraits::construct(message_alloc, ptr); + std::unique_ptr msg(ptr, message_deleter); + EXPECT_THROW({ + publisher->publish(std::move(msg)); + executor.spin_some(); + }, std::runtime_error); +} From 490cca76b96b3588b637dcb53c27b3cc0a1615c0 Mon Sep 17 00:00:00 2001 From: William Woodall Date: Tue, 27 Apr 2021 17:02:52 -0700 Subject: [PATCH 3/7] forward template arguments to avoid mismatched types in intra process manager Signed-off-by: William Woodall --- .../experimental/intra_process_manager.hpp | 18 +++++++++++------- .../test/rclcpp/test_intra_process_manager.cpp | 5 ++++- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/rclcpp/include/rclcpp/experimental/intra_process_manager.hpp b/rclcpp/include/rclcpp/experimental/intra_process_manager.hpp index cae722acaf..38aaec3c1d 100644 --- a/rclcpp/include/rclcpp/experimental/intra_process_manager.hpp +++ b/rclcpp/include/rclcpp/experimental/intra_process_manager.hpp @@ -202,7 +202,8 @@ class IntraProcessManager // None of the buffers require ownership, so we promote the pointer std::shared_ptr msg = std::move(message); - this->template add_shared_msg_to_buffers(msg, sub_ids.take_shared_subscriptions); + this->template add_shared_msg_to_buffers( + msg, sub_ids.take_shared_subscriptions); } else if (!sub_ids.take_ownership_subscriptions.empty() && // NOLINT sub_ids.take_shared_subscriptions.size() <= 1) { @@ -227,7 +228,7 @@ class IntraProcessManager // for the buffers that do not require ownership auto shared_msg = std::allocate_shared(*allocator, *message); - this->template add_shared_msg_to_buffers( + this->template add_shared_msg_to_buffers( shared_msg, sub_ids.take_shared_subscriptions); this->template add_owned_msg_to_buffers( std::move(message), sub_ids.take_ownership_subscriptions, allocator); @@ -263,7 +264,7 @@ class IntraProcessManager // If there are no owning, just convert to shared. std::shared_ptr shared_msg = std::move(message); if (!sub_ids.take_shared_subscriptions.empty()) { - this->template add_shared_msg_to_buffers( + this->template add_shared_msg_to_buffers( shared_msg, sub_ids.take_shared_subscriptions); } return shared_msg; @@ -273,7 +274,7 @@ class IntraProcessManager auto shared_msg = std::allocate_shared(*allocator, *message); if (!sub_ids.take_shared_subscriptions.empty()) { - this->template add_shared_msg_to_buffers( + this->template add_shared_msg_to_buffers( shared_msg, sub_ids.take_shared_subscriptions); } @@ -350,7 +351,10 @@ class IntraProcessManager bool can_communicate(PublisherInfo pub_info, SubscriptionInfo sub_info) const; - template + template< + typename MessageT, + typename Alloc, + typename Deleter> void add_shared_msg_to_buffers( std::shared_ptr message, @@ -364,7 +368,7 @@ class IntraProcessManager auto subscription_base = subscription_it->second.subscription.lock(); if (subscription_base) { auto subscription = std::dynamic_pointer_cast< - rclcpp::experimental::SubscriptionIntraProcess + rclcpp::experimental::SubscriptionIntraProcess >(subscription_base); if (nullptr == subscription) { throw std::runtime_error( @@ -401,7 +405,7 @@ class IntraProcessManager auto subscription_base = subscription_it->second.subscription.lock(); if (subscription_base) { auto subscription = std::dynamic_pointer_cast< - rclcpp::experimental::SubscriptionIntraProcess + rclcpp::experimental::SubscriptionIntraProcess >(subscription_base); if (nullptr == subscription) { throw std::runtime_error( diff --git a/rclcpp/test/rclcpp/test_intra_process_manager.cpp b/rclcpp/test/rclcpp/test_intra_process_manager.cpp index b71fb1d061..ec7ccc7e5a 100644 --- a/rclcpp/test/rclcpp/test_intra_process_manager.cpp +++ b/rclcpp/test/rclcpp/test_intra_process_manager.cpp @@ -216,7 +216,10 @@ class SubscriptionIntraProcessBase const char * topic_name; }; -template +template< + typename MessageT, + typename Alloc = std::allocator, + typename Deleter = std::default_delete> class SubscriptionIntraProcess : public SubscriptionIntraProcessBase { public: From 144da644076a219cfa0cc1acf35546ae605472a4 Mon Sep 17 00:00:00 2001 From: William Woodall Date: Tue, 27 Apr 2021 17:03:44 -0700 Subject: [PATCH 4/7] style fixes Signed-off-by: William Woodall --- .../rclcpp/experimental/intra_process_manager.hpp | 14 ++++++++------ .../test_intra_process_manager_with_allocators.cpp | 11 +++++++---- 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/rclcpp/include/rclcpp/experimental/intra_process_manager.hpp b/rclcpp/include/rclcpp/experimental/intra_process_manager.hpp index 38aaec3c1d..f3990cb76b 100644 --- a/rclcpp/include/rclcpp/experimental/intra_process_manager.hpp +++ b/rclcpp/include/rclcpp/experimental/intra_process_manager.hpp @@ -372,9 +372,10 @@ class IntraProcessManager >(subscription_base); if (nullptr == subscription) { throw std::runtime_error( - "failed to dynamic cast SubscriptionIntraProcessBase to " - "SubscriptionIntraProcess, which can happen when the publisher " - "and subscription use different allocator types, which is not supported"); + "failed to dynamic cast SubscriptionIntraProcessBase to " + "SubscriptionIntraProcess, which " + "can happen when the publisher and subscription use different " + "allocator types, which is not supported"); } subscription->provide_intra_process_message(message); @@ -409,9 +410,10 @@ class IntraProcessManager >(subscription_base); if (nullptr == subscription) { throw std::runtime_error( - "failed to dynamic cast SubscriptionIntraProcessBase to " - "SubscriptionIntraProcess, which can happen when the publisher " - "and subscription use different allocator types, which is not supported"); + "failed to dynamic cast SubscriptionIntraProcessBase to " + "SubscriptionIntraProcess, which " + "can happen when the publisher and subscription use different " + "allocator types, which is not supported"); } if (std::next(it) == subscription_ids.end()) { diff --git a/rclcpp/test/rclcpp/test_intra_process_manager_with_allocators.cpp b/rclcpp/test/rclcpp/test_intra_process_manager_with_allocators.cpp index 398efcdc7f..ea60dcea21 100644 --- a/rclcpp/test/rclcpp/test_intra_process_manager_with_allocators.cpp +++ b/rclcpp/test/rclcpp/test_intra_process_manager_with_allocators.cpp @@ -121,7 +121,7 @@ TEST(TestIntraProcessManagerWithAllocators, custom_allocator) { subscription_options.allocator = alloc; auto msg_mem_strat = std::make_shared< - rclcpp::message_memory_strategy::MessageMemoryStrategy + rclcpp::message_memory_strategy::MessageMemoryStrategy >(alloc); auto subscriber = node->create_subscription( "custom_allocator_test", 10, callback, subscription_options, msg_mem_strat); @@ -148,7 +148,8 @@ TEST(TestIntraProcessManagerWithAllocators, custom_allocator) { auto ptr = MessageAllocTraits::allocate(message_alloc, 1); MessageAllocTraits::construct(message_alloc, ptr); std::unique_ptr msg(ptr, message_deleter); - EXPECT_NO_THROW({ + EXPECT_NO_THROW( + { publisher->publish(std::move(msg)); executor.spin_some(); }); @@ -184,7 +185,8 @@ TEST(TestIntraProcessManagerWithAllocators, custom_allocator_wrong) { subscription_options.allocator = std_alloc; auto msg_mem_strat = std::make_shared< - rclcpp::message_memory_strategy::MessageMemoryStrategy> + rclcpp::message_memory_strategy::MessageMemoryStrategy> >(std_alloc); auto subscriber = node->create_subscription( "custom_allocator_test", 10, callback, subscription_options, msg_mem_strat); @@ -211,7 +213,8 @@ TEST(TestIntraProcessManagerWithAllocators, custom_allocator_wrong) { auto ptr = MessageAllocTraits::allocate(message_alloc, 1); MessageAllocTraits::construct(message_alloc, ptr); std::unique_ptr msg(ptr, message_deleter); - EXPECT_THROW({ + EXPECT_THROW( + { publisher->publish(std::move(msg)); executor.spin_some(); }, std::runtime_error); From 060526dd91e0bed7f629fb6260eb8f6c795210fa Mon Sep 17 00:00:00 2001 From: William Woodall Date: Wed, 28 Apr 2021 14:53:37 -0700 Subject: [PATCH 5/7] refactor to test message address and count, more DRY Signed-off-by: William Woodall --- ..._intra_process_manager_with_allocators.cpp | 247 +++++++++++------- 1 file changed, 151 insertions(+), 96 deletions(-) diff --git a/rclcpp/test/rclcpp/test_intra_process_manager_with_allocators.cpp b/rclcpp/test/rclcpp/test_intra_process_manager_with_allocators.cpp index ea60dcea21..3bd5fe687c 100644 --- a/rclcpp/test/rclcpp/test_intra_process_manager_with_allocators.cpp +++ b/rclcpp/test/rclcpp/test_intra_process_manager_with_allocators.cpp @@ -93,43 +93,103 @@ constexpr bool operator!=( return false; } -/* - This tests the case where a custom allocator is used correctly, i.e. the same - custom allocator on both sides. - */ -TEST(TestIntraProcessManagerWithAllocators, custom_allocator) { +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 */, // intentinally unused + SubscriptionAllocatorT subscription_allocator, + MessageMemoryStrategyAllocatorT message_memory_strategy, + MemoryStrategyAllocatorT memory_strategy_allocator) +{ + using PublishedMessageAllocTraits = + rclcpp::allocator::AllocRebind; + using PublishedMessageAlloc = typename PublishedMessageAllocTraits::allocator_type; + using PublishedMessageDeleter = + rclcpp::allocator::Deleter; + + using SubscribedMessageAllocTraits = + rclcpp::allocator::AllocRebind; + using SubscribedMessageAlloc = typename SubscribedMessageAllocTraits::allocator_type; + using SubscribedMessageDeleter = + rclcpp::allocator::Deleter; + + // init and node auto context = std::make_shared(); context->init(0, nullptr); auto node = std::make_shared( "custom_allocator_test", rclcpp::NodeOptions().context(context).use_intra_process_comms(true)); + // publisher + auto shared_publisher_allocator = std::make_shared(publisher_allocator); + rclcpp::PublisherOptionsWithAllocator publisher_options; + publisher_options.allocator = shared_publisher_allocator; + auto publisher = + node->create_publisher("custom_allocator_test", 10, publisher_options); + + // callback for subscription uint32_t counter = 0; + std::promise> received_message; + auto received_message_future = received_message.get_future(); auto callback = - [&counter](std::shared_ptr) { + [&counter, &received_message]( + std::unique_ptr msg) + { ++counter; + received_message.set_value(std::move(msg)); }; - using Alloc = MyAllocator; - auto alloc = std::make_shared(); - rclcpp::PublisherOptionsWithAllocator publisher_options; - publisher_options.allocator = alloc; - auto publisher = - node->create_publisher("custom_allocator_test", 10, publisher_options); - - rclcpp::SubscriptionOptionsWithAllocator subscription_options; - subscription_options.allocator = alloc; - auto msg_mem_strat = - std::make_shared< - rclcpp::message_memory_strategy::MessageMemoryStrategy - >(alloc); - auto subscriber = node->create_subscription( - "custom_allocator_test", 10, callback, subscription_options, msg_mem_strat); + // subscription + auto shared_subscription_allocator = + std::make_shared(subscription_allocator); + rclcpp::SubscriptionOptionsWithAllocator subscription_options; + subscription_options.allocator = shared_subscription_allocator; + auto shared_message_strategy_allocator = + std::make_shared(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::type; + auto subscriber = node->create_subscription< + test_msgs::msg::Empty, + decltype(callback), + SubscriptionAllocatorT, + CallbackMessageT, + rclcpp::Subscription, + rclcpp::message_memory_strategy::MessageMemoryStrategy< + CallbackMessageT, + MessageMemoryStrategyAllocatorT + > + >( + "custom_allocator_test", + 10, + std::forward(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( + memory_strategy_allocator); std::shared_ptr memory_strategy = - std::make_shared>(alloc); + std::make_shared>( + shared_memory_strategy_allocator); + // executor rclcpp::ExecutorOptions options; options.memory_strategy = memory_strategy; options.context = context; @@ -137,22 +197,57 @@ TEST(TestIntraProcessManagerWithAllocators, custom_allocator) { executor.add_node(node); - using MessageAllocTraits = - rclcpp::allocator::AllocRebind; - using MessageAlloc = MessageAllocTraits::allocator_type; - using MessageDeleter = rclcpp::allocator::Deleter; - MessageDeleter message_deleter; - MessageAlloc message_alloc = *alloc; - rclcpp::allocator::set_allocator_for_deleter(&message_deleter, &message_alloc); - - auto ptr = MessageAllocTraits::allocate(message_alloc, 1); - MessageAllocTraits::construct(message_alloc, ptr); - std::unique_ptr msg(ptr, message_deleter); - EXPECT_NO_THROW( - { - publisher->publish(std::move(msg)); - executor.spin_some(); - }); + // 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 msg(ptr, message_deleter); + + // publisher and receive + if constexpr (std::is_same_v) { + // 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; + using PublisherAllocatorT = std::allocator; + using SubscribedMessageAllocatorT = std::allocator; + using SubscriptionAllocatorT = std::allocator; + using MessageMemoryStrategyAllocatorT = std::allocator; + using MemoryStrategyAllocatorT = std::allocator; + auto allocator = std::allocator(); + do_custom_allocator_test< + PublishedMessageAllocatorT, + PublisherAllocatorT, + SubscribedMessageAllocatorT, + SubscriptionAllocatorT, + MessageMemoryStrategyAllocatorT, + MemoryStrategyAllocatorT, + void // no exception expected + >(allocator, allocator, allocator, allocator, allocator, allocator); } /* @@ -160,62 +255,22 @@ TEST(TestIntraProcessManagerWithAllocators, custom_allocator) { custom allocators on both sides. */ TEST(TestIntraProcessManagerWithAllocators, custom_allocator_wrong) { - auto context = std::make_shared(); - context->init(0, nullptr); - auto node = std::make_shared( - "custom_allocator_test", - rclcpp::NodeOptions().context(context).use_intra_process_comms(true)); - - uint32_t counter = 0; - auto callback = - [&counter](std::shared_ptr) { - ++counter; - }; - - using Alloc = MyAllocator; - auto alloc = std::make_shared(); - rclcpp::PublisherOptionsWithAllocator publisher_options; - publisher_options.allocator = alloc; - auto publisher = - node->create_publisher("custom_allocator_test", 10, publisher_options); - - // Explicitly use std::allocator - rclcpp::SubscriptionOptionsWithAllocator> subscription_options; - auto std_alloc = std::make_shared>(); - subscription_options.allocator = std_alloc; - auto msg_mem_strat = - std::make_shared< - rclcpp::message_memory_strategy::MessageMemoryStrategy> - >(std_alloc); - auto subscriber = node->create_subscription( - "custom_allocator_test", 10, callback, subscription_options, msg_mem_strat); - - using rclcpp::memory_strategies::allocator_memory_strategy::AllocatorMemoryStrategy; - std::shared_ptr memory_strategy = - std::make_shared>(alloc); - - rclcpp::ExecutorOptions options; - options.memory_strategy = memory_strategy; - options.context = context; - rclcpp::executors::SingleThreadedExecutor executor(options); - - executor.add_node(node); - - using MessageAllocTraits = - rclcpp::allocator::AllocRebind; - using MessageAlloc = MessageAllocTraits::allocator_type; - using MessageDeleter = rclcpp::allocator::Deleter; - MessageDeleter message_deleter; - MessageAlloc message_alloc = *alloc; - rclcpp::allocator::set_allocator_for_deleter(&message_deleter, &message_alloc); - - auto ptr = MessageAllocTraits::allocate(message_alloc, 1); - MessageAllocTraits::construct(message_alloc, ptr); - std::unique_ptr msg(ptr, message_deleter); - EXPECT_THROW( - { - publisher->publish(std::move(msg)); - executor.spin_some(); - }, std::runtime_error); + // explicitly use a different allocator here to provoke a failure + using PublishedMessageAllocatorT = std::allocator; + using PublisherAllocatorT = std::allocator; + using SubscribedMessageAllocatorT = MyAllocator; + using SubscriptionAllocatorT = MyAllocator; + using MessageMemoryStrategyAllocatorT = MyAllocator; + using MemoryStrategyAllocatorT = std::allocator; + auto allocator = std::allocator(); + auto my_allocator = MyAllocator(); + do_custom_allocator_test< + PublishedMessageAllocatorT, + PublisherAllocatorT, + SubscribedMessageAllocatorT, + SubscriptionAllocatorT, + MessageMemoryStrategyAllocatorT, + MemoryStrategyAllocatorT, + std::runtime_error // expected exception + >(allocator, allocator, my_allocator, my_allocator, my_allocator, allocator); } From b4a0aa1d13af489bc2eac2f0f1ec0deb4305d3be Mon Sep 17 00:00:00 2001 From: William Woodall Date: Wed, 28 Apr 2021 14:54:52 -0700 Subject: [PATCH 6/7] update copyright Signed-off-by: William Woodall --- .../test/rclcpp/test_intra_process_manager_with_allocators.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rclcpp/test/rclcpp/test_intra_process_manager_with_allocators.cpp b/rclcpp/test/rclcpp/test_intra_process_manager_with_allocators.cpp index 3bd5fe687c..6a0c156e3a 100644 --- a/rclcpp/test/rclcpp/test_intra_process_manager_with_allocators.cpp +++ b/rclcpp/test/rclcpp/test_intra_process_manager_with_allocators.cpp @@ -1,4 +1,4 @@ -// Copyright 2015 Open Source Robotics Foundation, Inc. +// 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. From a2b1d391b4de9abde44886984f0aa7469ce2e592 Mon Sep 17 00:00:00 2001 From: William Woodall Date: Thu, 29 Apr 2021 10:36:01 -0700 Subject: [PATCH 7/7] fix typo Signed-off-by: William Woodall Co-authored-by: Michel Hidalgo --- .../test/rclcpp/test_intra_process_manager_with_allocators.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rclcpp/test/rclcpp/test_intra_process_manager_with_allocators.cpp b/rclcpp/test/rclcpp/test_intra_process_manager_with_allocators.cpp index 6a0c156e3a..f8cb96a421 100644 --- a/rclcpp/test/rclcpp/test_intra_process_manager_with_allocators.cpp +++ b/rclcpp/test/rclcpp/test_intra_process_manager_with_allocators.cpp @@ -106,7 +106,7 @@ void do_custom_allocator_test( PublishedMessageAllocatorT published_message_allocator, PublisherAllocatorT publisher_allocator, - SubscribedMessageAllocatorT /* subscribed_message_allocator */, // intentinally unused + SubscribedMessageAllocatorT /* subscribed_message_allocator */, // intentionally unused SubscriptionAllocatorT subscription_allocator, MessageMemoryStrategyAllocatorT message_memory_strategy, MemoryStrategyAllocatorT memory_strategy_allocator)