From 65d41fffc9a2459c0067aa68683f8960509450df Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Thu, 24 Jun 2021 14:47:26 +0200 Subject: [PATCH] Fix mutex lock count on PDPListener (#2023) * Fix mutex lock count on PDPListener (#2020) * Refs 11857. Added regression test. Signed-off-by: Miguel Company * Refs 11857. Fixed issue. Signed-off-by: Miguel Company * Refs 11857. Bonus: fixed PDPServerListener. Signed-off-by: Miguel Company * Refs 11857. Added comments on test. Signed-off-by: Miguel Company (cherry picked from commit 392defcc0008c98cbe3f0cf609fc6b7ef01ea540) # Conflicts: # src/cpp/rtps/builtin/discovery/participant/PDPServerListener.cpp # test/blackbox/CMakeLists.txt # test/blackbox/DDSBlackboxTestsDiscovery.cpp * Fixed conflicts Signed-off-by: Miguel Company * Linters Signed-off-by: Miguel Company * Fix conflicts on CMakeLists.txt Signed-off-by: Miguel Company Co-authored-by: Miguel Company --- .../discovery/participant/PDPListener.cpp | 7 +- .../participant/PDPServerListener.cpp | 3 +- test/blackbox/BlackboxTestsDiscovery.cpp | 2 +- test/blackbox/CMakeLists.txt | 2 + test/blackbox/DDSBlackboxTestsDiscovery.cpp | 55 +++++++++++++++ test/blackbox/api/dds-pim/PubSubReader.hpp | 52 +++++++++++++- test/blackbox/discovery_participant_flags.xml | 67 +++++++++++++++++++ 7 files changed, 179 insertions(+), 9 deletions(-) create mode 100644 test/blackbox/DDSBlackboxTestsDiscovery.cpp create mode 100644 test/blackbox/discovery_participant_flags.xml diff --git a/src/cpp/rtps/builtin/discovery/participant/PDPListener.cpp b/src/cpp/rtps/builtin/discovery/participant/PDPListener.cpp index f80c6029ccf..1178780be96 100644 --- a/src/cpp/rtps/builtin/discovery/participant/PDPListener.cpp +++ b/src/cpp/rtps/builtin/discovery/participant/PDPListener.cpp @@ -125,11 +125,12 @@ void PDPListener::onNewCacheChangeAdded( { // Create a new one when not found pdata = parent_pdp_->createParticipantProxyData(temp_participant_data_, writer_guid); + + reader->getMutex().unlock(); + lock.unlock(); + if (pdata != nullptr) { - reader->getMutex().unlock(); - lock.unlock(); - logInfo(RTPS_PDP_DISCOVERY, "New participant " << pdata->m_guid << " at " << "MTTLoc: " << pdata->metatraffic_locators diff --git a/src/cpp/rtps/builtin/discovery/participant/PDPServerListener.cpp b/src/cpp/rtps/builtin/discovery/participant/PDPServerListener.cpp index 5cb72c8837c..cf939930d8f 100644 --- a/src/cpp/rtps/builtin/discovery/participant/PDPServerListener.cpp +++ b/src/cpp/rtps/builtin/discovery/participant/PDPServerListener.cpp @@ -100,7 +100,6 @@ void PDPServerListener::onNewCacheChangeAdded( { change->instanceHandle = local_data.m_key; guid = local_data.m_guid; - // At this point we can release reader lock. reader->getMutex().unlock(); @@ -152,7 +151,7 @@ void PDPServerListener::onNewCacheChangeAdded( // Included for symmetry with PDPListener to profit from a future updateInfoMatchesEDP override // right now servers update matching on clients that were previously relayed by a server if ( previous_lease_check_status != pdata->should_check_lease_duration - || parent_pdp_->updateInfoMatchesEDP() ) + || parent_pdp_->updateInfoMatchesEDP()) { parent_pdp_->assignRemoteEndpoints(pdata); parent_server_pdp_->queueParticipantForEDPMatch(pdata); diff --git a/test/blackbox/BlackboxTestsDiscovery.cpp b/test/blackbox/BlackboxTestsDiscovery.cpp index add7a5bc334..f8e9836640a 100644 --- a/test/blackbox/BlackboxTestsDiscovery.cpp +++ b/test/blackbox/BlackboxTestsDiscovery.cpp @@ -1025,4 +1025,4 @@ TEST(Discovery, ServerClientEnvironmentSetUp) output.clear(); ASSERT_FALSE(load_environment_server_info(text, output)); -} +} \ No newline at end of file diff --git a/test/blackbox/CMakeLists.txt b/test/blackbox/CMakeLists.txt index 08988537ec0..53cda8ff460 100644 --- a/test/blackbox/CMakeLists.txt +++ b/test/blackbox/CMakeLists.txt @@ -228,6 +228,8 @@ if(NOT ((MSVC OR MSVC_IDE) AND EPROSIMA_INSTALLER) AND fastcdr_FOUND) ${CMAKE_CURRENT_BINARY_DIR}/PubSubWriter.xml) configure_file(${CMAKE_CURRENT_SOURCE_DIR}/PubSubReader.xml.in ${CMAKE_CURRENT_BINARY_DIR}/PubSubReader.xml) + configure_file(${CMAKE_CURRENT_SOURCE_DIR}/discovery_participant_flags.xml + ${CMAKE_CURRENT_BINARY_DIR}/discovery_participant_flags.xml) if(FASTRTPS_API_TESTS) set(BLACKBOXTESTS_FASTRTPS_SOURCE diff --git a/test/blackbox/DDSBlackboxTestsDiscovery.cpp b/test/blackbox/DDSBlackboxTestsDiscovery.cpp new file mode 100644 index 00000000000..470d3562aa9 --- /dev/null +++ b/test/blackbox/DDSBlackboxTestsDiscovery.cpp @@ -0,0 +1,55 @@ +// 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 "BlackboxTests.hpp" + +#include "PubSubReader.hpp" + +// Regression test for redmine issue 11857 +TEST(DDSDiscovery, IgnoreParticipantFlags) +{ + // This participant is created with: + // - ignoreParticipantFlags = FILTER_SAME_PROCESS (will avoid discovery of p2) + // - metatrafficUnicastLocatorList = 127.0.0.1:7399, 127.0.0.1:7398 (to ensure two listening threads are created) + PubSubReader p1(TEST_TOPIC_NAME); + p1.set_xml_filename("discovery_participant_flags.xml"); + p1.set_participant_profile("participant_1"); + p1.init(); + EXPECT_TRUE(p1.isInitialized()); + + // This participant is created with initialPeersList = 127.0.0.1:7399 + // When the announcements of this participant arrive to p1, they will be ignored, and thus p1 will not + // announce itself back to p2. + PubSubReader p2(TEST_TOPIC_NAME); + p2.set_xml_filename("discovery_participant_flags.xml"); + p2.set_participant_profile("participant_2"); + p2.init(); + EXPECT_TRUE(p2.isInitialized()); + EXPECT_FALSE(p2.wait_participant_discovery(1, std::chrono::seconds(1))); + EXPECT_FALSE(p1.wait_participant_discovery(1, std::chrono::seconds(1))); + + // This participant is created with: + // - initialPeersList = 127.0.0.1:7398 + // - a custom guid prefix + // The announcements of this participant will arrive to p1 on a different listening thread. + // Due to the custom prefix, they should not be ignored, and mutual discovery should happen + PubSubReader p3(TEST_TOPIC_NAME); + p3.set_xml_filename("discovery_participant_flags.xml"); + p3.set_participant_profile("participant_3"); + p3.init(); + EXPECT_TRUE(p1.wait_participant_discovery()); + EXPECT_TRUE(p3.wait_participant_discovery()); +} \ No newline at end of file diff --git a/test/blackbox/api/dds-pim/PubSubReader.hpp b/test/blackbox/api/dds-pim/PubSubReader.hpp index 325f937d063..15970b826b8 100644 --- a/test/blackbox/api/dds-pim/PubSubReader.hpp +++ b/test/blackbox/api/dds-pim/PubSubReader.hpp @@ -434,13 +434,52 @@ class PubSubReader std::cout << "Reader discovery finished..." << std::endl; } + bool wait_participant_discovery( + unsigned int min_participants = 1, + std::chrono::seconds timeout = std::chrono::seconds::zero()) + { + bool ret_value = true; + std::unique_lock lock(mutexDiscovery_); + + std::cout << "Reader is waiting discovery of at least " << min_participants << " participants..." << std::endl; + + if (timeout == std::chrono::seconds::zero()) + { + cvDiscovery_.wait(lock, [&]() + { + return participant_matched_ >= min_participants; + }); + } + else + { + if (!cvDiscovery_.wait_for(lock, timeout, [&]() + { + return participant_matched_ >= min_participants; + })) + { + ret_value = false; + } + } + + if (ret_value) + { + std::cout << "Reader participant discovery finished successfully..." << std::endl; + } + else + { + std::cout << "Reader participant discovery finished unsuccessfully..." << std::endl; + } + + return ret_value; + } + bool wait_participant_undiscovery( std::chrono::seconds timeout = std::chrono::seconds::zero()) { bool ret_value = true; std::unique_lock lock(mutexDiscovery_); - std::cout << "Reader is waiting undiscovery..." << std::endl; + std::cout << "Reader is waiting participant undiscovery..." << std::endl; if (timeout == std::chrono::seconds::zero()) { @@ -462,11 +501,11 @@ class PubSubReader if (ret_value) { - std::cout << "Reader undiscovery finished successfully..." << std::endl; + std::cout << "Reader participant undiscovery finished successfully..." << std::endl; } else { - std::cout << "Reader undiscovery finished unsuccessfully..." << std::endl; + std::cout << "Reader participant undiscovery finished unsuccessfully..." << std::endl; } return ret_value; @@ -784,6 +823,13 @@ class PubSubReader return *this; } + PubSubReader& ignore_participant_flags( + eprosima::fastrtps::rtps::ParticipantFilteringFlags_t flags) + { + participant_qos_.wire_protocol().builtin.discovery_config.ignoreParticipantFlags = flags; + return *this; + } + PubSubReader& socket_buffer_size( uint32_t sockerBufferSize) { diff --git a/test/blackbox/discovery_participant_flags.xml b/test/blackbox/discovery_participant_flags.xml new file mode 100644 index 00000000000..ff4d216972c --- /dev/null +++ b/test/blackbox/discovery_participant_flags.xml @@ -0,0 +1,67 @@ + + + + + + + Discovery.IgnoreParticipantFlags.p1 + + + + FILTER_SAME_PROCESS + + + + + 7399 +
127.0.0.1
+ + + + + 7398 +
127.0.0.1
+ + + + + + + + + + Discovery.IgnoreParticipantFlags.p2 + + + + + + 7399 +
127.0.0.1
+ + + + + + + + + + f0.f0.f0.f0.f0.f0.f0.f0.f0.f0.f0.f0 + Discovery.IgnoreParticipantFlags.p3 + + + + + + 7398 +
127.0.0.1
+ + + + + + + + +