From bc53ad5ba0a4ceddeb1eb5dea0d6a9d5c678981d Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Tue, 4 Feb 2025 16:21:49 +0100 Subject: [PATCH] Reliable volatile change dropped when all history acked (#5606) (#5615) * 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) * Fix namespace Signed-off-by: Eugenio Collado --------- Signed-off-by: Eugenio Collado Co-authored-by: juanjo4936 <69901369+juanjo4936@users.noreply.github.com> Co-authored-by: Eugenio Collado --- 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 fb32a87a9a3..7d585010a52 100644 --- a/src/cpp/rtps/writer/StatefulWriter.cpp +++ b/src/cpp/rtps/writer/StatefulWriter.cpp @@ -1576,7 +1576,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 f03c5cb84c4..b734bf39d7b 100644 --- a/test/blackbox/api/dds-pim/PubSubWriter.hpp +++ b/test/blackbox/api/dds-pim/PubSubWriter.hpp @@ -862,6 +862,15 @@ class PubSubWriter return *this; } + PubSubWriter& reliability( + const eprosima::fastdds::dds::ReliabilityQosPolicyKind kind, + eprosima::fastrtps::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::fastrtps::rtps::MemoryManagementPolicy mem_policy) { diff --git a/test/blackbox/common/DDSBlackboxTestsListeners.cpp b/test/blackbox/common/DDSBlackboxTestsListeners.cpp index 9304681085a..26fff170ec1 100644 --- a/test/blackbox/common/DDSBlackboxTestsListeners.cpp +++ b/test/blackbox/common/DDSBlackboxTestsListeners.cpp @@ -3388,6 +3388,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::fastrtps::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.