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 <[email protected]> * Refs #22658: Fix bug Signed-off-by: Juanjo Garcia <[email protected]> * Refs #22658: corrected failing CI Signed-off-by: Juanjo Garcia <[email protected]> * Refs #22658: Included reviewer suggestions Signed-off-by: Juanjo Garcia <[email protected]> --------- Signed-off-by: Juanjo Garcia <[email protected]> (cherry picked from commit 90790ce) * Fix namespace Signed-off-by: Eugenio Collado <[email protected]> --------- Signed-off-by: Eugenio Collado <[email protected]> Co-authored-by: juanjo4936 <[email protected]> Co-authored-by: Eugenio Collado <[email protected]>
eProsima · Feb 4, 2025 · bc53ad5 · bc53ad5
1 parent 3dbb3c0
commit bc53ad5
Show file tree

Hide file tree

Showing 3 changed files with 52 additions and 1 deletion.
diff --git 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
@@ -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
@@ -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<HelloWorldPubSubType> writer(TEST_TOPIC_NAME);
+    PubSubReader<HelloWorldPubSubType> 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.