Fixing deadlock in WLP (#3115)

* Refs 16166. Test reproducer. Signed-off-by: Miguel Barro <miguelbarro@eprosima.com> * Refs 16166. Fixing deadlock. Signed-off-by: Miguel Barro <miguelbarro@eprosima.com> * Refs 16166. Linter. Signed-off-by: Miguel Barro <miguelbarro@eprosima.com> * Refs 16166. Fixing deprecated blackbox tests Signed-off-by: Miguel Barro <miguelbarro@eprosima.com> * Refs 16166. Linter Signed-off-by: Miguel Barro <miguelbarro@eprosima.com> Signed-off-by: Miguel Barro <miguelbarro@eprosima.com>
eProsima · Dec 15, 2022 · acc3fd8 · acc3fd8
1 parent be82e6f
commit acc3fd8
Show file tree

Hide file tree

Showing 4 changed files with 151 additions and 1 deletion.
diff --git a/src/cpp/rtps/builtin/liveliness/WLP.cpp b/src/cpp/rtps/builtin/liveliness/WLP.cpp
@@ -866,12 +866,13 @@ bool WLP::automatic_liveliness_assertion()
 
 bool WLP::participant_liveliness_assertion()
 {
-    std::lock_guard<std::recursive_mutex> guard(*mp_builtinProtocols->mp_PDP->getMutex());
+    std::unique_lock<std::recursive_mutex> lock(*mp_builtinProtocols->mp_PDP->getMutex());
 
     if (0 < manual_by_participant_writers_.size())
     {
         if (pub_liveliness_manager_->is_any_alive(MANUAL_BY_PARTICIPANT_LIVELINESS_QOS))
         {
+            lock.unlock();
             return send_liveliness_message(manual_by_participant_instance_handle_);
         }
     }

diff --git a/test/blackbox/api/dds-pim/PubSubWriterReader.hpp b/test/blackbox/api/dds-pim/PubSubWriterReader.hpp
@@ -757,6 +757,53 @@ class PubSubWriterReader
         return *this;
     }
 
+    PubSubWriterReader& pub_liveliness_kind(
+            const eprosima::fastrtps::LivelinessQosPolicyKind kind)
+    {
+        datawriter_qos_.liveliness().kind = kind;
+        return *this;
+    }
+
+    PubSubWriterReader& sub_liveliness_kind(
+            const eprosima::fastrtps::LivelinessQosPolicyKind kind)
+    {
+        datareader_qos_.liveliness().kind = kind;
+        return *this;
+    }
+
+    PubSubWriterReader& pub_liveliness_announcement_period(
+            const eprosima::fastrtps::Duration_t announcement_period)
+    {
+        datawriter_qos_.liveliness().announcement_period = announcement_period;
+        return *this;
+    }
+
+    PubSubWriterReader& sub_liveliness_announcement_period(
+            const eprosima::fastrtps::Duration_t announcement_period)
+    {
+        datareader_qos_.liveliness().announcement_period = announcement_period;
+        return *this;
+    }
+
+    PubSubWriterReader& pub_liveliness_lease_duration(
+            const eprosima::fastrtps::Duration_t lease_duration)
+    {
+        datawriter_qos_.liveliness().lease_duration = lease_duration;
+        return *this;
+    }
+
+    PubSubWriterReader& sub_liveliness_lease_duration(
+            const eprosima::fastrtps::Duration_t lease_duration)
+    {
+        datareader_qos_.liveliness().lease_duration = lease_duration;
+        return *this;
+    }
+
+    void assert_liveliness()
+    {
+        datawriter_->assert_liveliness();
+    }
+
     size_t get_num_discovered_participants() const
     {
         return participant_listener_.get_num_discovered_participants();

diff --git a/test/blackbox/api/fastrtps_deprecated/PubSubWriterReader.hpp b/test/blackbox/api/fastrtps_deprecated/PubSubWriterReader.hpp
@@ -657,6 +657,53 @@ class PubSubWriterReader
         return *this;
     }
 
+    PubSubWriterReader& pub_liveliness_kind(
+            const eprosima::fastrtps::LivelinessQosPolicyKind kind)
+    {
+        publisher_attr_.qos.m_liveliness.kind = kind;
+        return *this;
+    }
+
+    PubSubWriterReader& sub_liveliness_kind(
+            const eprosima::fastrtps::LivelinessQosPolicyKind kind)
+    {
+        subscriber_attr_.qos.m_liveliness.kind = kind;
+        return *this;
+    }
+
+    PubSubWriterReader& pub_liveliness_announcement_period(
+            const eprosima::fastrtps::Duration_t announcement_period)
+    {
+        publisher_attr_.qos.m_liveliness.announcement_period = announcement_period;
+        return *this;
+    }
+
+    PubSubWriterReader& sub_liveliness_announcement_period(
+            const eprosima::fastrtps::Duration_t announcement_period)
+    {
+        subscriber_attr_.qos.m_liveliness.announcement_period = announcement_period;
+        return *this;
+    }
+
+    PubSubWriterReader& pub_liveliness_lease_duration(
+            const eprosima::fastrtps::Duration_t lease_duration)
+    {
+        publisher_attr_.qos.m_liveliness.lease_duration = lease_duration;
+        return *this;
+    }
+
+    PubSubWriterReader& sub_liveliness_lease_duration(
+            const eprosima::fastrtps::Duration_t lease_duration)
+    {
+        subscriber_attr_.qos.m_liveliness.lease_duration = lease_duration;
+        return *this;
+    }
+
+    void assert_liveliness()
+    {
+        publisher_->assert_liveliness();
+    }
+
     size_t get_num_discovered_participants() const
     {
         return participant_listener_.get_num_discovered_participants();

diff --git a/test/blackbox/common/BlackboxTestsLivelinessQos.cpp b/test/blackbox/common/BlackboxTestsLivelinessQos.cpp
@@ -14,8 +14,11 @@
 
 #include "BlackboxTests.hpp"
 
+#include <thread>
+
 #include "PubSubReader.hpp"
 #include "PubSubWriter.hpp"
+#include "PubSubWriterReader.hpp"
 #include "PubSubParticipant.hpp"
 #include "ReqRepAsReliableHelloWorldRequester.hpp"
 #include "ReqRepAsReliableHelloWorldReplier.hpp"
@@ -1891,6 +1894,58 @@ TEST_P(LivelinessQos, AssertLivelinessParticipant)
     EXPECT_EQ(publishers.pub_times_liveliness_lost(), 2u);
 }
 
+//! Tests associated with an ABBA deadlock discovered between PDP mutexes on two different participants.
+//! The deadlock scenario involved:
+//! + Intraprocess
+//! + WLP assertions due to MANUAL_BY_PARTICIPANT set up
+//! + Both participants' event thread trigger the liveliness assertion simultaneously
+TEST(LivelinessTests, Detect_Deadlock_ManualByParticipant_Intraprocess)
+{
+    // Set up intraprocess
+    LibrarySettingsAttributes library_settings;
+    library_settings.intraprocess_delivery = IntraprocessDeliveryType::INTRAPROCESS_FULL;
+    xmlparser::XMLProfileManager::library_settings(library_settings);
+
+    // Create two participants
+    PubSubWriterReader<HelloWorldPubSubType> participantA(TEST_TOPIC_NAME), participantB(TEST_TOPIC_NAME);
+
+    // Set up MANUAL_BY_PARTICIPANT liveliness and make it fast
+    unsigned int lease_duration_ms = 1000;
+    unsigned int announcement_period_ms = 1;
+
+    participantA.pub_liveliness_kind(MANUAL_BY_PARTICIPANT_LIVELINESS_QOS)
+            .sub_liveliness_kind(MANUAL_BY_PARTICIPANT_LIVELINESS_QOS)
+            .pub_liveliness_announcement_period(announcement_period_ms * 1e-3)
+            .sub_liveliness_announcement_period(announcement_period_ms * 1e-3)
+            .pub_liveliness_lease_duration(lease_duration_ms * 1e-3)
+            .sub_liveliness_lease_duration(lease_duration_ms * 1e-3)
+            .init();
+
+    participantB.pub_liveliness_kind(MANUAL_BY_PARTICIPANT_LIVELINESS_QOS)
+            .sub_liveliness_kind(MANUAL_BY_PARTICIPANT_LIVELINESS_QOS)
+            .pub_liveliness_announcement_period(announcement_period_ms * 1e-3)
+            .sub_liveliness_announcement_period(announcement_period_ms * 1e-3)
+            .pub_liveliness_lease_duration(lease_duration_ms * 1e-3)
+            .sub_liveliness_lease_duration(lease_duration_ms * 1e-3)
+            .init();
+
+    ASSERT_TRUE(participantA.isInitialized());
+    ASSERT_TRUE(participantB.isInitialized());
+
+    // Wait for discovery.
+    participantA.wait_discovery();
+    participantB.wait_discovery();
+
+    // The DataWriter is alive
+    participantA.assert_liveliness();
+    participantB.assert_liveliness();
+
+    // Wait a second expecting a deadlock
+    std::this_thread::sleep_for(std::chrono::seconds(1));
+
+    // Test failure is due to timeout
+}
+
 #ifdef INSTANTIATE_TEST_SUITE_P
 #define GTEST_INSTANTIATE_TEST_MACRO(x, y, z, w) INSTANTIATE_TEST_SUITE_P(x, y, z, w)
 #else