Just show warning when inconsistency between depth and max_samples_pe…

…r_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>
(cherry picked from commit 12201f4)

src/cpp/fastdds/publisher/DataWriterImpl.cpp

@@ -1826,9 +1826,11 @@ ReturnCode_t DataWriterImpl::check_qos(
             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;
+        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;
 }

src/cpp/fastdds/subscriber/DataReaderImpl.cpp

@@ -1466,9 +1466,11 @@ ReturnCode_t DataReaderImpl::check_qos(
             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;
+        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;
 }

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

test/blackbox/common/DDSBlackboxTestsDataWriter.cpp

@@ -421,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

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>
@@ -36,14 +40,13 @@
 #include <fastdds/dds/subscriber/qos/DataReaderQos.hpp>
 #include <fastdds/dds/subscriber/qos/SubscriberQos.hpp>
 #include <fastdds/dds/subscriber/Subscriber.hpp>
-#include <fastdds/publisher/DataWriterImpl.hpp>
 #include <fastdds/rtps/writer/RTPSWriter.h>
 #include <fastdds/rtps/writer/StatefulWriter.h>
 
 #include <fastdds/publisher/DataWriterImpl.hpp>
 
-#include "../../logging/mock/MockConsumer.h"
 #include "../../common/CustomPayloadPool.hpp"
+#include "../../logging/mock/MockConsumer.h"
 
 #include <mutex>
 #include <condition_variable>
@@ -681,8 +684,10 @@ TEST(DataWriterTests, InvalidQos)
     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
+    // 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);
@@ -2006,6 +2011,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

test/unittest/dds/subscriber/DataReaderTests.cpp

@@ -15,65 +15,58 @@
 #include <array>
 #include <cassert>
 #include <chrono>
+#include <cstdint>
 #include <forward_list>
 #include <iostream>
 #include <memory>
 #include <sstream>
 #include <thread>
 #include <type_traits>
 
+#include <asio.hpp>
+
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
 
 #include <fastcdr/Cdr.h>
 
 #include <fastdds/dds/builtin/topic/PublicationBuiltinTopicData.hpp>
-
+#include <fastdds/dds/core/condition/WaitSet.hpp>
 #include <fastdds/dds/core/Entity.hpp>
 #include <fastdds/dds/core/LoanableArray.hpp>
 #include <fastdds/dds/core/LoanableCollection.hpp>
 #include <fastdds/dds/core/LoanableSequence.hpp>
 #include <fastdds/dds/core/StackAllocatedSequence.hpp>
-#include <fastdds/dds/core/condition/WaitSet.hpp>
 #include <fastdds/dds/core/status/BaseStatus.hpp>
 #include <fastdds/dds/core/status/SampleRejectedStatus.hpp>
 #include <fastdds/dds/core/status/SubscriptionMatchedStatus.hpp>
-
-#include <fastdds/dds/domain/DomainParticipantFactory.hpp>
 #include <fastdds/dds/domain/DomainParticipant.hpp>
+#include <fastdds/dds/domain/DomainParticipantFactory.hpp>
 #include <fastdds/dds/domain/DomainParticipantListener.hpp>
-
+#include <fastdds/dds/log/Log.hpp>
 #include <fastdds/dds/publisher/DataWriter.hpp>
 #include <fastdds/dds/publisher/Publisher.hpp>
 #include <fastdds/dds/publisher/qos/DataWriterQos.hpp>
 #include <fastdds/dds/publisher/qos/PublisherQos.hpp>
-
 #include <fastdds/dds/subscriber/DataReader.hpp>
 #include <fastdds/dds/subscriber/DataReaderListener.hpp>
-#include <fastdds/dds/subscriber/SampleInfo.hpp>
-#include <fastdds/dds/subscriber/Subscriber.hpp>
 #include <fastdds/dds/subscriber/qos/DataReaderQos.hpp>
 #include <fastdds/dds/subscriber/qos/SubscriberQos.hpp>
-
+#include <fastdds/dds/subscriber/SampleInfo.hpp>
+#include <fastdds/dds/subscriber/Subscriber.hpp>
 #include <fastdds/rtps/common/Locator.h>
-#include <fastrtps/utils/IPLocator.h>
-
-#include "FooBoundedType.hpp"
-#include "FooBoundedTypeSupport.hpp"
-
-#include "FooType.hpp"
-#include "FooTypeSupport.hpp"
-
-#include "../../logging/mock/MockConsumer.h"
-
 #include <fastdds/rtps/transport/test_UDPv4TransportDescriptor.h>
+#include <fastrtps/utils/IPLocator.h>
 #include <fastrtps/xmlparser/XMLProfileManager.h>
 
 #include "../../common/CustomPayloadPool.hpp"
+#include "../../logging/mock/MockConsumer.h"
 #include "fastdds/dds/common/InstanceHandle.hpp"
 #include "fastdds/dds/core/policy/QosPolicies.hpp"
-
-#include <asio.hpp>
+#include "FooBoundedType.hpp"
+#include "FooBoundedTypeSupport.hpp"
+#include "FooType.hpp"
+#include "FooTypeSupport.hpp"
 
 #if defined(__cplusplus_winrt)
 #define GET_PID GetCurrentProcessId
@@ -701,9 +694,13 @@ TEST_F(DataReaderTests, InvalidQos)
     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
+    // KEEP LAST 2000 but max_samples_per_instance default (400) is inconsistent but right now it only shows a warning
+    // In the reader, this returns RETCODE_INMUTABLE_POLICY, because the depth cannot be changed on run time.
+    // Because of the implementation, we know de consistency is checked before the inmutability, so by checking the
+    // return against RETCODE_INMUTABLE_POLICY we are testing that the setting are not considered inconsistent yet.
+    // 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_IMMUTABLE_POLICY, data_reader_->set_qos(qos));
 
     /* Inmutable QoS */
     const ReturnCode_t inmutable_code = ReturnCode_t::RETCODE_IMMUTABLE_POLICY;
@@ -3542,6 +3539,71 @@ TEST_F(DataReaderTests, CustomPoolCreation)
     DomainParticipantFactory::get_instance()->delete_participant(participant);
 }
 
+TEST_F(DataReaderTests, 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 subscriber */
+    DomainParticipant* participant = DomainParticipantFactory::get_instance()->create_participant(0,
+                    PARTICIPANT_QOS_DEFAULT);
+    ASSERT_NE(participant, nullptr);
+
+    TypeSupport type(new FooTypeSupport());
+    type.register_type(participant);
+
+    Topic* topic = participant->create_topic("footopic", type.get_type_name(), TOPIC_QOS_DEFAULT);
+    ASSERT_NE(topic, nullptr);
+
+    Subscriber* subscriber = participant->create_subscriber(SUBSCRIBER_QOS_DEFAULT);
+    ASSERT_NE(subscriber, nullptr);
+
+    /* Create a datareader with the QoS that should generate a warning */
+    DataReaderQos qos;
+    qos.history().depth = 10;
+    qos.resource_limits().max_samples_per_instance = 5;
+    DataReader* datareader_1 = subscriber->create_datareader(topic, qos);
+    ASSERT_NE(datareader_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 a correctly initialized datareader does not produce any warning */
+    qos.history().depth = 10;
+    qos.resource_limits().max_samples_per_instance = 10;
+    DataReader* datareader_2 = subscriber->create_datareader(topic, qos);
+    ASSERT_NE(datareader_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();
+}
+
 int main(
         int argc,
         char** argv)