From 822f237e69ad2430526747af654be5805f349daf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jes=C3=BAs=20Poderoso?= <120394830+JesusPoderoso@users.noreply.github.com> Date: Tue, 20 Feb 2024 14:33:22 +0100 Subject: [PATCH 1/6] Check History QoS inconsistencies (#4375) * Refs #20401: Add regression test Signed-off-by: JesusPoderoso * Refs #20401: Check History QoS inconsistencies Signed-off-by: JesusPoderoso * Refs #20401: Check also the history depth vs resource limits max_sample_per_instance bounds Signed-off-by: JesusPoderoso * Refs #20401: Update HeartbeatWhileDestruction test Signed-off-by: JesusPoderoso * Refs #20401: Update unit test DDS profiles XML tests profile Signed-off-by: JesusPoderoso --------- Signed-off-by: JesusPoderoso (cherry picked from commit 68acb5a6cc9b45010f555af932b2f4a475362ab3) # Conflicts: # src/cpp/fastdds/publisher/DataWriterImpl.cpp # src/cpp/fastdds/subscriber/DataReaderImpl.cpp # test/unittest/dds/profiles/test_xml_profiles.xml --- src/cpp/fastdds/publisher/DataWriterImpl.cpp | 38 +++++++++++++++++++ src/cpp/fastdds/subscriber/DataReaderImpl.cpp | 38 +++++++++++++++++++ .../common/DDSBlackboxTestsDataWriter.cpp | 19 +++++++--- .../dds/profiles/test_xml_profiles.xml | 11 +++++- .../dds/publisher/DataWriterTests.cpp | 9 +++++ .../dds/subscriber/DataReaderTests.cpp | 8 ++++ 6 files changed, 116 insertions(+), 7 deletions(-) diff --git a/src/cpp/fastdds/publisher/DataWriterImpl.cpp b/src/cpp/fastdds/publisher/DataWriterImpl.cpp index 97dd80a4803..4bef7dbf4bd 100644 --- a/src/cpp/fastdds/publisher/DataWriterImpl.cpp +++ b/src/cpp/fastdds/publisher/DataWriterImpl.cpp @@ -1701,7 +1701,45 @@ ReturnCode_t DataWriterImpl::check_qos( (qos.endpoint().history_memory_policy != PREALLOCATED_MEMORY_MODE && qos.endpoint().history_memory_policy != PREALLOCATED_WITH_REALLOC_MEMORY_MODE)) { +<<<<<<< HEAD logError(RTPS_QOS_CHECK, "DATA_SHARING cannot be used with memory policies other than PREALLOCATED."); +======= + 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; +} + +ReturnCode_t DataWriterImpl::check_allocation_consistency( + const DataWriterQos& qos) +{ + if ((qos.resource_limits().max_samples > 0) && + (qos.resource_limits().max_samples < + (qos.resource_limits().max_instances * qos.resource_limits().max_samples_per_instance))) + { + EPROSIMA_LOG_ERROR(DDS_QOS_CHECK, + "max_samples should be greater than max_instances * max_samples_per_instance"); + return ReturnCode_t::RETCODE_INCONSISTENT_POLICY; + } + if ((qos.resource_limits().max_instances <= 0 || qos.resource_limits().max_samples_per_instance <= 0) && + (qos.resource_limits().max_samples > 0)) + { + EPROSIMA_LOG_ERROR(DDS_QOS_CHECK, + "max_samples should be infinite when max_instances or max_samples_per_instance are infinite"); +>>>>>>> 68acb5a6c (Check History QoS inconsistencies (#4375)) 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 index 73a0d89baec..f31e0babf24 100644 --- a/src/cpp/fastdds/subscriber/DataReaderImpl.cpp +++ b/src/cpp/fastdds/subscriber/DataReaderImpl.cpp @@ -1318,7 +1318,45 @@ ReturnCode_t DataReaderImpl::check_qos ( } if (qos_has_unique_network_request(qos) && qos_has_specific_locators(qos)) { +<<<<<<< HEAD logError(DDS_QOS_CHECK, "unique_network_request cannot be set along specific locators"); +======= + 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; +} + +ReturnCode_t DataReaderImpl::check_allocation_consistency( + const DataReaderQos& qos) +{ + if ((qos.resource_limits().max_samples > 0) && + (qos.resource_limits().max_samples < + (qos.resource_limits().max_instances * qos.resource_limits().max_samples_per_instance))) + { + EPROSIMA_LOG_ERROR(DDS_QOS_CHECK, + "max_samples should be greater than max_instances * max_samples_per_instance"); + return ReturnCode_t::RETCODE_INCONSISTENT_POLICY; + } + if ((qos.resource_limits().max_instances <= 0 || qos.resource_limits().max_samples_per_instance <= 0) && + (qos.resource_limits().max_samples > 0)) + { + EPROSIMA_LOG_ERROR(DDS_QOS_CHECK, + "max_samples should be infinite when max_instances or max_samples_per_instance are infinite"); +>>>>>>> 68acb5a6c (Check History QoS inconsistencies (#4375)) 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 index e7c31ecc5d1..e356fca9af1 100644 --- a/test/blackbox/common/DDSBlackboxTestsDataWriter.cpp +++ b/test/blackbox/common/DDSBlackboxTestsDataWriter.cpp @@ -298,6 +298,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) { @@ -310,13 +311,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(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(n_samples)) + .resource_limits_max_samples(static_cast(n_samples)) + .resource_limits_max_instances(static_cast(1)) + .resource_limits_max_samples_per_instance(static_cast(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_profiles.xml b/test/unittest/dds/profiles/test_xml_profiles.xml index 67408971cf2..b75bba59fed 100644 --- a/test/unittest/dds/profiles/test_xml_profiles.xml +++ b/test/unittest/dds/profiles/test_xml_profiles.xml @@ -480,9 +480,16 @@ 10 0 +<<<<<<< HEAD:test/unittest/dds/profiles/test_xml_profiles.xml +======= + + + + +>>>>>>> 68acb5a6c (Check History QoS inconsistencies (#4375)):test/unittest/dds/profiles/test_xml_profile.xml NO_KEY otherSamplePubSubTopic @@ -492,9 +499,9 @@ 500 - 10 + 2500 5 - 2 + 500 10 diff --git a/test/unittest/dds/publisher/DataWriterTests.cpp b/test/unittest/dds/publisher/DataWriterTests.cpp index 9912b4276c0..defd70dc08b 100644 --- a/test/unittest/dds/publisher/DataWriterTests.cpp +++ b/test/unittest/dds/publisher/DataWriterTests.cpp @@ -665,6 +665,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 index 166b37c6247..540eb8f45e9 100644 --- a/test/unittest/dds/subscriber/DataReaderTests.cpp +++ b/test/unittest/dds/subscriber/DataReaderTests.cpp @@ -679,6 +679,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; From 1cb8cf46012eb8b1bdb23098fcd2210b6e7395e8 Mon Sep 17 00:00:00 2001 From: JesusPoderoso Date: Tue, 20 Feb 2024 14:52:13 +0100 Subject: [PATCH 2/6] Refs #20401: Fix conflicts Signed-off-by: JesusPoderoso --- src/cpp/fastdds/publisher/DataWriterImpl.cpp | 30 ++----------------- src/cpp/fastdds/subscriber/DataReaderImpl.cpp | 30 ++----------------- .../dds/profiles/test_xml_profiles.xml | 12 ++------ 3 files changed, 7 insertions(+), 65 deletions(-) diff --git a/src/cpp/fastdds/publisher/DataWriterImpl.cpp b/src/cpp/fastdds/publisher/DataWriterImpl.cpp index 4bef7dbf4bd..efbe3266f69 100644 --- a/src/cpp/fastdds/publisher/DataWriterImpl.cpp +++ b/src/cpp/fastdds/publisher/DataWriterImpl.cpp @@ -1701,45 +1701,19 @@ ReturnCode_t DataWriterImpl::check_qos( (qos.endpoint().history_memory_policy != PREALLOCATED_MEMORY_MODE && qos.endpoint().history_memory_policy != PREALLOCATED_WITH_REALLOC_MEMORY_MODE)) { -<<<<<<< HEAD logError(RTPS_QOS_CHECK, "DATA_SHARING cannot be used with memory policies other than PREALLOCATED."); -======= - 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."); + logError(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; -} - -ReturnCode_t DataWriterImpl::check_allocation_consistency( - const DataWriterQos& qos) -{ - if ((qos.resource_limits().max_samples > 0) && - (qos.resource_limits().max_samples < - (qos.resource_limits().max_instances * qos.resource_limits().max_samples_per_instance))) - { - EPROSIMA_LOG_ERROR(DDS_QOS_CHECK, - "max_samples should be greater than max_instances * max_samples_per_instance"); - return ReturnCode_t::RETCODE_INCONSISTENT_POLICY; - } - if ((qos.resource_limits().max_instances <= 0 || qos.resource_limits().max_samples_per_instance <= 0) && - (qos.resource_limits().max_samples > 0)) - { - EPROSIMA_LOG_ERROR(DDS_QOS_CHECK, - "max_samples should be infinite when max_instances or max_samples_per_instance are infinite"); ->>>>>>> 68acb5a6c (Check History QoS inconsistencies (#4375)) + logError(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 index f31e0babf24..43bbcf23900 100644 --- a/src/cpp/fastdds/subscriber/DataReaderImpl.cpp +++ b/src/cpp/fastdds/subscriber/DataReaderImpl.cpp @@ -1318,45 +1318,19 @@ ReturnCode_t DataReaderImpl::check_qos ( } if (qos_has_unique_network_request(qos) && qos_has_specific_locators(qos)) { -<<<<<<< HEAD logError(DDS_QOS_CHECK, "unique_network_request cannot be set along specific locators"); -======= - 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."); + logError(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; -} - -ReturnCode_t DataReaderImpl::check_allocation_consistency( - const DataReaderQos& qos) -{ - if ((qos.resource_limits().max_samples > 0) && - (qos.resource_limits().max_samples < - (qos.resource_limits().max_instances * qos.resource_limits().max_samples_per_instance))) - { - EPROSIMA_LOG_ERROR(DDS_QOS_CHECK, - "max_samples should be greater than max_instances * max_samples_per_instance"); - return ReturnCode_t::RETCODE_INCONSISTENT_POLICY; - } - if ((qos.resource_limits().max_instances <= 0 || qos.resource_limits().max_samples_per_instance <= 0) && - (qos.resource_limits().max_samples > 0)) - { - EPROSIMA_LOG_ERROR(DDS_QOS_CHECK, - "max_samples should be infinite when max_instances or max_samples_per_instance are infinite"); ->>>>>>> 68acb5a6c (Check History QoS inconsistencies (#4375)) + logError(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/unittest/dds/profiles/test_xml_profiles.xml b/test/unittest/dds/profiles/test_xml_profiles.xml index b75bba59fed..b2a56011742 100644 --- a/test/unittest/dds/profiles/test_xml_profiles.xml +++ b/test/unittest/dds/profiles/test_xml_profiles.xml @@ -261,7 +261,7 @@ default_name - + NO_KEY @@ -480,16 +480,10 @@ 10 0 -<<<<<<< HEAD:test/unittest/dds/profiles/test_xml_profiles.xml - - -======= - - ->>>>>>> 68acb5a6c (Check History QoS inconsistencies (#4375)):test/unittest/dds/profiles/test_xml_profile.xml + NO_KEY otherSamplePubSubTopic @@ -585,7 +579,7 @@ 5 - + WITH_KEY From 614ac65b9d582981ad030ea4e41f25b499d8716d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Dom=C3=ADnguez=20L=C3=B3pez?= <116071334+Mario-DL@users.noreply.github.com> Date: Wed, 21 Feb 2024 21:14:35 +0100 Subject: [PATCH 3/6] Fix SecureHelloworldExample (#4416) Signed-off-by: Mario Dominguez --- .../C++/DDS/SecureHelloWorldExample/HelloWorldPublisher.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/examples/C++/DDS/SecureHelloWorldExample/HelloWorldPublisher.cpp b/examples/C++/DDS/SecureHelloWorldExample/HelloWorldPublisher.cpp index d874c154806..cbe96942815 100644 --- a/examples/C++/DDS/SecureHelloWorldExample/HelloWorldPublisher.cpp +++ b/examples/C++/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; From c29020b6861a78fd2cbe94efadf28f75a70e2c6b Mon Sep 17 00:00:00 2001 From: Miguel Company Date: Fri, 23 Feb 2024 07:04:42 +0100 Subject: [PATCH 4/6] Just show warning when inconsistency between depth and max_samples_per_instance (#4417) * Refs #20503. Add regression test Signed-off-by: EduPonz * Refs #20503. Show warning when depth > max_samples_per_instance Signed-off-by: EduPonz * Refs #20503. Fix InvalidQos tests Signed-off-by: EduPonz --------- Signed-off-by: EduPonz Co-authored-by: EduPonz --- src/cpp/fastdds/publisher/DataWriterImpl.cpp | 7 +- src/cpp/fastdds/subscriber/DataReaderImpl.cpp | 7 +- .../common/DDSBlackboxTestsDataReader.cpp | 11 ++ .../common/DDSBlackboxTestsDataWriter.cpp | 11 ++ .../dds/publisher/DataWriterTests.cpp | 80 ++++++++++++- .../dds/subscriber/DataReaderTests.cpp | 105 +++++++++++++++--- 6 files changed, 197 insertions(+), 24 deletions(-) diff --git a/src/cpp/fastdds/publisher/DataWriterImpl.cpp b/src/cpp/fastdds/publisher/DataWriterImpl.cpp index efbe3266f69..0e6131e5d49 100644 --- a/src/cpp/fastdds/publisher/DataWriterImpl.cpp +++ b/src/cpp/fastdds/publisher/DataWriterImpl.cpp @@ -1713,8 +1713,11 @@ ReturnCode_t DataWriterImpl::check_qos( qos.resource_limits().max_samples_per_instance > 0 && qos.history().depth > qos.resource_limits().max_samples_per_instance) { - logError(RTPS_QOS_CHECK,"HISTORY DEPTH must be lower or equal to the max_samples_per_instance value."); - return ReturnCode_t::RETCODE_INCONSISTENT_POLICY; + logWarning(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 index 43bbcf23900..866b7e6d36e 100644 --- a/src/cpp/fastdds/subscriber/DataReaderImpl.cpp +++ b/src/cpp/fastdds/subscriber/DataReaderImpl.cpp @@ -1330,8 +1330,11 @@ ReturnCode_t DataReaderImpl::check_qos ( qos.resource_limits().max_samples_per_instance > 0 && qos.history().depth > qos.resource_limits().max_samples_per_instance) { - logError(RTPS_QOS_CHECK, "HISTORY DEPTH must be lower or equal to the max_samples_per_instance value."); - return ReturnCode_t::RETCODE_INCONSISTENT_POLICY; + logWarning(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 index 46778700c49..d2bb301541b 100644 --- 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 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 index e356fca9af1..be0fc5d76d1 100644 --- a/test/blackbox/common/DDSBlackboxTestsDataWriter.cpp +++ b/test/blackbox/common/DDSBlackboxTestsDataWriter.cpp @@ -339,6 +339,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 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/publisher/DataWriterTests.cpp b/test/unittest/dds/publisher/DataWriterTests.cpp index defd70dc08b..7485f801e8e 100644 --- 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 #include +#include +#include #include #include @@ -24,6 +27,7 @@ #include #include #include +#include #include #include #include @@ -36,7 +40,6 @@ #include #include - #include "../../logging/mock/MockConsumer.h" namespace eprosima { @@ -671,8 +674,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); @@ -1518,6 +1523,75 @@ TEST_F(DataWriterUnsupportedTests, UnsupportedDataWriterMethods) ASSERT_EQ(DomainParticipantFactory::get_instance()->delete_participant(participant), ReturnCode_t::RETCODE_OK); } +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(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 diff --git a/test/unittest/dds/subscriber/DataReaderTests.cpp b/test/unittest/dds/subscriber/DataReaderTests.cpp index 540eb8f45e9..9d07f529fef 100644 --- a/test/unittest/dds/subscriber/DataReaderTests.cpp +++ b/test/unittest/dds/subscriber/DataReaderTests.cpp @@ -13,15 +13,23 @@ // limitations under the License. #include +#include +#include +#include +#include +#include +#include #include +#include + #include #include #include #include - +#include #include #include #include @@ -30,37 +38,31 @@ #include #include #include - -#include #include +#include #include - +#include #include #include #include #include - #include #include -#include -#include #include #include - +#include +#include #include +#include #include +#include +#include "../../logging/mock/MockConsumer.h" #include "FooBoundedType.hpp" #include "FooBoundedTypeSupport.hpp" - #include "FooType.hpp" #include "FooTypeSupport.hpp" -#include "../../logging/mock/MockConsumer.h" - -#include -#include - namespace eprosima { namespace fastdds { namespace dds { @@ -683,9 +685,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; @@ -2659,6 +2665,71 @@ TEST_F(DataReaderTests, delete_contained_entities) ASSERT_EQ(data_reader->delete_contained_entities(), ReturnCode_t::RETCODE_OK); } +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(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(); +} + } // namespace dds } // namespace fastdds } // namespace eprosima From 33103f1e45db012971aa287722209a74d6839955 Mon Sep 17 00:00:00 2001 From: JesusPoderoso Date: Wed, 20 Mar 2024 09:13:57 +0100 Subject: [PATCH 5/6] Refs #20401: Fix warning Signed-off-by: JesusPoderoso --- test/unittest/dds/subscriber/DataReaderTests.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/unittest/dds/subscriber/DataReaderTests.cpp b/test/unittest/dds/subscriber/DataReaderTests.cpp index 9d07f529fef..bdc9d43af31 100644 --- a/test/unittest/dds/subscriber/DataReaderTests.cpp +++ b/test/unittest/dds/subscriber/DataReaderTests.cpp @@ -21,8 +21,6 @@ #include #include -#include - #include #include From 15ed0271e7ce3bb3754e770579d74c82eff204f0 Mon Sep 17 00:00:00 2001 From: JesusPoderoso Date: Thu, 4 Apr 2024 12:30:02 +0200 Subject: [PATCH 6/6] Refs #20401: Fix segfault in Mac tests Signed-off-by: JesusPoderoso --- test/unittest/dds/publisher/DataWriterTests.cpp | 8 +++++--- test/unittest/dds/subscriber/DataReaderTests.cpp | 9 ++++++--- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/test/unittest/dds/publisher/DataWriterTests.cpp b/test/unittest/dds/publisher/DataWriterTests.cpp index 7485f801e8e..9b7de755dee 100644 --- a/test/unittest/dds/publisher/DataWriterTests.cpp +++ b/test/unittest/dds/publisher/DataWriterTests.cpp @@ -1587,9 +1587,11 @@ TEST(DataWriterTests, history_depth_max_samples_per_instance_warning) 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(); + ASSERT_EQ(publisher->delete_datawriter(datawriter_1), ReturnCode_t::RETCODE_OK); + ASSERT_EQ(publisher->delete_datawriter(datawriter_2), ReturnCode_t::RETCODE_OK); + ASSERT_EQ(participant->delete_publisher(publisher), ReturnCode_t::RETCODE_OK); + ASSERT_EQ(participant->delete_topic(topic), ReturnCode_t::RETCODE_OK); + ASSERT_EQ(DomainParticipantFactory::get_instance()->delete_participant(participant), ReturnCode_t::RETCODE_OK); } } // namespace dds diff --git a/test/unittest/dds/subscriber/DataReaderTests.cpp b/test/unittest/dds/subscriber/DataReaderTests.cpp index bdc9d43af31..d0afc841d57 100644 --- a/test/unittest/dds/subscriber/DataReaderTests.cpp +++ b/test/unittest/dds/subscriber/DataReaderTests.cpp @@ -2723,9 +2723,12 @@ TEST_F(DataReaderTests, history_depth_max_samples_per_instance_warning) 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(); + ASSERT_EQ(subscriber->delete_datareader(datareader_1), ReturnCode_t::RETCODE_OK); + ASSERT_EQ(subscriber->delete_datareader(datareader_2), ReturnCode_t::RETCODE_OK); + ASSERT_EQ(participant->delete_subscriber(subscriber), ReturnCode_t::RETCODE_OK); + ASSERT_EQ(participant->delete_topic(topic), ReturnCode_t::RETCODE_OK); + ASSERT_EQ(DomainParticipantFactory::get_instance()->delete_participant(participant), ReturnCode_t::RETCODE_OK); + } } // namespace dds