Fix inserting minimum CacheChange_t in GAP message (eProsima#2448)

* Refs #13556. Fix bug when history's min seq is irrelevant Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev> * Refs #13556. Add regression test. Signed-off-by: Ricardo González Moreno <ricardo@richiware.dev>
simonmcqueen · Feb 4, 2022 · df8dfde · df8dfde
1 parent 543825d
commit df8dfde
Show file tree

Hide file tree

Showing 5 changed files with 41 additions and 2 deletions.
diff --git a/src/cpp/rtps/writer/ReaderProxy.cpp b/src/cpp/rtps/writer/ReaderProxy.cpp
@@ -408,7 +408,7 @@ bool ReaderProxy::requested_changes_set(
                             isSomeoneWasSetRequested = true;
                         }
                     }
-                    else if ((sit > min_seq_in_history) && (sit > changes_low_mark_))
+                    else if ((sit >= min_seq_in_history) && (sit > changes_low_mark_))
                     {
                         gap_builder.add(sit);
                     }

diff --git a/test/mock/rtps/ReaderLocator/fastdds/rtps/writer/ReaderLocator.h b/test/mock/rtps/ReaderLocator/fastdds/rtps/writer/ReaderLocator.h
@@ -28,6 +28,7 @@
 #include <fastrtps/rtps/common/SequenceNumber.h>
 #include <fastrtps/rtps/messages/RTPSMessageGroup.h>
 #include <fastrtps/rtps/common/LocatorSelectorEntry.hpp>
+#include <fastrtps/rtps/messages/RTPSMessageSenderInterface.hpp>
 
 
 namespace eprosima {

diff --git a/test/mock/rtps/StatefulWriter/fastdds/rtps/writer/StatefulWriter.h b/test/mock/rtps/StatefulWriter/fastdds/rtps/writer/StatefulWriter.h
@@ -85,7 +85,7 @@ class StatefulWriter : public RTPSWriter
 
     SequenceNumber_t get_seq_num_min()
     {
-        return SequenceNumber_t(0, 0);
+        return SequenceNumber_t::unknown();
     }
 
     SequenceNumber_t next_sequence_number() const

diff --git a/test/unittest/rtps/writer/CMakeLists.txt b/test/unittest/rtps/writer/CMakeLists.txt
@@ -48,6 +48,7 @@ target_include_directories(ReaderProxyTests PRIVATE
     ${PROJECT_SOURCE_DIR}/test/mock/dds/QosPolicies
     ${PROJECT_SOURCE_DIR}/test/mock/rtps/ReaderLocator
     ${PROJECT_SOURCE_DIR}/test/mock/rtps/RTPSGapBuilder
+    ${PROJECT_SOURCE_DIR}/test/mock/rtps/RTPSMessageGroup
     ${PROJECT_SOURCE_DIR}/test/mock/rtps/TimedEvent
     ${PROJECT_SOURCE_DIR}/include
     ${PROJECT_BINARY_DIR}/include

diff --git a/test/unittest/rtps/writer/ReaderProxyTests.cpp b/test/unittest/rtps/writer/ReaderProxyTests.cpp
@@ -17,6 +17,7 @@
 
 #include <fastrtps/rtps/writer/ReaderProxy.h>
 #include <fastrtps/rtps/writer/StatefulWriter.h>
+#include <rtps/messages/RTPSGapBuilder.hpp>
 
 //using namespace eprosima::fastrtps::rtps;
 namespace eprosima {
@@ -111,6 +112,42 @@ TEST(ReaderProxyTests, find_change_removed_test)
     ASSERT_FALSE(rproxy.change_is_acked(SequenceNumber_t(0, 4)));
 }
 
+// Regression test for #13556 (Github #2423)
+TEST(ReaderProxyTests, requested_changes_set_test)
+{
+    StatefulWriter writerMock;
+    WriterTimes wTimes;
+    RemoteLocatorsAllocationAttributes alloc;
+    ReaderProxy rproxy(wTimes, alloc, &writerMock);
+    CacheChange_t seq1; seq1.sequenceNumber = {0, 1};
+    CacheChange_t seq2; seq2.sequenceNumber = {0, 2};
+    CacheChange_t seq3; seq3.sequenceNumber = {0, 3};
+    CacheChange_t seq4; seq4.sequenceNumber = {0, 4};
+    RTPSMessageGroup message_group(nullptr, false);
+    RTPSGapBuilder gap_builder(message_group);
+
+    ReaderProxyData reader_attributes(0, 0);
+    reader_attributes.m_qos.m_reliability.kind = RELIABLE_RELIABILITY_QOS;
+    rproxy.start(reader_attributes);
+
+
+    rproxy.add_change(ChangeForReader_t(&seq1), false, false);
+    rproxy.add_change(ChangeForReader_t(&seq2), true, false);
+    rproxy.add_change(ChangeForReader_t(&seq3), true, false);
+    rproxy.add_change(ChangeForReader_t(&seq4), false, false);
+
+    SequenceNumberSet_t set({0, 1});
+    set.add({0, 1});
+    set.add({0, 2});
+    set.add({0, 3});
+    set.add({0, 4});
+
+    EXPECT_CALL(gap_builder, add(SequenceNumber_t(0, 1))).Times(1).WillOnce(testing::Return(true));
+    EXPECT_CALL(gap_builder, add(SequenceNumber_t(0, 4))).Times(1).WillOnce(testing::Return(true));
+
+    rproxy.requested_changes_set(set, gap_builder, {0, 1});
+}
+
 } // namespace rtps
 } // namespace fastrtps
 } // namespace eprosima