From 8b69cbbe1b078753497b92a9ab931a0e2c169a49 Mon Sep 17 00:00:00 2001 From: Iker Luengo Date: Tue, 9 Feb 2021 11:37:48 +0100 Subject: [PATCH 1/5] Refs 10533 Fix singleton destructin order These singletons are used on the destruction of certain classes. In order to avoid these singletons to be already destructed, these classes keep a shared_pointer to an instance to the singleton. This ensures that the singleton is destructed AFTER the class instance. Signed-off-by: Iker Luengo --- .../domain/DomainParticipantFactory.cpp | 6 ++++ .../rtps/history/TopicPayloadPoolRegistry.cpp | 7 ++++- .../rtps/history/TopicPayloadPoolRegistry.hpp | 8 +++++ .../TopicPayloadPoolRegistry.hpp | 6 ++-- .../transport/shared_mem/SharedMemGlobal.hpp | 30 ++++++++++++------- .../transport/shared_mem/SharedMemManager.hpp | 30 ++++++++++++------- .../utils/shared_memory/SharedMemWatchdog.hpp | 20 ++++++------- .../dds/publisher/DataWriterTests.cpp | 19 ++++++++++++ .../dds/subscriber/DataReaderTests.cpp | 27 +++++++++++++++++ 9 files changed, 116 insertions(+), 37 deletions(-) diff --git a/src/cpp/fastdds/domain/DomainParticipantFactory.cpp b/src/cpp/fastdds/domain/DomainParticipantFactory.cpp index cee8311f5b5..3a120905b4a 100644 --- a/src/cpp/fastdds/domain/DomainParticipantFactory.cpp +++ b/src/cpp/fastdds/domain/DomainParticipantFactory.cpp @@ -32,6 +32,8 @@ #include #include +#include + using namespace eprosima::fastrtps::xmlparser; using eprosima::fastrtps::ParticipantAttributes; @@ -97,6 +99,10 @@ DomainParticipantFactory::~DomainParticipantFactory() DomainParticipantFactory* DomainParticipantFactory::get_instance() { + // Keep a reference to the topic payload pool to avoid it to be destroyed before our own instance + using pool_registry_ref = eprosima::fastrtps::rtps::TopicPayloadPoolRegistry::reference; + static pool_registry_ref topic_pool_registry = eprosima::fastrtps::rtps::TopicPayloadPoolRegistry::instance(); + static DomainParticipantFactory instance; return &instance; } diff --git a/src/cpp/rtps/history/TopicPayloadPoolRegistry.cpp b/src/cpp/rtps/history/TopicPayloadPoolRegistry.cpp index 4cbc6312158..defe03e8ab8 100644 --- a/src/cpp/rtps/history/TopicPayloadPoolRegistry.cpp +++ b/src/cpp/rtps/history/TopicPayloadPoolRegistry.cpp @@ -28,11 +28,16 @@ namespace eprosima { namespace fastrtps { namespace rtps { +const TopicPayloadPoolRegistry::reference& TopicPayloadPoolRegistry::instance() +{ + return detail::TopicPayloadPoolRegistry::instance(); +} + std::shared_ptr TopicPayloadPoolRegistry::get( const std::string& topic_name, const BasicPoolConfig& config) { - return detail::TopicPayloadPoolRegistry::instance().get(topic_name, config); + return detail::TopicPayloadPoolRegistry::instance()->get(topic_name, config); } } // namespace rtps diff --git a/src/cpp/rtps/history/TopicPayloadPoolRegistry.hpp b/src/cpp/rtps/history/TopicPayloadPoolRegistry.hpp index b4965910fd4..5f12c70b910 100644 --- a/src/cpp/rtps/history/TopicPayloadPoolRegistry.hpp +++ b/src/cpp/rtps/history/TopicPayloadPoolRegistry.hpp @@ -30,11 +30,19 @@ namespace eprosima { namespace fastrtps { namespace rtps { +namespace detail { + class TopicPayloadPoolRegistry; +} + class TopicPayloadPoolRegistry { public: + using reference = std::shared_ptr; + + static const reference& instance(); + static std::shared_ptr get( const std::string& topic_name, const BasicPoolConfig& config); diff --git a/src/cpp/rtps/history/TopicPayloadPoolRegistry_impl/TopicPayloadPoolRegistry.hpp b/src/cpp/rtps/history/TopicPayloadPoolRegistry_impl/TopicPayloadPoolRegistry.hpp index 58fbd425ef0..f79bb8f30f4 100644 --- a/src/cpp/rtps/history/TopicPayloadPoolRegistry_impl/TopicPayloadPoolRegistry.hpp +++ b/src/cpp/rtps/history/TopicPayloadPoolRegistry_impl/TopicPayloadPoolRegistry.hpp @@ -37,12 +37,10 @@ class TopicPayloadPoolRegistry public: - ~TopicPayloadPoolRegistry() = default; - /// @return reference to singleton instance - static TopicPayloadPoolRegistry& instance() + static const std::shared_ptr& instance() { - static TopicPayloadPoolRegistry pool_registry_instance; + static auto pool_registry_instance = std::shared_ptr(new TopicPayloadPoolRegistry()); return pool_registry_instance; } diff --git a/src/cpp/rtps/transport/shared_mem/SharedMemGlobal.hpp b/src/cpp/rtps/transport/shared_mem/SharedMemGlobal.hpp index 8791d376330..002dde9cd65 100644 --- a/src/cpp/rtps/transport/shared_mem/SharedMemGlobal.hpp +++ b/src/cpp/rtps/transport/shared_mem/SharedMemGlobal.hpp @@ -164,10 +164,10 @@ class SharedMemGlobal MultiProducerConsumerRingBuffer* buffer; }; - static WatchTask& get() + static const std::shared_ptr& get() { - static WatchTask watch_task; - return watch_task; + static auto watch_task_instance = std::shared_ptr(new WatchTask()); + return watch_task_instance; } /** @@ -203,19 +203,23 @@ class SharedMemGlobal } + ~WatchTask() + { + shared_mem_watchdog_->remove_task(this); + } + private: std::vector> watched_ports_; std::mutex watched_ports_mutex_; - WatchTask() - { - SharedMemWatchdog::get().add_task(this); - } + // Keep a reference to the SharedMemWatchdog so that it is not destroyed until this instance is destroyed + std::shared_ptr shared_mem_watchdog_; - ~WatchTask() + WatchTask() + : shared_mem_watchdog_(SharedMemWatchdog::get()) { - SharedMemWatchdog::get().remove_task(this); + shared_mem_watchdog_->add_task(this); } bool update_status_all_listeners( @@ -340,6 +344,9 @@ class SharedMemGlobal return (listeners_found == node_->num_listeners); } + // Keep a reference to the WatchTask so that it is not destroyed until the last Port instance is destroyed + std::shared_ptr watch_task_; + public: /** @@ -379,6 +386,7 @@ class SharedMemGlobal , node_(node) , overflows_count_(0) , read_exclusive_lock_(std::move(read_exclusive_lock)) + , watch_task_(WatchTask::get()) { auto buffer_base = static_cast::Cell*>( port_segment_->get_address_from_offset(node_->buffer)); @@ -393,12 +401,12 @@ class SharedMemGlobal auto port_context = std::make_shared(); *port_context = {port_segment_, node_, buffer_.get()}; - Port::WatchTask::get().add_port(std::move(port_context)); + Port::WatchTask::get()->add_port(std::move(port_context)); } ~Port() { - Port::WatchTask::get().remove_port(node_); + Port::WatchTask::get()->remove_port(node_); if (node_->ref_counter.fetch_sub(1) == 1) { diff --git a/src/cpp/rtps/transport/shared_mem/SharedMemManager.hpp b/src/cpp/rtps/transport/shared_mem/SharedMemManager.hpp index 0c0b8042404..e724c51dabe 100644 --- a/src/cpp/rtps/transport/shared_mem/SharedMemManager.hpp +++ b/src/cpp/rtps/transport/shared_mem/SharedMemManager.hpp @@ -230,6 +230,7 @@ class SharedMemManager : const std::string& domain_name) : segments_mem_(0) , global_segment_(domain_name) + , watch_task_(SegmentWrapper::WatchTask::get()) { static_assert(std::alignment_of::value % 8 == 0, "SharedMemManager::BufferNode bad alignment"); @@ -1000,10 +1001,10 @@ class SharedMemManager : { public: - static WatchTask& get() + static std::shared_ptr& get() { - static WatchTask watch_task; - return watch_task; + static auto watch_task_instance = std::shared_ptr(new WatchTask()); + return watch_task_instance; } void add_segment( @@ -1024,6 +1025,11 @@ class SharedMemManager : to_remove_.push_back(segment); } + ~WatchTask() + { + shared_mem_watchdog_->remove_task(this); + } + private: std::unordered_map, uint32_t> watched_segments_; @@ -1033,15 +1039,14 @@ class SharedMemManager : std::vector> to_add_; std::vector> to_remove_; + // Keep a reference to the SharedMemWatchdog so that it is not destroyed until this instance is destroyed + std::shared_ptr shared_mem_watchdog_; + WatchTask() : watched_it_(watched_segments_.end()) + , shared_mem_watchdog_(SharedMemWatchdog::get()) { - SharedMemWatchdog::get().add_task(this); - } - - ~WatchTask() - { - SharedMemWatchdog::get().remove_task(this); + shared_mem_watchdog_->add_task(this); } void update_watched_segments() @@ -1192,6 +1197,9 @@ class SharedMemManager : SharedMemGlobal global_segment_; + // Keep a reference to the WatchTask so that it is not destroyed until all Manger instances are destroyed + std::shared_ptr watch_task_; + std::shared_ptr find_segment( SharedMemSegment::Id id) { @@ -1213,7 +1221,7 @@ class SharedMemManager : ids_segments_[id.get()] = segment_wrapper; segments_mem_ += segment->mem_size(); - SegmentWrapper::WatchTask::get().add_segment(segment_wrapper); + SegmentWrapper::WatchTask::get()->add_segment(segment_wrapper); } return segment; @@ -1244,7 +1252,7 @@ class SharedMemManager : for (auto segment : ids_segments_) { - SegmentWrapper::WatchTask::get().remove_segment(segment.second); + SegmentWrapper::WatchTask::get()->remove_segment(segment.second); } } diff --git a/src/cpp/utils/shared_memory/SharedMemWatchdog.hpp b/src/cpp/utils/shared_memory/SharedMemWatchdog.hpp index d74e95191bc..052d3994a4a 100644 --- a/src/cpp/utils/shared_memory/SharedMemWatchdog.hpp +++ b/src/cpp/utils/shared_memory/SharedMemWatchdog.hpp @@ -40,10 +40,10 @@ class SharedMemWatchdog virtual void run() = 0; }; - static SharedMemWatchdog& get() + static std::shared_ptr& get() { - static SharedMemWatchdog watch_dog; - return watch_dog; + static auto watch_dog_instance = std::shared_ptr(new SharedMemWatchdog()); + return watch_dog_instance; } /** @@ -79,6 +79,13 @@ class SharedMemWatchdog return std::chrono::milliseconds(1000); } + ~SharedMemWatchdog() + { + exit_thread_ = true; + wake_up(); + thread_run_.join(); + } + private: std::unordered_set tasks_; @@ -98,13 +105,6 @@ class SharedMemWatchdog thread_run_ = std::thread(&SharedMemWatchdog::run, this); } - ~SharedMemWatchdog() - { - exit_thread_ = true; - wake_up(); - thread_run_.join(); - } - /** * Forces Wake-up of the checking thread */ diff --git a/test/unittest/dds/publisher/DataWriterTests.cpp b/test/unittest/dds/publisher/DataWriterTests.cpp index 5ba49f83e6b..62041c906c7 100644 --- a/test/unittest/dds/publisher/DataWriterTests.cpp +++ b/test/unittest/dds/publisher/DataWriterTests.cpp @@ -506,6 +506,25 @@ TEST(DataWriterTests, SetListener) ASSERT_TRUE(DomainParticipantFactory::get_instance()->delete_participant(participant) == ReturnCode_t::RETCODE_OK); } +TEST(DataWriterTests, TerminateWithoutDestroyingWriter) +{ + DomainParticipant* participant = + DomainParticipantFactory::get_instance()->create_participant(0, PARTICIPANT_QOS_DEFAULT); + ASSERT_NE(participant, nullptr); + + Publisher* publisher = participant->create_publisher(PUBLISHER_QOS_DEFAULT); + ASSERT_NE(publisher, nullptr); + + TypeSupport type(new TopicDataTypeMock()); + type.register_type(participant); + + Topic* topic = participant->create_topic("footopic", type.get_type_name(), TOPIC_QOS_DEFAULT); + ASSERT_NE(topic, nullptr); + + DataWriter* datawriter = publisher->create_datawriter(topic, DATAWRITER_QOS_DEFAULT); + ASSERT_NE(datawriter, nullptr); +} + struct LoanableType { static constexpr uint32_t initialization_value() diff --git a/test/unittest/dds/subscriber/DataReaderTests.cpp b/test/unittest/dds/subscriber/DataReaderTests.cpp index 0d60d913c74..976382dcabe 100644 --- a/test/unittest/dds/subscriber/DataReaderTests.cpp +++ b/test/unittest/dds/subscriber/DataReaderTests.cpp @@ -77,6 +77,11 @@ class DataReaderTests : public ::testing::Test void TearDown() override { + if (!destroy_entities_) + { + return; + } + if (data_writer_) { ASSERT_EQ(publisher_->delete_datawriter(data_writer_), ReturnCode_t::RETCODE_OK); @@ -528,6 +533,7 @@ class DataReaderTests : public ::testing::Test DataReader* data_reader_ = nullptr; DataWriter* data_writer_ = nullptr; TypeSupport type_; + bool destroy_entities_ = true; InstanceHandle_t handle_ok_ = HANDLE_NIL; InstanceHandle_t handle_wrong_ = HANDLE_NIL; @@ -1272,6 +1278,27 @@ TEST_F(DataReaderTests, read_unread) } } +TEST_F(DataReaderTests, TerminateWithoutDestroyingReader) +{ + destroy_entities_ = false; + + DomainParticipant* participant = + DomainParticipantFactory::get_instance()->create_participant(0, PARTICIPANT_QOS_DEFAULT); + ASSERT_NE(participant, nullptr); + + Subscriber* subscriber = participant->create_subscriber(SUBSCRIBER_QOS_DEFAULT); + ASSERT_NE(participant, nullptr); + + TypeSupport type(new FooTypeSupport()); + type.register_type(participant); + + Topic* topic = participant->create_topic("footopic", type.get_type_name(), TOPIC_QOS_DEFAULT); + ASSERT_NE(topic, nullptr); + + DataReader* datareader = subscriber->create_datareader(topic, DATAREADER_QOS_DEFAULT); + ASSERT_NE(datareader, nullptr); +} + void set_listener_test ( DataReader* reader, DataReaderListener* listener, From 25988c874ce9ee3ec3b8b42a2fe72b3b8a9ff4b6 Mon Sep 17 00:00:00 2001 From: Iker Luengo Date: Tue, 9 Feb 2021 13:09:10 +0100 Subject: [PATCH 2/5] Refs 10533 Uncrustify Signed-off-by: Iker Luengo --- src/cpp/rtps/history/TopicPayloadPoolRegistry.hpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cpp/rtps/history/TopicPayloadPoolRegistry.hpp b/src/cpp/rtps/history/TopicPayloadPoolRegistry.hpp index 5f12c70b910..466f66866e2 100644 --- a/src/cpp/rtps/history/TopicPayloadPoolRegistry.hpp +++ b/src/cpp/rtps/history/TopicPayloadPoolRegistry.hpp @@ -31,8 +31,8 @@ namespace fastrtps { namespace rtps { namespace detail { - class TopicPayloadPoolRegistry; -} +class TopicPayloadPoolRegistry; +} // namespace detail class TopicPayloadPoolRegistry { From 5a7af92864c7c9733013869adf442b20f760ea84 Mon Sep 17 00:00:00 2001 From: Iker Luengo Date: Tue, 9 Feb 2021 17:53:38 +0100 Subject: [PATCH 3/5] Refs 10533 Resolve warnings Signed-off-by: Iker Luengo --- src/cpp/rtps/transport/shared_mem/SharedMemGlobal.hpp | 2 +- src/cpp/rtps/transport/shared_mem/SharedMemManager.hpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cpp/rtps/transport/shared_mem/SharedMemGlobal.hpp b/src/cpp/rtps/transport/shared_mem/SharedMemGlobal.hpp index 002dde9cd65..3e11a0a3b7b 100644 --- a/src/cpp/rtps/transport/shared_mem/SharedMemGlobal.hpp +++ b/src/cpp/rtps/transport/shared_mem/SharedMemGlobal.hpp @@ -203,7 +203,7 @@ class SharedMemGlobal } - ~WatchTask() + virtual ~WatchTask() { shared_mem_watchdog_->remove_task(this); } diff --git a/src/cpp/rtps/transport/shared_mem/SharedMemManager.hpp b/src/cpp/rtps/transport/shared_mem/SharedMemManager.hpp index e724c51dabe..ae3bef1fe4a 100644 --- a/src/cpp/rtps/transport/shared_mem/SharedMemManager.hpp +++ b/src/cpp/rtps/transport/shared_mem/SharedMemManager.hpp @@ -1025,7 +1025,7 @@ class SharedMemManager : to_remove_.push_back(segment); } - ~WatchTask() + virtual ~WatchTask() { shared_mem_watchdog_->remove_task(this); } From 4da5971c3f114cb737c2742dbf1e3ab64b96788c Mon Sep 17 00:00:00 2001 From: Iker Luengo Date: Wed, 10 Feb 2021 08:37:50 +0100 Subject: [PATCH 4/5] Refs 10533 Avoid copy on instance construction Signed-off-by: Iker Luengo --- .../TopicPayloadPoolRegistry_impl/TopicPayloadPoolRegistry.hpp | 2 +- src/cpp/rtps/transport/shared_mem/SharedMemGlobal.hpp | 2 +- src/cpp/rtps/transport/shared_mem/SharedMemManager.hpp | 2 +- src/cpp/utils/shared_memory/SharedMemWatchdog.hpp | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/cpp/rtps/history/TopicPayloadPoolRegistry_impl/TopicPayloadPoolRegistry.hpp b/src/cpp/rtps/history/TopicPayloadPoolRegistry_impl/TopicPayloadPoolRegistry.hpp index f79bb8f30f4..d55dfb7ce22 100644 --- a/src/cpp/rtps/history/TopicPayloadPoolRegistry_impl/TopicPayloadPoolRegistry.hpp +++ b/src/cpp/rtps/history/TopicPayloadPoolRegistry_impl/TopicPayloadPoolRegistry.hpp @@ -40,7 +40,7 @@ class TopicPayloadPoolRegistry /// @return reference to singleton instance static const std::shared_ptr& instance() { - static auto pool_registry_instance = std::shared_ptr(new TopicPayloadPoolRegistry()); + static std::shared_ptr pool_registry_instance(new TopicPayloadPoolRegistry()); return pool_registry_instance; } diff --git a/src/cpp/rtps/transport/shared_mem/SharedMemGlobal.hpp b/src/cpp/rtps/transport/shared_mem/SharedMemGlobal.hpp index 3e11a0a3b7b..eea6af10572 100644 --- a/src/cpp/rtps/transport/shared_mem/SharedMemGlobal.hpp +++ b/src/cpp/rtps/transport/shared_mem/SharedMemGlobal.hpp @@ -166,7 +166,7 @@ class SharedMemGlobal static const std::shared_ptr& get() { - static auto watch_task_instance = std::shared_ptr(new WatchTask()); + static std::shared_ptr watch_task_instance(new WatchTask()); return watch_task_instance; } diff --git a/src/cpp/rtps/transport/shared_mem/SharedMemManager.hpp b/src/cpp/rtps/transport/shared_mem/SharedMemManager.hpp index ae3bef1fe4a..7497855b798 100644 --- a/src/cpp/rtps/transport/shared_mem/SharedMemManager.hpp +++ b/src/cpp/rtps/transport/shared_mem/SharedMemManager.hpp @@ -1003,7 +1003,7 @@ class SharedMemManager : static std::shared_ptr& get() { - static auto watch_task_instance = std::shared_ptr(new WatchTask()); + static std::shared_ptr watch_task_instance(new WatchTask()); return watch_task_instance; } diff --git a/src/cpp/utils/shared_memory/SharedMemWatchdog.hpp b/src/cpp/utils/shared_memory/SharedMemWatchdog.hpp index 052d3994a4a..c0e395bc812 100644 --- a/src/cpp/utils/shared_memory/SharedMemWatchdog.hpp +++ b/src/cpp/utils/shared_memory/SharedMemWatchdog.hpp @@ -42,7 +42,7 @@ class SharedMemWatchdog static std::shared_ptr& get() { - static auto watch_dog_instance = std::shared_ptr(new SharedMemWatchdog()); + static std::shared_ptr watch_dog_instance(new SharedMemWatchdog()); return watch_dog_instance; } From b8bd004ca92de0b33b27a72786e6abd6d1eb65ac Mon Sep 17 00:00:00 2001 From: Iker Luengo Date: Wed, 10 Feb 2021 08:38:43 +0100 Subject: [PATCH 5/5] Refs 10533 use create_entities on the new test Signed-off-by: Iker Luengo --- .../unittest/dds/subscriber/DataReaderTests.cpp | 17 +---------------- 1 file changed, 1 insertion(+), 16 deletions(-) diff --git a/test/unittest/dds/subscriber/DataReaderTests.cpp b/test/unittest/dds/subscriber/DataReaderTests.cpp index 976382dcabe..0b0e8f8146f 100644 --- a/test/unittest/dds/subscriber/DataReaderTests.cpp +++ b/test/unittest/dds/subscriber/DataReaderTests.cpp @@ -1281,22 +1281,7 @@ TEST_F(DataReaderTests, read_unread) TEST_F(DataReaderTests, TerminateWithoutDestroyingReader) { destroy_entities_ = false; - - DomainParticipant* participant = - DomainParticipantFactory::get_instance()->create_participant(0, PARTICIPANT_QOS_DEFAULT); - ASSERT_NE(participant, nullptr); - - Subscriber* subscriber = participant->create_subscriber(SUBSCRIBER_QOS_DEFAULT); - ASSERT_NE(participant, nullptr); - - TypeSupport type(new FooTypeSupport()); - type.register_type(participant); - - Topic* topic = participant->create_topic("footopic", type.get_type_name(), TOPIC_QOS_DEFAULT); - ASSERT_NE(topic, nullptr); - - DataReader* datareader = subscriber->create_datareader(topic, DATAREADER_QOS_DEFAULT); - ASSERT_NE(datareader, nullptr); + create_entities(); } void set_listener_test (