From 83fb9b29bf9aa0b3139c9e3b62a8868b85ba60af Mon Sep 17 00:00:00 2001 From: Mario Dominguez Date: Wed, 27 Nov 2024 16:08:22 +0100 Subject: [PATCH 1/4] Refs #22339: Add BB test Signed-off-by: Mario Dominguez --- .../blackbox/common/DDSBlackboxTestsBasic.cpp | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/test/blackbox/common/DDSBlackboxTestsBasic.cpp b/test/blackbox/common/DDSBlackboxTestsBasic.cpp index 1670ec49b0c..540281d62d5 100644 --- a/test/blackbox/common/DDSBlackboxTestsBasic.cpp +++ b/test/blackbox/common/DDSBlackboxTestsBasic.cpp @@ -1014,6 +1014,43 @@ TEST(DDSBasic, successful_destruction_among_intraprocess_participants) } } } +TEST(DDSBasic, reliable_volatile_writer_secure_builtin_no_potential_deadlock) +{ + // Create + PubSubWriter writer("HelloWorldTopic_no_potential_deadlock"); + PubSubReader reader("HelloWorldTopic_no_potential_deadlock"); + + writer.asynchronously(eprosima::fastdds::dds::ASYNCHRONOUS_PUBLISH_MODE) + .durability_kind(eprosima::fastdds::dds::VOLATILE_DURABILITY_QOS) + .history_kind(eprosima::fastdds::dds::KEEP_LAST_HISTORY_QOS) + .history_depth(20) + .init(); + + ASSERT_TRUE(writer.isInitialized()); + + reader.reliability(eprosima::fastdds::dds::RELIABLE_RELIABILITY_QOS) + .history_kind(eprosima::fastdds::dds::KEEP_LAST_HISTORY_QOS) + .history_depth(20) + .durability_kind(eprosima::fastdds::dds::VOLATILE_DURABILITY_QOS) + .init(); + + ASSERT_TRUE(reader.isInitialized()); + + auto data = default_helloworld_data_generator(30); + + std::thread th([&]() + { + reader.startReception(data); + reader.block_for_at_least(5); + }); + + writer.wait_discovery(); + writer.send(data); + + th.join(); + reader.destroy(); + writer.destroy(); +} } // namespace dds } // namespace fastdds From 9de8ea9d3cb965e2a2555f75850ffac61b74f681 Mon Sep 17 00:00:00 2001 From: Mario Dominguez Date: Wed, 27 Nov 2024 16:08:54 +0100 Subject: [PATCH 2/4] Refs #22339: Fix tsan deadlock report Signed-off-by: Mario Dominguez --- src/cpp/rtps/writer/StatefulWriter.cpp | 75 +++++++++++++------------- 1 file changed, 37 insertions(+), 38 deletions(-) diff --git a/src/cpp/rtps/writer/StatefulWriter.cpp b/src/cpp/rtps/writer/StatefulWriter.cpp index ced15bae493..83c01a9098f 100644 --- a/src/cpp/rtps/writer/StatefulWriter.cpp +++ b/src/cpp/rtps/writer/StatefulWriter.cpp @@ -1217,56 +1217,59 @@ bool StatefulWriter::matched_reader_remove( const GUID_t& reader_guid) { ReaderProxy* rproxy = nullptr; - std::unique_lock lock(mp_mutex); - std::unique_lock guard_locator_selector_general(locator_selector_general_); - std::unique_lock guard_locator_selector_async(locator_selector_async_); - for (ReaderProxyIterator it = matched_local_readers_.begin(); - it != matched_local_readers_.end(); ++it) { - if ((*it)->guid() == reader_guid) - { - EPROSIMA_LOG_INFO(RTPS_WRITER, "Reader Proxy removed: " << reader_guid); - rproxy = std::move(*it); - it = matched_local_readers_.erase(it); - break; - } - } + std::lock_guard lock(mp_mutex); + std::lock_guard guard_locator_selector_general(locator_selector_general_); + std::lock_guard guard_locator_selector_async(locator_selector_async_); - if (rproxy == nullptr) - { - for (ReaderProxyIterator it = matched_datasharing_readers_.begin(); - it != matched_datasharing_readers_.end(); ++it) + for (ReaderProxyIterator it = matched_local_readers_.begin(); + it != matched_local_readers_.end(); ++it) { if ((*it)->guid() == reader_guid) { EPROSIMA_LOG_INFO(RTPS_WRITER, "Reader Proxy removed: " << reader_guid); rproxy = std::move(*it); - it = matched_datasharing_readers_.erase(it); + it = matched_local_readers_.erase(it); break; } } - } - if (rproxy == nullptr) - { - for (ReaderProxyIterator it = matched_remote_readers_.begin(); - it != matched_remote_readers_.end(); ++it) + if (rproxy == nullptr) { - if ((*it)->guid() == reader_guid) + for (ReaderProxyIterator it = matched_datasharing_readers_.begin(); + it != matched_datasharing_readers_.end(); ++it) { - EPROSIMA_LOG_INFO(RTPS_WRITER, "Reader Proxy removed: " << reader_guid); - rproxy = std::move(*it); - it = matched_remote_readers_.erase(it); - break; + if ((*it)->guid() == reader_guid) + { + EPROSIMA_LOG_INFO(RTPS_WRITER, "Reader Proxy removed: " << reader_guid); + rproxy = std::move(*it); + it = matched_datasharing_readers_.erase(it); + break; + } + } + } + + if (rproxy == nullptr) + { + for (ReaderProxyIterator it = matched_remote_readers_.begin(); + it != matched_remote_readers_.end(); ++it) + { + if ((*it)->guid() == reader_guid) + { + EPROSIMA_LOG_INFO(RTPS_WRITER, "Reader Proxy removed: " << reader_guid); + rproxy = std::move(*it); + it = matched_remote_readers_.erase(it); + break; + } } } - } - locator_selector_general_.locator_selector.remove_entry(reader_guid); - locator_selector_async_.locator_selector.remove_entry(reader_guid); - update_reader_info(locator_selector_general_, false); - update_reader_info(locator_selector_async_, false); + locator_selector_general_.locator_selector.remove_entry(reader_guid); + locator_selector_async_.locator_selector.remove_entry(reader_guid); + update_reader_info(locator_selector_general_, false); + update_reader_info(locator_selector_async_, false); + } if (get_matched_readers_size() == 0) { @@ -1282,11 +1285,7 @@ bool StatefulWriter::matched_reader_remove( if (nullptr != listener_) { - // call the listener without locks taken - guard_locator_selector_async.unlock(); - guard_locator_selector_general.unlock(); - lock.unlock(); - + // listener is called without locks taken listener_->on_reader_discovery(this, ReaderDiscoveryStatus::REMOVED_READER, reader_guid, nullptr); } From 211423d4320ee78e046edc7721f90c0837c21264 Mon Sep 17 00:00:00 2001 From: Mario Dominguez Date: Wed, 11 Dec 2024 10:47:31 +0100 Subject: [PATCH 3/4] Refs #22339: Take writer's mutex before rproxy->stop() and check_acked_status() Signed-off-by: Mario Dominguez --- src/cpp/rtps/writer/StatefulWriter.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cpp/rtps/writer/StatefulWriter.cpp b/src/cpp/rtps/writer/StatefulWriter.cpp index 83c01a9098f..2e6970bb317 100644 --- a/src/cpp/rtps/writer/StatefulWriter.cpp +++ b/src/cpp/rtps/writer/StatefulWriter.cpp @@ -1217,9 +1217,9 @@ bool StatefulWriter::matched_reader_remove( const GUID_t& reader_guid) { ReaderProxy* rproxy = nullptr; + std::lock_guard lock(mp_mutex); { - std::lock_guard lock(mp_mutex); std::lock_guard guard_locator_selector_general(locator_selector_general_); std::lock_guard guard_locator_selector_async(locator_selector_async_); From a5a1787c5dd2d3959a35a2b5c8b854a34df53646 Mon Sep 17 00:00:00 2001 From: Mario Dominguez Date: Wed, 11 Dec 2024 12:45:49 +0100 Subject: [PATCH 4/4] Refs #22339: Apply Miguels suggestion Signed-off-by: Mario Dominguez --- src/cpp/rtps/writer/StatefulWriter.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/cpp/rtps/writer/StatefulWriter.cpp b/src/cpp/rtps/writer/StatefulWriter.cpp index 2e6970bb317..f0dd8f2d883 100644 --- a/src/cpp/rtps/writer/StatefulWriter.cpp +++ b/src/cpp/rtps/writer/StatefulWriter.cpp @@ -1217,7 +1217,7 @@ bool StatefulWriter::matched_reader_remove( const GUID_t& reader_guid) { ReaderProxy* rproxy = nullptr; - std::lock_guard lock(mp_mutex); + std::unique_lock lock(mp_mutex); { std::lock_guard guard_locator_selector_general(locator_selector_general_); @@ -1286,6 +1286,7 @@ bool StatefulWriter::matched_reader_remove( if (nullptr != listener_) { // listener is called without locks taken + lock.unlock(); listener_->on_reader_discovery(this, ReaderDiscoveryStatus::REMOVED_READER, reader_guid, nullptr); }