From ddf4bb95639b583748349392e1ca0b6142988f3c Mon Sep 17 00:00:00 2001 From: juanjo4936 <69901369+juanjo4936@users.noreply.github.com> Date: Thu, 30 Jan 2025 12:23:58 +0100 Subject: [PATCH] Reliable volatile change dropped when all history acked (#5606) * Refs #22658: Regression test Signed-off-by: Juanjo Garcia * Refs #22658: Fix bug Signed-off-by: Juanjo Garcia * Refs #22658: corrected failing CI Signed-off-by: Juanjo Garcia * Refs #22658: Included reviewer suggestions Signed-off-by: Juanjo Garcia --------- Signed-off-by: Juanjo Garcia (cherry picked from commit 90790cea8fb3a4da01bde6b5d3ca4d04cd59f80b) --- src/cpp/rtps/writer/StatefulWriter.cpp | 4 +- test/blackbox/api/dds-pim/PubSubWriter.hpp | 9 +++++ .../common/DDSBlackboxTestsListeners.cpp | 40 +++++++++++++++++++ 3 files changed, 52 insertions(+), 1 deletion(-) diff --git a/src/cpp/rtps/writer/StatefulWriter.cpp b/src/cpp/rtps/writer/StatefulWriter.cpp index 638cb201196..c81cc792d56 100644 --- a/src/cpp/rtps/writer/StatefulWriter.cpp +++ b/src/cpp/rtps/writer/StatefulWriter.cpp @@ -1516,7 +1516,9 @@ void StatefulWriter::check_acked_status() if (min_low_mark >= get_seq_num_min()) { - may_remove_change_ = 1; + // get_seq_num_min() returns SequenceNumber_t::unknown() when the history is empty. + // Thus, it is set to 2 to indicate that all samples have been removed. + may_remove_change_ = (get_seq_num_min() == SequenceNumber_t::unknown()) ? 2 : 1; } min_readers_low_mark_ = min_low_mark; diff --git a/test/blackbox/api/dds-pim/PubSubWriter.hpp b/test/blackbox/api/dds-pim/PubSubWriter.hpp index de086698407..f75ec3d2444 100644 --- a/test/blackbox/api/dds-pim/PubSubWriter.hpp +++ b/test/blackbox/api/dds-pim/PubSubWriter.hpp @@ -876,6 +876,15 @@ class PubSubWriter return *this; } + PubSubWriter& reliability( + const eprosima::fastdds::dds::ReliabilityQosPolicyKind kind, + eprosima::fastdds::dds::Duration_t max_blocking_time) + { + datawriter_qos_.reliability().kind = kind; + datawriter_qos_.reliability().max_blocking_time = max_blocking_time; + return *this; + } + PubSubWriter& mem_policy( const eprosima::fastdds::rtps::MemoryManagementPolicy mem_policy) { diff --git a/test/blackbox/common/DDSBlackboxTestsListeners.cpp b/test/blackbox/common/DDSBlackboxTestsListeners.cpp index 513de562d92..4c1ae522a6e 100644 --- a/test/blackbox/common/DDSBlackboxTestsListeners.cpp +++ b/test/blackbox/common/DDSBlackboxTestsListeners.cpp @@ -3436,6 +3436,46 @@ TEST(DDSStatus, keyed_reliable_positive_acks_disabled_on_unack_sample_removed) delete dummy_data; } +/*! + * Regression Test for 22658: when the entire history is acked in volatile, given that the entries are deleted from the + * history, check_acked_status satisfies min_low_mark >= get_seq_num_min() because seq_num_min is unknown. This makes + * try_remove to fail, because it tries to remove changes but there were none. This causes prepare_change to not + * perform the changes, since the history was full and could not delete any changes. + */ + +TEST(DDSStatus, entire_history_acked_volatile_unknown_pointer) +{ + PubSubWriter writer(TEST_TOPIC_NAME); + PubSubReader reader(TEST_TOPIC_NAME); + + writer.reliability(eprosima::fastdds::dds::RELIABLE_RELIABILITY_QOS, eprosima::fastdds::dds::Duration_t (200, 0)) + .durability_kind(eprosima::fastdds::dds::VOLATILE_DURABILITY_QOS) + .history_kind(eprosima::fastdds::dds::KEEP_ALL_HISTORY_QOS) + .resource_limits_max_instances(1) + .resource_limits_max_samples(1) + .resource_limits_max_samples_per_instance(1) + .init(); + ASSERT_TRUE(writer.isInitialized()); + + reader.reliability(eprosima::fastdds::dds::RELIABLE_RELIABILITY_QOS) + .durability_kind(eprosima::fastdds::dds::VOLATILE_DURABILITY_QOS) + .init(); + ASSERT_TRUE(reader.isInitialized()); + + // Wait for discovery + writer.wait_discovery(); + reader.wait_discovery(); + + auto data = default_helloworld_data_generator(2); + for (auto sample : data) + { + // A value of true means that the sample was sent successfully. + // This aligns with the expected behaviour of having the history + // acknowledged and emptied before the next message. + EXPECT_TRUE(writer.send_sample(sample)); + } +} + /*! * Test that checks with a writer of each type that having the same listener attached, the notified writer in the * callback is the corresponding writer that has removed a sample unacknowledged.