From f85b26b16438c91ab33553358e172970563941d7 Mon Sep 17 00:00:00 2001 From: Eugenio Collado Date: Thu, 5 Dec 2024 08:53:22 +0100 Subject: [PATCH 01/12] Tests arithmetic overflow in fragment size calculations Signed-off-by: Eugenio Collado --- test/unittest/rtps/history/CMakeLists.txt | 21 ++++ .../rtps/history/WriterHistoryTests.cpp | 99 +++++++++++++++++++ test/unittest/rtps/writer/BaseWriterTests.cpp | 90 +++++++++++++++++ test/unittest/rtps/writer/CMakeLists.txt | 16 +++ 4 files changed, 226 insertions(+) create mode 100644 test/unittest/rtps/history/WriterHistoryTests.cpp create mode 100644 test/unittest/rtps/writer/BaseWriterTests.cpp diff --git a/test/unittest/rtps/history/CMakeLists.txt b/test/unittest/rtps/history/CMakeLists.txt index c5f8fb1401b..de2fe5b01a4 100644 --- a/test/unittest/rtps/history/CMakeLists.txt +++ b/test/unittest/rtps/history/CMakeLists.txt @@ -76,6 +76,8 @@ set(TOPICPAYLOADPOOLTESTS_SOURCE ${PROJECT_SOURCE_DIR}/src/cpp/utils/IPLocator.cpp ${PROJECT_SOURCE_DIR}/src/cpp/utils/SystemInfo.cpp) +set(WRITERHISTORYTESTS_SOURCE WriterHistoryTests.cpp) + if(WIN32) add_definitions(-D_WIN32_WINNT=0x0601) endif() @@ -164,3 +166,22 @@ target_link_libraries(TopicPayloadPoolTests GTest::gtest ${CMAKE_DL_LIBS}) gtest_discover_tests(TopicPayloadPoolTests) + + + +add_executable(WriterHistoryTests ${WRITERHISTORYTESTS_SOURCE}) +target_compile_definitions(WriterHistoryTests PRIVATE + BOOST_ASIO_STANDALONE + ASIO_STANDALONE + $<$>,$>:__DEBUG> + $<$:__INTERNALDEBUG> # Internal debug activated. + ) +target_include_directories(WriterHistoryTests PRIVATE + ${Asio_INCLUDE_DIR}) +target_link_libraries(WriterHistoryTests + fastcdr + fastdds + foonathan_memory + GTest::gmock + ${CMAKE_DL_LIBS}) +gtest_discover_tests(WriterHistoryTests) diff --git a/test/unittest/rtps/history/WriterHistoryTests.cpp b/test/unittest/rtps/history/WriterHistoryTests.cpp new file mode 100644 index 00000000000..7cd0c2543e6 --- /dev/null +++ b/test/unittest/rtps/history/WriterHistoryTests.cpp @@ -0,0 +1,99 @@ +// Copyright 2020 Proyectos y Sistemas de Mantenimiento SL (eProsima). +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include +#include + +#include +#include +#include +#include +#include + +#include + + +namespace eprosima { +namespace fastdds { +namespace rtps { + +using namespace testing; + +#define MAX_MESSAGE_SIZE 300 + +void cache_change_fragment( + uint32_t inline_qos_length) +{ + uint32_t domain_id = 0; + uint32_t initial_reserved_caches = 10; + uint32_t max_message_size = MAX_MESSAGE_SIZE; + std::string max_message_size_str = std::to_string(max_message_size); + + RTPSParticipantAttributes p_attr; + p_attr.properties.properties().emplace_back("fastdds.max_message_size", max_message_size_str); + RTPSParticipant* participant = RTPSDomain::createParticipant( + domain_id, true, p_attr); + + ASSERT_NE(participant, nullptr); + + HistoryAttributes h_attr; + h_attr.memoryPolicy = DYNAMIC_RESERVE_MEMORY_MODE; + h_attr.initialReservedCaches = initial_reserved_caches; + h_attr.payloadMaxSize = 250; + WriterHistory* history = new WriterHistory(h_attr); + + WriterAttributes w_attr; + RTPSWriter* writer = RTPSDomain::createRTPSWriter(participant, w_attr, history); + + ASSERT_NE(writer, nullptr); + + BaseWriter* bwriter = BaseWriter::downcast(writer); + auto max_allowed_payload_size = bwriter->get_max_allowed_payload_size(); + + CacheChange_t change; + change.writerGUID = bwriter->getGuid(); + change.serializedPayload.length = 3 * max_allowed_payload_size; // Force to setFragmentSize + change.inline_qos.length = inline_qos_length; + + history->add_change(&change); + + auto result = change.getFragmentSize(); + std::cout << "Fragment size: " << result << std::endl; + ASSERT_NE(result, 0); // Fragment size should always be greater than 0 +} + +/** + * This test checks the fragment size calculation for a cache change depending on the inline qos length. + * The change.serializedPayload.length is set to 3 times the max_allowed_payload_size, so the fragment size should always be set. + * In case of an overflow in the attribute high_mark_for_frag_ the fragment size will not be set, which is an error. + */ +TEST(WriterHistoryTests, calculate_max_payload_size_overflow) +{ + for (uint32_t inline_qos_length = 0; inline_qos_length < MAX_MESSAGE_SIZE; inline_qos_length += 40) + { + cache_change_fragment(inline_qos_length); + } +} + +} // namespace rtps +} // namespace fastdds +} // namespace eprosima + +int main( + int argc, + char** argv) +{ + testing::InitGoogleMock(&argc, argv); + return RUN_ALL_TESTS(); +} diff --git a/test/unittest/rtps/writer/BaseWriterTests.cpp b/test/unittest/rtps/writer/BaseWriterTests.cpp new file mode 100644 index 00000000000..fb8def65ec9 --- /dev/null +++ b/test/unittest/rtps/writer/BaseWriterTests.cpp @@ -0,0 +1,90 @@ +// Copyright 2020 Proyectos y Sistemas de Mantenimiento SL (eProsima). +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include +#include + +#include +#include +#include +#include +#include + +#include + + +namespace eprosima { +namespace fastdds { +namespace rtps { + +using namespace testing; + +void max_allowed_payload_size( + uint32_t max_message_size) +{ + uint32_t domain_id = 0; + uint32_t initial_reserved_caches = 10; + std::string max_message_size_str = std::to_string(max_message_size); + + RTPSParticipantAttributes p_attr; + p_attr.properties.properties().emplace_back("fastdds.max_message_size", max_message_size_str); + RTPSParticipant* participant = RTPSDomain::createParticipant( + domain_id, true, p_attr); + + ASSERT_NE(participant, nullptr); + + HistoryAttributes h_attr; + h_attr.memoryPolicy = DYNAMIC_RESERVE_MEMORY_MODE; + h_attr.initialReservedCaches = initial_reserved_caches; + h_attr.payloadMaxSize = 250; + WriterHistory* history = new WriterHistory(h_attr); + + WriterAttributes w_attr; + RTPSWriter* writer = RTPSDomain::createRTPSWriter(participant, w_attr, history); + + ASSERT_NE(writer, nullptr); + + BaseWriter* bwriter = BaseWriter::downcast(writer); + + auto result = bwriter->get_max_allowed_payload_size(); + std::cout << "For max_message_size: " << max_message_size << " the max allowed payload size is: " << result << std::endl; + + ASSERT_LE(result, max_message_size); +} + +/** + * This test checks the get_max_allowed_payload_size() method of the BaseWriter. + * The method is called within a loop with different values of max_message_size. + * The test checks that the max_payload_size is always less than max_message_size, + * in other case it means an overflow has occurred. + */ +TEST(BaseWriterTests, calculate_max_payload_size_overflow) +{ + for (uint32_t max_message_size = 200; max_message_size > 150; max_message_size -= 12) + { + max_allowed_payload_size(max_message_size); + } +} + +} // namespace rtps +} // namespace fastdds +} // namespace eprosima + +int main( + int argc, + char** argv) +{ + testing::InitGoogleMock(&argc, argv); + return RUN_ALL_TESTS(); +} diff --git a/test/unittest/rtps/writer/CMakeLists.txt b/test/unittest/rtps/writer/CMakeLists.txt index 3e83e12a8e1..b4142086155 100644 --- a/test/unittest/rtps/writer/CMakeLists.txt +++ b/test/unittest/rtps/writer/CMakeLists.txt @@ -108,6 +108,22 @@ target_link_libraries(LivelinessManagerTests PRIVATE ) gtest_discover_tests(LivelinessManagerTests) +set(BASEWRITERTESTS_SOURCE BaseWriterTests.cpp) + +add_executable(BaseWriterTests ${BASEWRITERTESTS_SOURCE}) +target_compile_definitions(BaseWriterTests PRIVATE + BOOST_ASIO_STANDALONE + ASIO_STANDALONE + $<$>,$>:__DEBUG> + $<$:__INTERNALDEBUG> # Internal debug activated. + ) +target_include_directories(BaseWriterTests PRIVATE + ${Asio_INCLUDE_DIR}) +target_link_libraries(BaseWriterTests fastcdr fastdds foonathan_memory + GTest::gmock + ${CMAKE_DL_LIBS}) +gtest_discover_tests(BaseWriterTests) + if(NOT QNX) set(RTPSWRITERTESTS_SOURCE RTPSWriterTests.cpp) From 2d4823d18ca61950a23fbaf96e7c5427a763238f Mon Sep 17 00:00:00 2001 From: Miguel Company Date: Mon, 7 Oct 2024 09:40:25 +0200 Subject: [PATCH 02/12] Refs #21814. Fix code in BaseWriter.cpp. Signed-off-by: Miguel Company --- src/cpp/rtps/writer/BaseWriter.cpp | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/src/cpp/rtps/writer/BaseWriter.cpp b/src/cpp/rtps/writer/BaseWriter.cpp index cd8aa6bfc88..b3de3b4f5c8 100644 --- a/src/cpp/rtps/writer/BaseWriter.cpp +++ b/src/cpp/rtps/writer/BaseWriter.cpp @@ -189,30 +189,35 @@ uint32_t BaseWriter::calculate_max_payload_size( constexpr uint32_t heartbeat_message_length = 32; uint32_t max_data_size = mp_RTPSParticipant->calculateMaxDataSize(datagram_length); - - max_data_size -= info_dst_message_length + + uint32_t overhead = info_dst_message_length + info_ts_message_length + data_frag_submessage_header_length + heartbeat_message_length; - //TODO(Ricardo) inlineqos in future. - #if HAVE_SECURITY if (getAttributes().security_attributes().is_submessage_protected) { - max_data_size -= mp_RTPSParticipant->security_manager().calculate_extra_size_for_rtps_submessage(m_guid); + overhead += mp_RTPSParticipant->security_manager().calculate_extra_size_for_rtps_submessage(m_guid); } if (getAttributes().security_attributes().is_payload_protected) { - max_data_size -= mp_RTPSParticipant->security_manager().calculate_extra_size_for_encoded_payload(m_guid); + overhead += mp_RTPSParticipant->security_manager().calculate_extra_size_for_encoded_payload(m_guid); } #endif // if HAVE_SECURITY #ifdef FASTDDS_STATISTICS - max_data_size -= eprosima::fastdds::statistics::rtps::statistics_submessage_length; + overhead += eprosima::fastdds::statistics::rtps::statistics_submessage_length; #endif // FASTDDS_STATISTICS + if (overhead > max_data_size) + { + EPROSIMA_LOG_ERROR(RTPS_WRITER, "Datagram length '" << datagram_length << "' is too small." << + "At least " << overhead << " bytes are needed to send a message. Fixing fragments to 4 bytes."); + return 4; + } + + max_data_size -= overhead; return max_data_size; } From 95a17e55428caec5eda0a1e88179da8f2ae7b02d Mon Sep 17 00:00:00 2001 From: Eugenio Collado Date: Thu, 5 Dec 2024 09:07:07 +0100 Subject: [PATCH 03/12] Fix corner case overhead==max_data_size Signed-off-by: Eugenio Collado --- src/cpp/rtps/writer/BaseWriter.cpp | 7 ++++--- test/unittest/rtps/writer/BaseWriterTests.cpp | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/cpp/rtps/writer/BaseWriter.cpp b/src/cpp/rtps/writer/BaseWriter.cpp index b3de3b4f5c8..fa64a30d739 100644 --- a/src/cpp/rtps/writer/BaseWriter.cpp +++ b/src/cpp/rtps/writer/BaseWriter.cpp @@ -210,11 +210,12 @@ uint32_t BaseWriter::calculate_max_payload_size( overhead += eprosima::fastdds::statistics::rtps::statistics_submessage_length; #endif // FASTDDS_STATISTICS - if (overhead > max_data_size) + constexpr uint32_t min_fragment_size = 4; + if ((overhead + min_fragment_size) > max_data_size) { EPROSIMA_LOG_ERROR(RTPS_WRITER, "Datagram length '" << datagram_length << "' is too small." << - "At least " << overhead << " bytes are needed to send a message. Fixing fragments to 4 bytes."); - return 4; + "At least " << overhead << " bytes are needed to send a message. Fixing fragments to " << min_fragment_size << " bytes."); + return min_fragment_size; } max_data_size -= overhead; diff --git a/test/unittest/rtps/writer/BaseWriterTests.cpp b/test/unittest/rtps/writer/BaseWriterTests.cpp index fb8def65ec9..c0acdfa2777 100644 --- a/test/unittest/rtps/writer/BaseWriterTests.cpp +++ b/test/unittest/rtps/writer/BaseWriterTests.cpp @@ -71,7 +71,7 @@ void max_allowed_payload_size( */ TEST(BaseWriterTests, calculate_max_payload_size_overflow) { - for (uint32_t max_message_size = 200; max_message_size > 150; max_message_size -= 12) + for (uint32_t max_message_size = 200; max_message_size > 150; max_message_size -= 4) { max_allowed_payload_size(max_message_size); } From 805291c3454af1f2beca959b1c084773f28c7a65 Mon Sep 17 00:00:00 2001 From: Miguel Company Date: Mon, 7 Oct 2024 09:41:08 +0200 Subject: [PATCH 04/12] Refs #21814. Fix code in WriterHistory.cpp. Signed-off-by: Miguel Company --- src/cpp/rtps/history/WriterHistory.cpp | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/cpp/rtps/history/WriterHistory.cpp b/src/cpp/rtps/history/WriterHistory.cpp index a7315451854..ddb61f6ce2b 100644 --- a/src/cpp/rtps/history/WriterHistory.cpp +++ b/src/cpp/rtps/history/WriterHistory.cpp @@ -484,9 +484,15 @@ void WriterHistory::set_fragments( // If inlineqos for related_sample_identity is required, then remove its size from the final fragment size. if (0 < inline_qos_size) { - final_high_mark_for_frag -= ( - fastdds::dds::ParameterSerializer::PARAMETER_SENTINEL_SIZE + - inline_qos_size); + uint32_t overhead = fastdds::dds::ParameterSerializer::PARAMETER_SENTINEL_SIZE + inline_qos_size; + if (final_high_mark_for_frag < overhead) + { + final_high_mark_for_frag = 4; + } + else + { + final_high_mark_for_frag -= overhead; + } } // If it is big data, fragment it. From 0823f101001e30d0cd5803f94d02650630e60145 Mon Sep 17 00:00:00 2001 From: Eugenio Collado Date: Thu, 5 Dec 2024 09:15:18 +0100 Subject: [PATCH 05/12] Fix corner case overhead==final_high_mark_for_frag Signed-off-by: Eugenio Collado --- src/cpp/rtps/history/WriterHistory.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/cpp/rtps/history/WriterHistory.cpp b/src/cpp/rtps/history/WriterHistory.cpp index ddb61f6ce2b..0b301d3e7d9 100644 --- a/src/cpp/rtps/history/WriterHistory.cpp +++ b/src/cpp/rtps/history/WriterHistory.cpp @@ -485,9 +485,10 @@ void WriterHistory::set_fragments( if (0 < inline_qos_size) { uint32_t overhead = fastdds::dds::ParameterSerializer::PARAMETER_SENTINEL_SIZE + inline_qos_size; - if (final_high_mark_for_frag < overhead) + constexpr uint32_t min_fragment_size = 4; + if (final_high_mark_for_frag < (overhead + min_fragment_size)) { - final_high_mark_for_frag = 4; + final_high_mark_for_frag = min_fragment_size; } else { From a180b98a3226a31ad34e017bdaa39b994b4733cb Mon Sep 17 00:00:00 2001 From: Eugenio Collado Date: Thu, 5 Dec 2024 09:47:46 +0100 Subject: [PATCH 06/12] Uncrustify Signed-off-by: Eugenio Collado --- src/cpp/rtps/writer/BaseWriter.cpp | 3 ++- test/unittest/rtps/history/WriterHistoryTests.cpp | 8 ++++---- test/unittest/rtps/writer/BaseWriterTests.cpp | 9 +++++---- 3 files changed, 11 insertions(+), 9 deletions(-) diff --git a/src/cpp/rtps/writer/BaseWriter.cpp b/src/cpp/rtps/writer/BaseWriter.cpp index fa64a30d739..a2c8278493b 100644 --- a/src/cpp/rtps/writer/BaseWriter.cpp +++ b/src/cpp/rtps/writer/BaseWriter.cpp @@ -214,7 +214,8 @@ uint32_t BaseWriter::calculate_max_payload_size( if ((overhead + min_fragment_size) > max_data_size) { EPROSIMA_LOG_ERROR(RTPS_WRITER, "Datagram length '" << datagram_length << "' is too small." << - "At least " << overhead << " bytes are needed to send a message. Fixing fragments to " << min_fragment_size << " bytes."); + "At least " << overhead << " bytes are needed to send a message. Fixing fragments to " << + min_fragment_size << " bytes."); return min_fragment_size; } diff --git a/test/unittest/rtps/history/WriterHistoryTests.cpp b/test/unittest/rtps/history/WriterHistoryTests.cpp index 7cd0c2543e6..50d9705ace9 100644 --- a/test/unittest/rtps/history/WriterHistoryTests.cpp +++ b/test/unittest/rtps/history/WriterHistoryTests.cpp @@ -33,7 +33,7 @@ using namespace testing; #define MAX_MESSAGE_SIZE 300 void cache_change_fragment( - uint32_t inline_qos_length) + uint32_t inline_qos_length) { uint32_t domain_id = 0; uint32_t initial_reserved_caches = 10; @@ -41,7 +41,7 @@ void cache_change_fragment( std::string max_message_size_str = std::to_string(max_message_size); RTPSParticipantAttributes p_attr; - p_attr.properties.properties().emplace_back("fastdds.max_message_size", max_message_size_str); + p_attr.properties.properties().emplace_back("fastdds.max_message_size", max_message_size_str); RTPSParticipant* participant = RTPSDomain::createParticipant( domain_id, true, p_attr); @@ -65,10 +65,10 @@ void cache_change_fragment( change.writerGUID = bwriter->getGuid(); change.serializedPayload.length = 3 * max_allowed_payload_size; // Force to setFragmentSize change.inline_qos.length = inline_qos_length; - + history->add_change(&change); - auto result = change.getFragmentSize(); + auto result = change.getFragmentSize(); std::cout << "Fragment size: " << result << std::endl; ASSERT_NE(result, 0); // Fragment size should always be greater than 0 } diff --git a/test/unittest/rtps/writer/BaseWriterTests.cpp b/test/unittest/rtps/writer/BaseWriterTests.cpp index c0acdfa2777..26391271155 100644 --- a/test/unittest/rtps/writer/BaseWriterTests.cpp +++ b/test/unittest/rtps/writer/BaseWriterTests.cpp @@ -31,14 +31,14 @@ namespace rtps { using namespace testing; void max_allowed_payload_size( - uint32_t max_message_size) + uint32_t max_message_size) { uint32_t domain_id = 0; uint32_t initial_reserved_caches = 10; std::string max_message_size_str = std::to_string(max_message_size); RTPSParticipantAttributes p_attr; - p_attr.properties.properties().emplace_back("fastdds.max_message_size", max_message_size_str); + p_attr.properties.properties().emplace_back("fastdds.max_message_size", max_message_size_str); RTPSParticipant* participant = RTPSDomain::createParticipant( domain_id, true, p_attr); @@ -58,7 +58,8 @@ void max_allowed_payload_size( BaseWriter* bwriter = BaseWriter::downcast(writer); auto result = bwriter->get_max_allowed_payload_size(); - std::cout << "For max_message_size: " << max_message_size << " the max allowed payload size is: " << result << std::endl; + std::cout << "For max_message_size: " << max_message_size << " the max allowed payload size is: " << result << + std::endl; ASSERT_LE(result, max_message_size); } @@ -66,7 +67,7 @@ void max_allowed_payload_size( /** * This test checks the get_max_allowed_payload_size() method of the BaseWriter. * The method is called within a loop with different values of max_message_size. - * The test checks that the max_payload_size is always less than max_message_size, + * The test checks that the max_payload_size is always less than max_message_size, * in other case it means an overflow has occurred. */ TEST(BaseWriterTests, calculate_max_payload_size_overflow) From da7b1c659925432152e5b226d595285f29f3f616 Mon Sep 17 00:00:00 2001 From: Eugenio Collado Date: Mon, 9 Dec 2024 14:51:07 +0100 Subject: [PATCH 07/12] Fix log error message Signed-off-by: Eugenio Collado --- src/cpp/rtps/writer/BaseWriter.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/cpp/rtps/writer/BaseWriter.cpp b/src/cpp/rtps/writer/BaseWriter.cpp index a2c8278493b..386badbdba9 100644 --- a/src/cpp/rtps/writer/BaseWriter.cpp +++ b/src/cpp/rtps/writer/BaseWriter.cpp @@ -213,8 +213,9 @@ uint32_t BaseWriter::calculate_max_payload_size( constexpr uint32_t min_fragment_size = 4; if ((overhead + min_fragment_size) > max_data_size) { + auto min_datagram_length = overhead + min_fragment_size + 1 + (datagram_length - max_data_size); EPROSIMA_LOG_ERROR(RTPS_WRITER, "Datagram length '" << datagram_length << "' is too small." << - "At least " << overhead << " bytes are needed to send a message. Fixing fragments to " << + "At least " << min_datagram_length << " bytes are needed to send a message. Fixing fragments to " << min_fragment_size << " bytes."); return min_fragment_size; } From c9a4272aa41a7a81219e95d7973c13ba4193e4ef Mon Sep 17 00:00:00 2001 From: Eugenio Collado Date: Mon, 9 Dec 2024 14:56:52 +0100 Subject: [PATCH 08/12] Fix test fragments not been dropped Signed-off-by: Eugenio Collado --- test/blackbox/common/DDSBlackboxTestsListeners.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/test/blackbox/common/DDSBlackboxTestsListeners.cpp b/test/blackbox/common/DDSBlackboxTestsListeners.cpp index 5d8892e55c2..5da5f206fd4 100644 --- a/test/blackbox/common/DDSBlackboxTestsListeners.cpp +++ b/test/blackbox/common/DDSBlackboxTestsListeners.cpp @@ -2982,9 +2982,6 @@ TEST(DDSStatus, sample_rejected_waitset) .disable_builtin_transport() .add_user_transport_to_pparams(testTransport) .disable_heartbeat_piggyback(true) - .asynchronously(eprosima::fastdds::dds::PublishModeQosPolicyKind::ASYNCHRONOUS_PUBLISH_MODE) - .add_flow_controller_descriptor_to_pparams( // Be sure are sent in separate submessage each DATA. - eprosima::fastdds::rtps::FlowControllerSchedulerPolicy::FIFO, 100, 50) .init(); reader.history_kind(eprosima::fastdds::dds::KEEP_ALL_HISTORY_QOS) From fe4dca77ed1c15706311b6b8c5fef0f42e9af322 Mon Sep 17 00:00:00 2001 From: Eugenio Collado Date: Mon, 9 Dec 2024 15:49:02 +0100 Subject: [PATCH 09/12] Fix corner case RTPSParticipantImpl max_data_size < overhead Signed-off-by: Eugenio Collado --- src/cpp/rtps/participant/RTPSParticipantImpl.cpp | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/cpp/rtps/participant/RTPSParticipantImpl.cpp b/src/cpp/rtps/participant/RTPSParticipantImpl.cpp index 56b202131e3..c77c00fd1e2 100644 --- a/src/cpp/rtps/participant/RTPSParticipantImpl.cpp +++ b/src/cpp/rtps/participant/RTPSParticipantImpl.cpp @@ -2320,20 +2320,22 @@ uint32_t RTPSParticipantImpl::getMaxDataSize() uint32_t RTPSParticipantImpl::calculateMaxDataSize( uint32_t length) { - uint32_t maxDataSize = length; - + // RTPS header + uint32_t overhead = RTPSMESSAGE_HEADER_SIZE; #if HAVE_SECURITY // If there is rtps messsage protection, reduce max size for messages, // because extra data is added on encryption. if (security_attributes_.is_rtps_protected) { - maxDataSize -= m_security_manager.calculate_extra_size_for_rtps_message(); + overhead += m_security_manager.calculate_extra_size_for_rtps_message(); } #endif // if HAVE_SECURITY - // RTPS header - maxDataSize -= RTPSMESSAGE_HEADER_SIZE; - return maxDataSize; + if (length <= overhead) + { + return 0; + } + return length - overhead; } bool RTPSParticipantImpl::networkFactoryHasRegisteredTransports() const From 99f9c289eb34491aaf85b47259c29620cdef3976 Mon Sep 17 00:00:00 2001 From: Eugenio Collado Date: Fri, 13 Dec 2024 11:37:56 +0100 Subject: [PATCH 10/12] Test refactor for windows compilation Signed-off-by: Eugenio Collado --- test/unittest/rtps/history/CMakeLists.txt | 9 +- .../rtps/history/WriterHistoryTests.cpp | 58 ++++++++---- test/unittest/rtps/writer/BaseWriterTests.cpp | 91 ------------------- test/unittest/rtps/writer/CMakeLists.txt | 16 ---- 4 files changed, 45 insertions(+), 129 deletions(-) delete mode 100644 test/unittest/rtps/writer/BaseWriterTests.cpp diff --git a/test/unittest/rtps/history/CMakeLists.txt b/test/unittest/rtps/history/CMakeLists.txt index de2fe5b01a4..9f056484205 100644 --- a/test/unittest/rtps/history/CMakeLists.txt +++ b/test/unittest/rtps/history/CMakeLists.txt @@ -177,11 +177,14 @@ target_compile_definitions(WriterHistoryTests PRIVATE $<$:__INTERNALDEBUG> # Internal debug activated. ) target_include_directories(WriterHistoryTests PRIVATE - ${Asio_INCLUDE_DIR}) + ${Asio_INCLUDE_DIR} + ${THIRDPARTY_BOOST_INCLUDE_DIR} + ${PROJECT_SOURCE_DIR}/src/cpp + ${PROJECT_SOURCE_DIR}/include ${PROJECT_BINARY_DIR}/include) target_link_libraries(WriterHistoryTests fastcdr - fastdds + fastdds foonathan_memory - GTest::gmock + GTest::gtest ${CMAKE_DL_LIBS}) gtest_discover_tests(WriterHistoryTests) diff --git a/test/unittest/rtps/history/WriterHistoryTests.cpp b/test/unittest/rtps/history/WriterHistoryTests.cpp index 50d9705ace9..3082120e7d4 100644 --- a/test/unittest/rtps/history/WriterHistoryTests.cpp +++ b/test/unittest/rtps/history/WriterHistoryTests.cpp @@ -12,7 +12,6 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include #include #include @@ -21,8 +20,6 @@ #include #include -#include - namespace eprosima { namespace fastdds { @@ -33,11 +30,12 @@ using namespace testing; #define MAX_MESSAGE_SIZE 300 void cache_change_fragment( - uint32_t inline_qos_length) + uint32_t max_message_size, + uint32_t inline_qos_length, + bool expected_fragmentation) { uint32_t domain_id = 0; uint32_t initial_reserved_caches = 10; - uint32_t max_message_size = MAX_MESSAGE_SIZE; std::string max_message_size_str = std::to_string(max_message_size); RTPSParticipantAttributes p_attr; @@ -58,19 +56,41 @@ void cache_change_fragment( ASSERT_NE(writer, nullptr); - BaseWriter* bwriter = BaseWriter::downcast(writer); - auto max_allowed_payload_size = bwriter->get_max_allowed_payload_size(); - - CacheChange_t change; - change.writerGUID = bwriter->getGuid(); - change.serializedPayload.length = 3 * max_allowed_payload_size; // Force to setFragmentSize - change.inline_qos.length = inline_qos_length; - - history->add_change(&change); + CacheChange_t* change = history->create_change(ALIVE); + if (expected_fragmentation) + { + change->serializedPayload.length = 3 * max_message_size; + } + else + { + change->serializedPayload.length = max_message_size / 3; + } + change->inline_qos.length = inline_qos_length; + history->add_change(change); - auto result = change.getFragmentSize(); + auto result = change->getFragmentSize(); std::cout << "Fragment size: " << result << std::endl; - ASSERT_NE(result, 0); // Fragment size should always be greater than 0 + if (expected_fragmentation) + { + ASSERT_NE(result, 0); + } + else + { + ASSERT_EQ(result, 0); + } +} + +/** + * This test checks the get_max_allowed_payload_size() method of the BaseWriter class. + * When setting the RTPS Participant Attribute property fastdds.max_message_size to a value lower than the + * message overhead, if the method does not overflow the fragment size will be set. + * If the max_message_size is big enough for the overhead, inline_qos and serializedPayload, + * then no fragmentation will occur. + */ +TEST(WriterHistoryTests, get_max_allowed_payload_size_overflow) +{ + cache_change_fragment(100, 0, true); + cache_change_fragment(MAX_MESSAGE_SIZE, 0, false); } /** @@ -78,11 +98,11 @@ void cache_change_fragment( * The change.serializedPayload.length is set to 3 times the max_allowed_payload_size, so the fragment size should always be set. * In case of an overflow in the attribute high_mark_for_frag_ the fragment size will not be set, which is an error. */ -TEST(WriterHistoryTests, calculate_max_payload_size_overflow) +TEST(WriterHistoryTests, final_high_mark_for_frag_overflow) { for (uint32_t inline_qos_length = 0; inline_qos_length < MAX_MESSAGE_SIZE; inline_qos_length += 40) { - cache_change_fragment(inline_qos_length); + cache_change_fragment(MAX_MESSAGE_SIZE, inline_qos_length, true); } } @@ -94,6 +114,6 @@ int main( int argc, char** argv) { - testing::InitGoogleMock(&argc, argv); + testing::InitGoogleTest(&argc, argv); return RUN_ALL_TESTS(); } diff --git a/test/unittest/rtps/writer/BaseWriterTests.cpp b/test/unittest/rtps/writer/BaseWriterTests.cpp deleted file mode 100644 index 26391271155..00000000000 --- a/test/unittest/rtps/writer/BaseWriterTests.cpp +++ /dev/null @@ -1,91 +0,0 @@ -// Copyright 2020 Proyectos y Sistemas de Mantenimiento SL (eProsima). -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -#include -#include - -#include -#include -#include -#include -#include - -#include - - -namespace eprosima { -namespace fastdds { -namespace rtps { - -using namespace testing; - -void max_allowed_payload_size( - uint32_t max_message_size) -{ - uint32_t domain_id = 0; - uint32_t initial_reserved_caches = 10; - std::string max_message_size_str = std::to_string(max_message_size); - - RTPSParticipantAttributes p_attr; - p_attr.properties.properties().emplace_back("fastdds.max_message_size", max_message_size_str); - RTPSParticipant* participant = RTPSDomain::createParticipant( - domain_id, true, p_attr); - - ASSERT_NE(participant, nullptr); - - HistoryAttributes h_attr; - h_attr.memoryPolicy = DYNAMIC_RESERVE_MEMORY_MODE; - h_attr.initialReservedCaches = initial_reserved_caches; - h_attr.payloadMaxSize = 250; - WriterHistory* history = new WriterHistory(h_attr); - - WriterAttributes w_attr; - RTPSWriter* writer = RTPSDomain::createRTPSWriter(participant, w_attr, history); - - ASSERT_NE(writer, nullptr); - - BaseWriter* bwriter = BaseWriter::downcast(writer); - - auto result = bwriter->get_max_allowed_payload_size(); - std::cout << "For max_message_size: " << max_message_size << " the max allowed payload size is: " << result << - std::endl; - - ASSERT_LE(result, max_message_size); -} - -/** - * This test checks the get_max_allowed_payload_size() method of the BaseWriter. - * The method is called within a loop with different values of max_message_size. - * The test checks that the max_payload_size is always less than max_message_size, - * in other case it means an overflow has occurred. - */ -TEST(BaseWriterTests, calculate_max_payload_size_overflow) -{ - for (uint32_t max_message_size = 200; max_message_size > 150; max_message_size -= 4) - { - max_allowed_payload_size(max_message_size); - } -} - -} // namespace rtps -} // namespace fastdds -} // namespace eprosima - -int main( - int argc, - char** argv) -{ - testing::InitGoogleMock(&argc, argv); - return RUN_ALL_TESTS(); -} diff --git a/test/unittest/rtps/writer/CMakeLists.txt b/test/unittest/rtps/writer/CMakeLists.txt index b4142086155..3e83e12a8e1 100644 --- a/test/unittest/rtps/writer/CMakeLists.txt +++ b/test/unittest/rtps/writer/CMakeLists.txt @@ -108,22 +108,6 @@ target_link_libraries(LivelinessManagerTests PRIVATE ) gtest_discover_tests(LivelinessManagerTests) -set(BASEWRITERTESTS_SOURCE BaseWriterTests.cpp) - -add_executable(BaseWriterTests ${BASEWRITERTESTS_SOURCE}) -target_compile_definitions(BaseWriterTests PRIVATE - BOOST_ASIO_STANDALONE - ASIO_STANDALONE - $<$>,$>:__DEBUG> - $<$:__INTERNALDEBUG> # Internal debug activated. - ) -target_include_directories(BaseWriterTests PRIVATE - ${Asio_INCLUDE_DIR}) -target_link_libraries(BaseWriterTests fastcdr fastdds foonathan_memory - GTest::gmock - ${CMAKE_DL_LIBS}) -gtest_discover_tests(BaseWriterTests) - if(NOT QNX) set(RTPSWRITERTESTS_SOURCE RTPSWriterTests.cpp) From a92ba0f51f8e8b9fa287321b011d06e7d4563420 Mon Sep 17 00:00:00 2001 From: Eugenio Collado Date: Mon, 16 Dec 2024 08:06:46 +0100 Subject: [PATCH 11/12] Fix blackbox test Signed-off-by: Eugenio Collado --- test/blackbox/common/DDSBlackboxTestsListeners.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/blackbox/common/DDSBlackboxTestsListeners.cpp b/test/blackbox/common/DDSBlackboxTestsListeners.cpp index 5da5f206fd4..1ddab3ed6be 100644 --- a/test/blackbox/common/DDSBlackboxTestsListeners.cpp +++ b/test/blackbox/common/DDSBlackboxTestsListeners.cpp @@ -2982,6 +2982,9 @@ TEST(DDSStatus, sample_rejected_waitset) .disable_builtin_transport() .add_user_transport_to_pparams(testTransport) .disable_heartbeat_piggyback(true) + .asynchronously(eprosima::fastdds::dds::PublishModeQosPolicyKind::ASYNCHRONOUS_PUBLISH_MODE) + .add_flow_controller_descriptor_to_pparams( // Be sure are sent in separate submessage each DATA. + eprosima::fastdds::rtps::FlowControllerSchedulerPolicy::FIFO, 300, 300) // Be sure the first message is processed before sending the second. .init(); reader.history_kind(eprosima::fastdds::dds::KEEP_ALL_HISTORY_QOS) From 0467bbb0310b95624ba496a5ddb782a73d38138a Mon Sep 17 00:00:00 2001 From: EugenioCollado <121509066+EugenioCollado@users.noreply.github.com> Date: Tue, 17 Dec 2024 15:53:21 +0100 Subject: [PATCH 12/12] Applied review suggestions Signed-off-by: EugenioCollado <121509066+EugenioCollado@users.noreply.github.com> --- test/unittest/rtps/history/CMakeLists.txt | 5 ----- 1 file changed, 5 deletions(-) diff --git a/test/unittest/rtps/history/CMakeLists.txt b/test/unittest/rtps/history/CMakeLists.txt index 9f056484205..1fe07d13ee6 100644 --- a/test/unittest/rtps/history/CMakeLists.txt +++ b/test/unittest/rtps/history/CMakeLists.txt @@ -176,11 +176,6 @@ target_compile_definitions(WriterHistoryTests PRIVATE $<$>,$>:__DEBUG> $<$:__INTERNALDEBUG> # Internal debug activated. ) -target_include_directories(WriterHistoryTests PRIVATE - ${Asio_INCLUDE_DIR} - ${THIRDPARTY_BOOST_INCLUDE_DIR} - ${PROJECT_SOURCE_DIR}/src/cpp - ${PROJECT_SOURCE_DIR}/include ${PROJECT_BINARY_DIR}/include) target_link_libraries(WriterHistoryTests fastcdr fastdds