From 1ac57910394abebc33c4b63b4f23f6a5dce40f0c Mon Sep 17 00:00:00 2001 From: juanlofer-eprosima <88179026+juanlofer-eprosima@users.noreply.github.com> Date: Mon, 18 Dec 2023 16:21:26 +0100 Subject: [PATCH] Fix data race on writer destruction while sending heartbeat (#4076) * Add regression test Signed-off-by: Juan Lopez Fernandez * Protect changes release when destroying writer Signed-off-by: Juan Lopez Fernandez * Apply suggestions Signed-off-by: Juan Lopez Fernandez * Fix warning Signed-off-by: Juan Lopez Fernandez --------- Signed-off-by: Juan Lopez Fernandez (cherry picked from commit 61e1c6a669005fe211a846e3ac874c189446dd90) --- src/cpp/rtps/writer/RTPSWriter.cpp | 13 +++--- .../common/DDSBlackboxTestsDataWriter.cpp | 43 +++++++++++++++++++ 2 files changed, 50 insertions(+), 6 deletions(-) diff --git a/src/cpp/rtps/writer/RTPSWriter.cpp b/src/cpp/rtps/writer/RTPSWriter.cpp index f967d28f9d2..22f066226ab 100644 --- a/src/cpp/rtps/writer/RTPSWriter.cpp +++ b/src/cpp/rtps/writer/RTPSWriter.cpp @@ -154,13 +154,14 @@ void RTPSWriter::deinit() { flow_controller_->remove_change(*it); } - } - for (auto it = mp_history->changesBegin(); it != mp_history->changesEnd(); ++it) - { - release_change(*it); - } - mp_history->m_changes.clear(); + for (auto it = mp_history->changesBegin(); it != mp_history->changesEnd(); ++it) + { + release_change(*it); + } + + mp_history->m_changes.clear(); + } flow_controller_->unregister_writer(this); } diff --git a/test/blackbox/common/DDSBlackboxTestsDataWriter.cpp b/test/blackbox/common/DDSBlackboxTestsDataWriter.cpp index b079af186f8..3a6ff7194f5 100644 --- a/test/blackbox/common/DDSBlackboxTestsDataWriter.cpp +++ b/test/blackbox/common/DDSBlackboxTestsDataWriter.cpp @@ -369,6 +369,49 @@ TEST(DDSDataWriter, OfferedDeadlineMissedListener) ASSERT_TRUE(ret); } +/** + * Regression test for EasyRedmine issue https://eprosima.easyredmine.com/issues/20059 + * + * The test creates a writer and reader that communicate with transient_local reliable QoS. + * The issue corresponds to a race condition involving writer's history destruction and heartbeat delivery, so in order + * to increment the probability of occurrence a high history depth and heartbeat frequency are used. + * + * Note: + * - Only affects TRANSPORT case (UDP or SHM communication, data_sharing and intraprocess disabled) + * - Destruction order matters: writer must be destroyed before reader (otherwise heartbeats would no be sent while + * destroying the writer) + */ +TEST(DDSDataWriter, HeartbeatWhileDestruction) +{ + PubSubReader reader(TEST_TOPIC_NAME); + + // Force writer to be destroyed before reader, so they are still matched, and heartbeats are sent while writer is destroyed + { + PubSubWriter writer(TEST_TOPIC_NAME); + + // A high number of samples increases the probability of the data race to occur + size_t n_samples = 1000; + + reader.reliability(RELIABLE_RELIABILITY_QOS).durability_kind(TRANSIENT_LOCAL_DURABILITY_QOS).init(); + ASSERT_TRUE(reader.isInitialized()); + + writer.reliability(RELIABLE_RELIABILITY_QOS).durability_kind(TRANSIENT_LOCAL_DURABILITY_QOS).history_kind( + KEEP_LAST_HISTORY_QOS).history_depth(static_cast(n_samples)).heartbeat_period_seconds(0). + heartbeat_period_nanosec( + 20 * 1000).init(); + ASSERT_TRUE(writer.isInitialized()); + + reader.wait_discovery(); + writer.wait_discovery(); + + auto data = default_helloworld_data_generator(n_samples); + reader.startReception(data); + writer.send(data); + + EXPECT_TRUE(data.empty()); + } +} + #ifdef INSTANTIATE_TEST_SUITE_P #define GTEST_INSTANTIATE_TEST_MACRO(x, y, z, w) INSTANTIATE_TEST_SUITE_P(x, y, z, w) #else