Check History QoS inconsistencies (#4375) (#4406)

* 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) * Fix SecureHelloworldExample (#4416) Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> * Just show warning when inconsistency between depth and max_samples_per_instance (#4417) * Refs #20503. Add regression test Signed-off-by: EduPonz <eduardoponz@eprosima.com> * Refs #20503. Show warning when depth > max_samples_per_instance Signed-off-by: EduPonz <eduardoponz@eprosima.com> * Refs #20503. Fix InvalidQos tests Signed-off-by: EduPonz <eduardoponz@eprosima.com> --------- Signed-off-by: EduPonz <eduardoponz@eprosima.com> Co-authored-by: EduPonz <eduardoponz@eprosima.com> --------- Signed-off-by: Mario Dominguez <mariodominguez@eprosima.com> Signed-off-by: EduPonz <eduardoponz@eprosima.com> Co-authored-by: Jesús Poderoso <120394830+JesusPoderoso@users.noreply.github.com> Co-authored-by: Mario Domínguez López <116071334+Mario-DL@users.noreply.github.com> Co-authored-by: Miguel Company <miguelcompany@eprosima.com> Co-authored-by: EduPonz <eduardoponz@eprosima.com>
eProsima · Mar 6, 2024 · cf03b85 · cf03b85
1 parent a8117be
commit cf03b85
Show file tree

Hide file tree

Showing 8 changed files with 246 additions and 31 deletions.
diff --git a/examples/cpp/dds/SecureHelloWorldExample/HelloWorldPublisher.cpp b/examples/cpp/dds/SecureHelloWorldExample/HelloWorldPublisher.cpp
@@ -88,7 +88,7 @@ bool HelloWorldPublisher::init()
     DataWriterQos wqos;
     wqos.reliability().kind = RELIABLE_RELIABILITY_QOS;
     wqos.history().kind = KEEP_LAST_HISTORY_QOS;
-    wqos.history().depth = 30;
+    wqos.history().depth = 20;
     wqos.resource_limits().max_samples = 50;
     wqos.resource_limits().max_samples_per_instance = 20;
     wqos.reliable_writer_qos().times.heartbeatPeriod.seconds = 2;

diff --git a/src/cpp/fastdds/publisher/DataWriterImpl.cpp b/src/cpp/fastdds/publisher/DataWriterImpl.cpp
@@ -1831,6 +1831,21 @@ 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_WARNING(RTPS_QOS_CHECK,
+                "HISTORY DEPTH '" << qos.history().depth <<
+                "' is inconsistent with max_samples_per_instance: '" << qos.resource_limits().max_samples_per_instance <<
+                "'. Consistency rule: depth <= max_samples_per_instance." <<
+                " Effectively using max_samples_per_instance as depth.");
+    }
     return ReturnCode_t::RETCODE_OK;
 }
 

diff --git a/src/cpp/fastdds/subscriber/DataReaderImpl.cpp b/src/cpp/fastdds/subscriber/DataReaderImpl.cpp
@@ -1457,6 +1457,21 @@ 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_WARNING(RTPS_QOS_CHECK,
+                "HISTORY DEPTH '" << qos.history().depth <<
+                "' is inconsistent with max_samples_per_instance: '" << qos.resource_limits().max_samples_per_instance <<
+                "'. Consistency rule: depth <= max_samples_per_instance." <<
+                " Effectively using max_samples_per_instance as depth.");
+    }
     return ReturnCode_t::RETCODE_OK;
 }
 

diff --git a/test/blackbox/common/DDSBlackboxTestsDataReader.cpp b/test/blackbox/common/DDSBlackboxTestsDataReader.cpp
@@ -297,6 +297,17 @@ TEST(DDSDataReader, ConsistentReliabilityWhenIntraprocess)
     xmlparser::XMLProfileManager::library_settings(library_settings);
 }
 
+/**
+ * This is a regression test for issue https://eprosima.easyredmine.com/issues/20504.
+ * It checks that a DataReader be created with default Qos and a large history depth.
+ */
+TEST(DDSDataReader, default_qos_large_history_depth)
+{
+    PubSubReader<HelloWorldPubSubType> reader(TEST_TOPIC_NAME);
+    reader.history_depth(1000).init();
+    ASSERT_TRUE(reader.isInitialized());
+}
+
 #ifdef INSTANTIATE_TEST_SUITE_P
 #define GTEST_INSTANTIATE_TEST_MACRO(x, y, z, w) INSTANTIATE_TEST_SUITE_P(x, y, z, w)
 #else

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();
@@ -412,6 +421,17 @@ TEST(DDSDataWriter, HeartbeatWhileDestruction)
     }
 }
 
+/**
+ * This is a regression test for issue https://eprosima.easyredmine.com/issues/20504.
+ * It checks that a DataWriter be created with default Qos and a large history depth.
+ */
+TEST(DDSDataWriter, default_qos_large_history_depth)
+{
+    PubSubWriter<HelloWorldPubSubType> writer(TEST_TOPIC_NAME);
+    writer.history_depth(1000).init();
+    ASSERT_TRUE(writer.isInitialized());
+}
+
 #ifdef INSTANTIATE_TEST_SUITE_P
 #define GTEST_INSTANTIATE_TEST_MACRO(x, y, z, w) INSTANTIATE_TEST_SUITE_P(x, y, z, w)
 #else

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
@@ -12,7 +12,10 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+#include <chrono>
 #include <condition_variable>
+#include <cstdint>
+#include <memory>
 #include <mutex>
 #include <thread>
 
@@ -28,6 +31,7 @@
 #include <fastdds/dds/domain/DomainParticipantFactory.hpp>
 #include <fastdds/dds/domain/DomainParticipantListener.hpp>
 #include <fastdds/dds/domain/qos/DomainParticipantQos.hpp>
+#include <fastdds/dds/log/Log.hpp>
 #include <fastdds/dds/publisher/DataWriter.hpp>
 #include <fastdds/dds/publisher/DataWriterListener.hpp>
 #include <fastdds/dds/publisher/Publisher.hpp>
@@ -40,9 +44,8 @@
 #include <fastdds/rtps/writer/RTPSWriter.h>
 #include <fastdds/rtps/writer/StatefulWriter.h>
 
-#include "../../logging/mock/MockConsumer.h"
-
 #include "../../common/CustomPayloadPool.hpp"
+#include "../../logging/mock/MockConsumer.h"
 
 namespace eprosima {
 namespace fastdds {
@@ -671,6 +674,17 @@ 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
+    // KEEP LAST 2000 but max_samples_per_instance default (400) is inconsistent but right now it only shows a warning
+    // This test will fail whenever we enforce the consistency between depth and max_samples_per_instance.
+    qos.history().depth = 2000;
+    EXPECT_EQ(ReturnCode_t::RETCODE_OK, datawriter->set_qos(qos));
+
     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);
@@ -1991,6 +2005,75 @@ TEST(DataWriterTests, CustomPoolCreation)
     DomainParticipantFactory::get_instance()->delete_participant(participant);
 }
 
+TEST(DataWriterTests, history_depth_max_samples_per_instance_warning)
+{
+
+    /* Setup log so it may catch the expected warning */
+    Log::ClearConsumers();
+    MockConsumer* mockConsumer = new MockConsumer("RTPS_QOS_CHECK");
+    Log::RegisterConsumer(std::unique_ptr<LogConsumer>(mockConsumer));
+    Log::SetVerbosity(Log::Warning);
+
+    /* Create a participant, topic, and a publisher */
+    DomainParticipant* participant = DomainParticipantFactory::get_instance()->create_participant(0,
+                    PARTICIPANT_QOS_DEFAULT);
+    ASSERT_NE(participant, nullptr);
+
+    TypeSupport type(new TopicDataTypeMock());
+    type.register_type(participant);
+
+    Topic* topic = participant->create_topic("footopic", type.get_type_name(), TOPIC_QOS_DEFAULT);
+    ASSERT_NE(topic, nullptr);
+
+    Publisher* publisher = participant->create_publisher(PUBLISHER_QOS_DEFAULT);
+    ASSERT_NE(publisher, nullptr);
+
+    /* Create a datawriter with the QoS that should generate a warning */
+    DataWriterQos qos;
+    qos.history().depth = 10;
+    qos.resource_limits().max_samples_per_instance = 5;
+    DataWriter* datawriter_1 = publisher->create_datawriter(topic, qos);
+    ASSERT_NE(datawriter_1, nullptr);
+
+    /* Check that the config generated a warning */
+    auto wait_for_log_entries =
+            [&mockConsumer](const uint32_t amount, const uint32_t retries, const uint32_t wait_ms) -> size_t
+            {
+                size_t entries = 0;
+                for (uint32_t i = 0; i < retries; i++)
+                {
+                    entries = mockConsumer->ConsumedEntries().size();
+                    if (entries >= amount)
+                    {
+                        break;
+                    }
+                    std::this_thread::sleep_for(std::chrono::milliseconds(wait_ms));
+                }
+                return entries;
+            };
+
+    const size_t expected_entries = 1;
+    const uint32_t retries = 4;
+    const uint32_t wait_ms = 25;
+    ASSERT_EQ(wait_for_log_entries(expected_entries, retries, wait_ms), expected_entries);
+
+    /* Check that the datawriter can send data */
+    FooType data;
+    ASSERT_EQ(ReturnCode_t::RETCODE_OK, datawriter_1->write(&data, HANDLE_NIL));
+
+    /* Check that a correctly initialized writer does not produce any warning */
+    qos.history().depth = 10;
+    qos.resource_limits().max_samples_per_instance = 10;
+    DataWriter* datawriter_2 = publisher->create_datawriter(topic, qos);
+    ASSERT_NE(datawriter_2, nullptr);
+    ASSERT_EQ(wait_for_log_entries(expected_entries, retries, wait_ms), expected_entries);
+
+    /* Tear down */
+    participant->delete_contained_entities();
+    DomainParticipantFactory::get_instance()->delete_participant(participant);
+    Log::KillThread();
+}
+
 } // namespace dds
 } // namespace fastdds
 } // namespace eprosima