From 8813cddc955a057918aafcb25566d9709ff8b5a1 Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Fri, 28 Oct 2022 11:54:34 +0200 Subject: [PATCH 1/3] Fix dataraces when creating DataWriters (#3015) * Refs #15905: Declare the PublishMode running flag as atomic Signed-off-by: Eduardo Ponz * Refs #15905: Add RTPS regression test Signed-off-by: Eduardo Ponz * Refs #15905: Add DomainParticipantImpl::create_instance_handle data race regression test Signed-off-by: Eduardo Ponz * Refs #15905: Set DomainParticipantImpl::next_instance_id_ as atomic Signed-off-by: Eduardo Ponz * Refs #15905: Apply suggestions Signed-off-by: Eduardo Ponz Signed-off-by: Eduardo Ponz (cherry picked from commit 4391864a8e597f767e7b7bc56b34eac60dc0646c) Co-authored-by: Eduardo Ponz Segrelles (cherry picked from commit 90777eccda61379a5ba325295c1f505f7810e55f) # Conflicts: # test/blackbox/common/RTPSBlackboxTestsBasic.cpp --- .../fastdds/domain/DomainParticipantImpl.cpp | 8 +- .../fastdds/domain/DomainParticipantImpl.hpp | 5 +- .../blackbox/common/DDSBlackboxTestsBasic.cpp | 110 ++++++++++++++++++ .../common/RTPSBlackboxTestsBasic.cpp | 13 +++ 4 files changed, 131 insertions(+), 5 deletions(-) create mode 100644 test/blackbox/common/DDSBlackboxTestsBasic.cpp diff --git a/src/cpp/fastdds/domain/DomainParticipantImpl.cpp b/src/cpp/fastdds/domain/DomainParticipantImpl.cpp index 7189db05c40..64e533f7462 100644 --- a/src/cpp/fastdds/domain/DomainParticipantImpl.cpp +++ b/src/cpp/fastdds/domain/DomainParticipantImpl.cpp @@ -1805,12 +1805,12 @@ void DomainParticipantImpl::create_instance_handle( { using eprosima::fastrtps::rtps::octet; - ++next_instance_id_; + uint32_t id = ++next_instance_id_; handle = guid_; handle.value[15] = 0x01; // Vendor specific; - handle.value[14] = static_cast(next_instance_id_ & 0xFF); - handle.value[13] = static_cast((next_instance_id_ >> 8) & 0xFF); - handle.value[12] = static_cast((next_instance_id_ >> 16) & 0xFF); + handle.value[14] = static_cast(id & 0xFF); + handle.value[13] = static_cast((id >> 8) & 0xFF); + handle.value[12] = static_cast((id >> 16) & 0xFF); } DomainParticipantListener* DomainParticipantImpl::get_listener_for( diff --git a/src/cpp/fastdds/domain/DomainParticipantImpl.hpp b/src/cpp/fastdds/domain/DomainParticipantImpl.hpp index 060a86dfdc6..cc31853c89b 100644 --- a/src/cpp/fastdds/domain/DomainParticipantImpl.hpp +++ b/src/cpp/fastdds/domain/DomainParticipantImpl.hpp @@ -20,6 +20,9 @@ #ifndef _FASTDDS_PARTICIPANTIMPL_HPP_ #define _FASTDDS_PARTICIPANTIMPL_HPP_ #ifndef DOXYGEN_SHOULD_SKIP_THIS_PUBLIC + +#include + #include #include #include @@ -402,7 +405,7 @@ class DomainParticipantImpl fastrtps::rtps::GUID_t guid_; //!For instance handle creation - uint32_t next_instance_id_; + std::atomic next_instance_id_; //!Participant Qos DomainParticipantQos qos_; diff --git a/test/blackbox/common/DDSBlackboxTestsBasic.cpp b/test/blackbox/common/DDSBlackboxTestsBasic.cpp new file mode 100644 index 00000000000..63a0e68e025 --- /dev/null +++ b/test/blackbox/common/DDSBlackboxTestsBasic.cpp @@ -0,0 +1,110 @@ +// Copyright 2022 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 +#include +#include +#include + +#include "BlackboxTests.hpp" + +namespace eprosima { +namespace fastdds { +namespace dds { + +using ReturnCode_t = eprosima::fastrtps::types::ReturnCode_t; + +/** + * This test checks a race condition when calling DomainParticipantImpl::create_instance_handle() + * from different threads simultaneously. This was resulting in a `free(): invalid pointer` crash + * when deleting publishers created this way, as there was a clash in their respective instance + * handles. Not only did the crash occur, but it was also reported by TSan. + * + * The test spawns 200 threads, each creating a publisher and then waiting on a command from the + * main thread to delete them (so all of them at deleted at the same time). + */ +TEST(DDSBasic, MultithreadedPublisherCreation) +{ + /* Get factory */ + DomainParticipantFactory* factory = DomainParticipantFactory::get_instance(); + ASSERT_NE(nullptr, factory); + + /* Create participant */ + DomainParticipant* participant = factory->create_participant((uint32_t)GET_PID() % 230, PARTICIPANT_QOS_DEFAULT); + ASSERT_NE(nullptr, participant); + + /* Test synchronization variables */ + std::mutex finish_mtx; + std::condition_variable finish_cv; + bool should_finish = false; + + /* Function to create publishers, deleting them on command */ + auto thread_run = + [participant, &finish_mtx, &finish_cv, &should_finish]() + { + /* Create publisher */ + Publisher* publisher = participant->create_publisher(PUBLISHER_QOS_DEFAULT); + ASSERT_NE(nullptr, publisher); + + { + /* Wait for test completion request */ + std::unique_lock lock(finish_mtx); + finish_cv.wait(lock, [&should_finish]() + { + return should_finish; + }); + } + + /* Delete publisher */ + ASSERT_EQ(ReturnCode_t::RETCODE_OK, participant->delete_publisher(publisher)); + }; + + { + /* Create threads */ + std::vector threads; + for (size_t i = 0; i < 200; i++) + { + threads.push_back(std::thread(thread_run)); + } + + /* Command threads to delete their publishers */ + { + std::lock_guard guard(finish_mtx); + should_finish = true; + finish_cv.notify_all(); + } + + /* Wait for all threads to join */ + for (std::thread& thr : threads) + { + thr.join(); + } + } + + /* Clean up */ + ASSERT_EQ(ReturnCode_t::RETCODE_OK, factory->delete_participant(participant)); +} + +} // namespace dds +} // namespace fastdds +} // namespace eprosima diff --git a/test/blackbox/common/RTPSBlackboxTestsBasic.cpp b/test/blackbox/common/RTPSBlackboxTestsBasic.cpp index c5cbb04f232..315489646fe 100644 --- a/test/blackbox/common/RTPSBlackboxTestsBasic.cpp +++ b/test/blackbox/common/RTPSBlackboxTestsBasic.cpp @@ -14,16 +14,29 @@ #include "BlackboxTests.hpp" +#include +#include +#include + +#include + +#include +#include +#include + #include "RTPSAsSocketReader.hpp" #include "RTPSAsSocketWriter.hpp" #include "RTPSWithRegistrationReader.hpp" #include "RTPSWithRegistrationWriter.hpp" +<<<<<<< HEAD #include #include #include #include +======= +>>>>>>> 90777eccd (Fix dataraces when creating DataWriters (#3015)) using namespace eprosima::fastrtps; using namespace eprosima::fastrtps::rtps; From d3829a7fd71f42ee4908b2f3bd221171406df758 Mon Sep 17 00:00:00 2001 From: Miguel Company Date: Fri, 28 Oct 2022 12:00:02 +0200 Subject: [PATCH 2/3] Fixed conflicts Signed-off-by: Miguel Company --- test/blackbox/common/RTPSBlackboxTestsBasic.cpp | 9 --------- 1 file changed, 9 deletions(-) diff --git a/test/blackbox/common/RTPSBlackboxTestsBasic.cpp b/test/blackbox/common/RTPSBlackboxTestsBasic.cpp index 315489646fe..38fef360096 100644 --- a/test/blackbox/common/RTPSBlackboxTestsBasic.cpp +++ b/test/blackbox/common/RTPSBlackboxTestsBasic.cpp @@ -28,15 +28,6 @@ #include "RTPSAsSocketWriter.hpp" #include "RTPSWithRegistrationReader.hpp" #include "RTPSWithRegistrationWriter.hpp" -<<<<<<< HEAD -#include -#include - -#include - -#include -======= ->>>>>>> 90777eccd (Fix dataraces when creating DataWriters (#3015)) using namespace eprosima::fastrtps; using namespace eprosima::fastrtps::rtps; From 30801fbe2f2c648d0f52766cc6fae256acd81a5e Mon Sep 17 00:00:00 2001 From: Miguel Company Date: Fri, 28 Oct 2022 13:23:24 +0200 Subject: [PATCH 3/3] Fix build on RTPSBlackboxTestsBasic.cpp Signed-off-by: Miguel Company --- test/blackbox/common/RTPSBlackboxTestsBasic.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/blackbox/common/RTPSBlackboxTestsBasic.cpp b/test/blackbox/common/RTPSBlackboxTestsBasic.cpp index 38fef360096..ec2701a16cd 100644 --- a/test/blackbox/common/RTPSBlackboxTestsBasic.cpp +++ b/test/blackbox/common/RTPSBlackboxTestsBasic.cpp @@ -21,8 +21,8 @@ #include #include +#include #include -#include #include "RTPSAsSocketReader.hpp" #include "RTPSAsSocketWriter.hpp"