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; diff --git a/src/cpp/fastdds/publisher/DataWriterImpl.cpp b/src/cpp/fastdds/publisher/DataWriterImpl.cpp index 97dd80a4803..0e6131e5d49 100644 --- a/src/cpp/fastdds/publisher/DataWriterImpl.cpp +++ b/src/cpp/fastdds/publisher/DataWriterImpl.cpp @@ -1704,6 +1704,21 @@ ReturnCode_t DataWriterImpl::check_qos( logError(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) + { + 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) + { + 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 73a0d89baec..866b7e6d36e 100644 --- a/src/cpp/fastdds/subscriber/DataReaderImpl.cpp +++ b/src/cpp/fastdds/subscriber/DataReaderImpl.cpp @@ -1321,6 +1321,21 @@ ReturnCode_t DataReaderImpl::check_qos ( logError(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) + { + 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) + { + 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 e7c31ecc5d1..be0fc5d76d1 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(); @@ -330,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/profiles/test_xml_profiles.xml b/test/unittest/dds/profiles/test_xml_profiles.xml index 67408971cf2..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 @@ -481,7 +481,8 @@ 0 - + + NO_KEY @@ -492,9 +493,9 @@ 500 - 10 + 2500 5 - 2 + 500 10 @@ -578,7 +579,7 @@ 5 - + WITH_KEY diff --git a/test/unittest/dds/publisher/DataWriterTests.cpp b/test/unittest/dds/publisher/DataWriterTests.cpp index 9912b4276c0..9b7de755dee 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 { @@ -665,6 +668,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); @@ -1509,6 +1523,77 @@ 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 */ + 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 } // namespace fastdds } // namespace eprosima diff --git a/test/unittest/dds/subscriber/DataReaderTests.cpp b/test/unittest/dds/subscriber/DataReaderTests.cpp index 166b37c6247..d0afc841d57 100644 --- a/test/unittest/dds/subscriber/DataReaderTests.cpp +++ b/test/unittest/dds/subscriber/DataReaderTests.cpp @@ -13,6 +13,12 @@ // limitations under the License. #include +#include +#include +#include +#include +#include +#include #include #include @@ -21,7 +27,7 @@ #include #include - +#include #include #include #include @@ -30,37 +36,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 { @@ -679,6 +679,18 @@ 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 + // 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; @@ -2651,6 +2663,74 @@ 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 */ + 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 } // namespace fastdds } // namespace eprosima