From 5939a9414cd05bd7158ec6b11cd8991bc111c457 Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Mon, 7 Dec 2020 21:33:02 +0000 Subject: [PATCH] Make sure to lock the mutex protecting client_endpoints_. There were actually 2 problems here: 1. We were missing the lock in check_for_subscription() completely (this is the original issue that caused me to examine this code). 2. We had an accessor for clients_endpoints_ so that external users could get a reference. But with that being the case, any access to the returned reference would be without a lock. Since we were only accessing client_endpoints_ in two places, make some methods in ServicePubListener to do the requested operations while holding the lock, getting rid of the problem. Signed-off-by: Chris Lalancette --- .../custom_service_info.hpp | 39 ++++++++++++------- 1 file changed, 24 insertions(+), 15 deletions(-) diff --git a/rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_service_info.hpp b/rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_service_info.hpp index 7e68c2f29..2b30ab929 100644 --- a/rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_service_info.hpp +++ b/rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_service_info.hpp @@ -125,10 +125,13 @@ class ServicePubListener : public eprosima::fastrtps::PublisherListener check_for_subscription( const eprosima::fastrtps::rtps::GUID_t & guid) { - // Check if the guid is still in the map - if (clients_endpoints_.find(guid) == clients_endpoints_.end()) { - // Client is gone - return client_present_t::GONE; + { + std::lock_guard lock(mutex_); + // Check if the guid is still in the map + if (clients_endpoints_.find(guid) == clients_endpoints_.end()) { + // Client is gone + return client_present_t::GONE; + } } // Wait for subscription if (!wait_for_subscription(guid, std::chrono::milliseconds(100))) { @@ -137,11 +140,23 @@ class ServicePubListener : public eprosima::fastrtps::PublisherListener return client_present_t::YES; } - // Accesors - clients_endpoints_map_t & clients_endpoints() + void endpoint_erase_if_exists(const eprosima::fastrtps::rtps::GUID_t & endpointGuid) + { + std::lock_guard lock(mutex_); + auto endpoint = clients_endpoints_.find(endpointGuid); + if (endpoint != clients_endpoints_.end()) { + clients_endpoints_.erase(endpoint->second); + clients_endpoints_.erase(endpointGuid); + } + } + + void endpoint_add_reader_and_writer( + const eprosima::fastrtps::rtps::GUID_t & readerGuid, + const eprosima::fastrtps::rtps::GUID_t & writerGuid) { std::lock_guard lock(mutex_); - return clients_endpoints_; + clients_endpoints_.emplace(readerGuid, writerGuid); + clients_endpoints_.emplace(writerGuid, readerGuid); } private: @@ -168,12 +183,7 @@ class ServiceListener : public eprosima::fastrtps::SubscriberListener { (void) sub; if (eprosima::fastrtps::rtps::REMOVED_MATCHING == matchingInfo.status) { - auto endpoint = info_->pub_listener_->clients_endpoints().find( - matchingInfo.remoteEndpointGuid); - if (endpoint != info_->pub_listener_->clients_endpoints().end()) { - info_->pub_listener_->clients_endpoints().erase(endpoint->second); - info_->pub_listener_->clients_endpoints().erase(matchingInfo.remoteEndpointGuid); - } + info_->pub_listener_->endpoint_erase_if_exists(matchingInfo.remoteEndpointGuid); } } @@ -202,8 +212,7 @@ class ServiceListener : public eprosima::fastrtps::SubscriberListener // Save both guids in the clients_endpoints map const eprosima::fastrtps::rtps::GUID_t & writer_guid = request.sample_info_.sample_identity.writer_guid(); - info_->pub_listener_->clients_endpoints().emplace(reader_guid, writer_guid); - info_->pub_listener_->clients_endpoints().emplace(writer_guid, reader_guid); + info_->pub_listener_->endpoint_add_reader_and_writer(reader_guid, writer_guid); std::lock_guard lock(internalMutex_);