Skip to content

Commit

Permalink
Fix mutex lock count on PDPListener (#2020)
Browse files Browse the repository at this point in the history
* Refs 11857. Added regression test.

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

* Refs 11857. Fixed issue.

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

* Refs 11857. Bonus: fixed PDPServerListener.

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

* Refs 11857. Added comments on test.

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
  • Loading branch information
MiguelCompany authored Jun 24, 2021
1 parent 1abda23 commit 392defc
Show file tree
Hide file tree
Showing 7 changed files with 185 additions and 13 deletions.
7 changes: 4 additions & 3 deletions src/cpp/rtps/builtin/discovery/participant/PDPListener.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
13 changes: 7 additions & 6 deletions src/cpp/rtps/builtin/discovery/participant/PDPServerListener.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -123,22 +123,23 @@ void PDPServerListener::onNewCacheChangeAdded(
// Deserialize the payload to access the discovery info
CDRMessage_t msg(change->serializedPayload);
temp_participant_data_.clear();
auto participant_data = temp_participant_data_;

if (temp_participant_data_.readFromCDRMessage(
if (participant_data.readFromCDRMessage(
&msg,
true,
pdp_server()->getRTPSParticipant()->network_factory(),
pdp_server()->getRTPSParticipant()->has_shm_transport()))
{
/* Check PID_VENDOR_ID */
if (temp_participant_data_.m_VendorId != fastrtps::rtps::c_VendorId_eProsima)
if (participant_data.m_VendorId != fastrtps::rtps::c_VendorId_eProsima)
{
logInfo(RTPS_PDP_LISTENER,
"DATA(p|Up) from different vendor is not supported for Discover-Server operation");
return;
}

fastrtps::ParameterPropertyList_t properties = temp_participant_data_.m_properties;
fastrtps::ParameterPropertyList_t properties = participant_data.m_properties;

/* Check DS_VERSION */
auto ds_version = std::find_if(
Expand Down Expand Up @@ -244,7 +245,7 @@ void PDPServerListener::onNewCacheChangeAdded(
if (pdp_server()->discovery_db().update(
change.get(),
ddb::DiscoveryParticipantChangeData(
temp_participant_data_.metatraffic_locators,
participant_data.metatraffic_locators,
is_client,
is_local)))
{
Expand Down Expand Up @@ -303,7 +304,7 @@ void PDPServerListener::onNewCacheChangeAdded(
logInfo(RTPS_PDP_LISTENER, "Registering a new participant: " << guid);

// Create a new participant proxy entry
pdata = pdp_server()->createParticipantProxyData(temp_participant_data_, writer_guid);
pdata = pdp_server()->createParticipantProxyData(participant_data, writer_guid);
// Realease PDP mutex
lock.unlock();

Expand All @@ -326,7 +327,7 @@ void PDPServerListener::onNewCacheChangeAdded(
else
{
// Update proxy
pdata->updateData(temp_participant_data_);
pdata->updateData(participant_data);
pdata->isAlive = true;
// Realease PDP mutex
lock.unlock();
Expand Down
2 changes: 2 additions & 0 deletions test/blackbox/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,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)
configure_file(${CMAKE_CURRENT_SOURCE_DIR}/persistence.xml
${CMAKE_CURRENT_BINARY_DIR}/persistence.xml)
configure_file(${CMAKE_CURRENT_SOURCE_DIR}/StatisticsDomainParticipant.xml
Expand Down
52 changes: 49 additions & 3 deletions test/blackbox/api/dds-pim/PubSubReader.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -568,13 +568,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<std::mutex> 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<std::mutex> 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())
{
Expand All @@ -596,11 +635,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;
Expand Down Expand Up @@ -1053,6 +1092,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)
{
Expand Down
2 changes: 1 addition & 1 deletion test/blackbox/common/BlackboxTestsDiscovery.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1264,4 +1264,4 @@ TEST(Discovery, ServerClientEnvironmentSetUp)
output.clear();
ASSERT_FALSE(load_environment_server_info(text, output));

}
}
55 changes: 55 additions & 0 deletions test/blackbox/common/DDSBlackboxTestsDiscovery.cpp
Original file line number Diff line number Diff line change
@@ -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 <gtest/gtest.h>

#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<HelloWorldType> 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<HelloWorldType> 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<HelloWorldType> 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());
}
67 changes: 67 additions & 0 deletions test/blackbox/discovery_participant_flags.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
<?xml version="1.0" encoding="utf-8" ?>
<dds xmlns="http://www.eprosima.com/XMLSchemas/fastRTPS_Profiles">
<profiles>

<participant profile_name="participant_1">
<rtps>
<name>Discovery.IgnoreParticipantFlags.p1</name>

<builtin>
<discovery_config>
<ignoreParticipantFlags>FILTER_SAME_PROCESS</ignoreParticipantFlags>
</discovery_config>
<metatrafficUnicastLocatorList>
<locator>
<udpv4>
<port>7399</port>
<address>127.0.0.1</address>
</udpv4>
</locator>
<locator>
<udpv4>
<port>7398</port>
<address>127.0.0.1</address>
</udpv4>
</locator>
</metatrafficUnicastLocatorList>
</builtin>
</rtps>
</participant>

<participant profile_name="participant_2">
<rtps>
<name>Discovery.IgnoreParticipantFlags.p2</name>

<builtin>
<initialPeersList>
<locator>
<udpv4>
<port>7399</port>
<address>127.0.0.1</address>
</udpv4>
</locator>
</initialPeersList>
</builtin>
</rtps>
</participant>

<participant profile_name="participant_3">
<rtps>
<prefix>f0.f0.f0.f0.f0.f0.f0.f0.f0.f0.f0.f0</prefix>
<name>Discovery.IgnoreParticipantFlags.p3</name>

<builtin>
<initialPeersList>
<locator>
<udpv4>
<port>7398</port>
<address>127.0.0.1</address>
</udpv4>
</locator>
</initialPeersList>
</builtin>
</rtps>
</participant>

</profiles>
</dds>

0 comments on commit 392defc

Please sign in to comment.