Check History QoS inconsistencies (#4375)

* Refs #20401: Add regression test Signed-off-by: JesusPoderoso <jesuspoderoso@eprosima.com> * Refs #20401: Check History QoS inconsistencies Signed-off-by: JesusPoderoso <jesuspoderoso@eprosima.com> * Refs #20401: Check also the history depth vs resource limits max_sample_per_instance bounds Signed-off-by: JesusPoderoso <jesuspoderoso@eprosima.com> * Refs #20401: Update HeartbeatWhileDestruction test Signed-off-by: JesusPoderoso <jesuspoderoso@eprosima.com> * Refs #20401: Update unit test DDS profiles XML tests profile Signed-off-by: JesusPoderoso <jesuspoderoso@eprosima.com> --------- Signed-off-by: JesusPoderoso <jesuspoderoso@eprosima.com> (cherry picked from commit 68acb5a)
eProsima · Mar 5, 2024 · ba2aa6f · ba2aa6f
1 parent 5ed912b
commit ba2aa6f
Show file tree

Hide file tree

Showing 6 changed files with 60 additions and 7 deletions.
diff --git a/src/cpp/fastdds/publisher/DataWriterImpl.cpp b/src/cpp/fastdds/publisher/DataWriterImpl.cpp
@@ -1831,6 +1831,19 @@ ReturnCode_t DataWriterImpl::check_qos(
         EPROSIMA_LOG_ERROR(RTPS_QOS_CHECK, "DATA_SHARING cannot be used with memory policies other than PREALLOCATED.");
         return ReturnCode_t::RETCODE_INCONSISTENT_POLICY;
     }
+    if (qos.history().kind == KEEP_LAST_HISTORY_QOS && qos.history().depth <= 0)
+    {
+        EPROSIMA_LOG_ERROR(RTPS_QOS_CHECK, "HISTORY DEPTH must be higher than 0 if HISTORY KIND is KEEP_LAST.");
+        return ReturnCode_t::RETCODE_INCONSISTENT_POLICY;
+    }
+    if (qos.history().kind == KEEP_LAST_HISTORY_QOS && qos.history().depth > 0 &&
+            qos.resource_limits().max_samples_per_instance > 0 &&
+            qos.history().depth > qos.resource_limits().max_samples_per_instance)
+    {
+        EPROSIMA_LOG_ERROR(RTPS_QOS_CHECK,
+                "HISTORY DEPTH must be lower or equal to the max_samples_per_instance value.");
+        return ReturnCode_t::RETCODE_INCONSISTENT_POLICY;
+    }
     return ReturnCode_t::RETCODE_OK;
 }
 

diff --git a/src/cpp/fastdds/subscriber/DataReaderImpl.cpp b/src/cpp/fastdds/subscriber/DataReaderImpl.cpp
@@ -1457,6 +1457,19 @@ ReturnCode_t DataReaderImpl::check_qos(
         EPROSIMA_LOG_ERROR(DDS_QOS_CHECK, "unique_network_request cannot be set along specific locators");
         return ReturnCode_t::RETCODE_INCONSISTENT_POLICY;
     }
+    if (qos.history().kind == KEEP_LAST_HISTORY_QOS && qos.history().depth <= 0)
+    {
+        EPROSIMA_LOG_ERROR(RTPS_QOS_CHECK, "HISTORY DEPTH must be higher than 0 if HISTORY KIND is KEEP_LAST.");
+        return ReturnCode_t::RETCODE_INCONSISTENT_POLICY;
+    }
+    if (qos.history().kind == KEEP_LAST_HISTORY_QOS && qos.history().depth > 0 &&
+            qos.resource_limits().max_samples_per_instance > 0 &&
+            qos.history().depth > qos.resource_limits().max_samples_per_instance)
+    {
+        EPROSIMA_LOG_ERROR(RTPS_QOS_CHECK,
+                "HISTORY DEPTH must be lower or equal to the max_samples_per_instance value.");
+        return ReturnCode_t::RETCODE_INCONSISTENT_POLICY;
+    }
     return ReturnCode_t::RETCODE_OK;
 }
 

diff --git a/test/blackbox/common/DDSBlackboxTestsDataWriter.cpp b/test/blackbox/common/DDSBlackboxTestsDataWriter.cpp
@@ -380,6 +380,7 @@ TEST(DDSDataWriter, OfferedDeadlineMissedListener)
  *   - 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)
+ * Edit: this test has been updated to ensure that HistoryQoS and ResourceLimitQoS constraints are met (#20401).
  */
 TEST(DDSDataWriter, HeartbeatWhileDestruction)
 {
@@ -392,13 +393,21 @@ TEST(DDSDataWriter, HeartbeatWhileDestruction)
         // 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();
+        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<int32_t>(n_samples)).heartbeat_period_seconds(0).
-                heartbeat_period_nanosec(
-            20 * 1000).init();
+        writer.reliability(RELIABLE_RELIABILITY_QOS)
+                .durability_kind(TRANSIENT_LOCAL_DURABILITY_QOS)
+                .history_kind(KEEP_LAST_HISTORY_QOS)
+                .history_depth(static_cast<int32_t>(n_samples))
+                .resource_limits_max_samples(static_cast<int32_t>(n_samples))
+                .resource_limits_max_instances(static_cast<int32_t>(1))
+                .resource_limits_max_samples_per_instance(static_cast<int32_t>(n_samples))
+                .heartbeat_period_seconds(0)
+                .heartbeat_period_nanosec(20 * 1000)
+                .init();
         ASSERT_TRUE(writer.isInitialized());
 
         reader.wait_discovery();

diff --git a/test/unittest/dds/profiles/test_xml_profile.xml b/test/unittest/dds/profiles/test_xml_profile.xml
@@ -457,16 +457,17 @@
             </matchedSubscribersAllocation>
         </data_writer>
 
+        <!-- This profile has been updated to ensure that HistoryQoS and ResourceLimitQoS constraints are met (#20401) -->
         <data_reader profile_name="test_subscriber_profile">
             <topic>
                 <historyQos>
                     <kind>KEEP_LAST</kind>
                     <depth>500</depth>
                 </historyQos>
                 <resourceLimitsQos>
-                    <max_samples>10</max_samples>
+                    <max_samples>2500</max_samples>
                     <max_instances>5</max_instances>
-                    <max_samples_per_instance>2</max_samples_per_instance>
+                    <max_samples_per_instance>500</max_samples_per_instance>
                     <allocated_samples>10</allocated_samples>
                 </resourceLimitsQos>
             </topic>

diff --git a/test/unittest/dds/publisher/DataWriterTests.cpp b/test/unittest/dds/publisher/DataWriterTests.cpp
@@ -671,6 +671,15 @@ TEST(DataWriterTests, InvalidQos)
     qos.endpoint().history_memory_policy = eprosima::fastrtps::rtps::PREALLOCATED_WITH_REALLOC_MEMORY_MODE;
     EXPECT_EQ(ReturnCode_t::RETCODE_OK, datawriter->set_qos(qos));
 
+    qos = DATAWRITER_QOS_DEFAULT;
+    qos.history().kind = KEEP_LAST_HISTORY_QOS;
+    qos.history().depth = 0;
+    EXPECT_EQ(inconsistent_code, datawriter->set_qos(qos)); // KEEP LAST 0 is inconsistent
+    qos.history().depth = 2;
+    EXPECT_EQ(ReturnCode_t::RETCODE_OK, datawriter->set_qos(qos)); // KEEP LAST 2 is OK
+    qos.resource_limits().max_samples_per_instance = 1;
+    EXPECT_EQ(inconsistent_code, datawriter->set_qos(qos)); // KEEP LAST 2 but max_samples_per_instance 1 is inconsistent
+
     ASSERT_TRUE(publisher->delete_datawriter(datawriter) == ReturnCode_t::RETCODE_OK);
     ASSERT_TRUE(participant->delete_topic(topic) == ReturnCode_t::RETCODE_OK);
     ASSERT_TRUE(participant->delete_publisher(publisher) == ReturnCode_t::RETCODE_OK);

diff --git a/test/unittest/dds/subscriber/DataReaderTests.cpp b/test/unittest/dds/subscriber/DataReaderTests.cpp
@@ -707,6 +707,14 @@ TEST_F(DataReaderTests, InvalidQos)
     qos.properties().properties().emplace_back("fastdds.unique_network_flows", "");
     EXPECT_EQ(inconsistent_code, data_reader_->set_qos(qos));
 
+    qos = DATAREADER_QOS_DEFAULT;
+    qos.history().kind = KEEP_LAST_HISTORY_QOS;
+    qos.history().depth = 0;
+    EXPECT_EQ(inconsistent_code, data_reader_->set_qos(qos)); // KEEP LAST 0 is inconsistent
+    qos.history().depth = 2;
+    qos.resource_limits().max_samples_per_instance = 1;
+    EXPECT_EQ(inconsistent_code, data_reader_->set_qos(qos)); // KEEP LAST 2 but max_samples_per_instance 1 is inconsistent
+
     /* Inmutable QoS */
     const ReturnCode_t inmutable_code = ReturnCode_t::RETCODE_IMMUTABLE_POLICY;