From acfd6187a24f259f1c97fc0de0b4067d6a8b600c Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Thu, 3 Nov 2022 07:49:27 +0100 Subject: [PATCH] Fix dataraces when creating DataWriters (#3051) * 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 * Fixed conflicts Signed-off-by: Miguel Company * Fix build on RTPSBlackboxTestsBasic.cpp Signed-off-by: Miguel Company Signed-off-by: Miguel Company Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Miguel Company --- .../fastdds/domain/DomainParticipantImpl.cpp | 8 +- .../fastdds/domain/DomainParticipantImpl.hpp | 5 +- .../blackbox/common/DDSBlackboxTestsBasic.cpp | 110 ++++++++++++++++++ .../common/RTPSBlackboxTestsBasic.cpp | 16 ++- 4 files changed, 128 insertions(+), 11 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..ec2701a16cd 100644 --- a/test/blackbox/common/RTPSBlackboxTestsBasic.cpp +++ b/test/blackbox/common/RTPSBlackboxTestsBasic.cpp @@ -14,16 +14,20 @@ #include "BlackboxTests.hpp" +#include +#include +#include + +#include + +#include +#include +#include + #include "RTPSAsSocketReader.hpp" #include "RTPSAsSocketWriter.hpp" #include "RTPSWithRegistrationReader.hpp" #include "RTPSWithRegistrationWriter.hpp" -#include -#include - -#include - -#include using namespace eprosima::fastrtps; using namespace eprosima::fastrtps::rtps;