From a42a3701f7dd50dbd5c39529a18d0c42a503529c Mon Sep 17 00:00:00 2001 From: Collin Date: Mon, 18 May 2020 10:14:30 -0700 Subject: Fix/issue 1951 (#3371) * remove deadlock danger when removing Connections from a ConnectionMap * fix style, use log debug instead of trace for singular messages * Revert "fix style, use log debug instead of trace for singular messages" This reverts commit c83288ec9fc9007e48044b09ad038edbf612e038. * fix style, use log debug instead of trace for singular messages * restore condition updates lost with merge of old versions Co-authored-by: Frank --- .../hmi/navi_audio_start_stream_request.cc | 6 +-- .../src/commands/hmi/navi_start_stream_request.cc | 6 +-- .../transport_adapter/transport_adapter_impl.h | 10 ++++ .../transport_adapter/transport_adapter_impl.cc | 53 ++++++++++++++++------ 4 files changed, 56 insertions(+), 19 deletions(-) diff --git a/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/hmi/navi_audio_start_stream_request.cc b/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/hmi/navi_audio_start_stream_request.cc index dfebfd3602..47f2f2505e 100644 --- a/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/hmi/navi_audio_start_stream_request.cc +++ b/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/hmi/navi_audio_start_stream_request.cc @@ -173,9 +173,9 @@ void AudioStartStreamRequest::RetryStartSession() { uint32_t curr_retry_number = app->audio_stream_retry_number(); if (curr_retry_number <= retry_number_) { - LOG4CXX_DEBUG(logger_, - "Retry number " << curr_retry_number << " of " - << retry_number_); + LOG4CXX_DEBUG( + logger_, + "Retry number " << curr_retry_number << " of " << retry_number_); MessageHelper::SendAudioStartStream(app->app_id(), application_manager_); app->set_audio_stream_retry_number(++curr_retry_number); } else { diff --git a/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/hmi/navi_start_stream_request.cc b/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/hmi/navi_start_stream_request.cc index 4bf1e9e525..00974085c3 100644 --- a/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/hmi/navi_start_stream_request.cc +++ b/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/hmi/navi_start_stream_request.cc @@ -175,9 +175,9 @@ void NaviStartStreamRequest::RetryStartSession() { uint32_t curr_retry_number = app->video_stream_retry_number(); if (curr_retry_number <= retry_number_) { - LOG4CXX_DEBUG(logger_, - "Retry number " << curr_retry_number << " of " - << retry_number_); + LOG4CXX_DEBUG( + logger_, + "Retry number " << curr_retry_number << " of " << retry_number_); MessageHelper::SendNaviStartStream(app->app_id(), application_manager_); app->set_video_stream_retry_number(++curr_retry_number); } else { diff --git a/src/components/transport_manager/include/transport_manager/transport_adapter/transport_adapter_impl.h b/src/components/transport_manager/include/transport_manager/transport_adapter/transport_adapter_impl.h index f8aa93b4e6..cfc886cb15 100644 --- a/src/components/transport_manager/include/transport_manager/transport_adapter/transport_adapter_impl.h +++ b/src/components/transport_manager/include/transport_manager/transport_adapter/transport_adapter_impl.h @@ -581,6 +581,16 @@ class TransportAdapterImpl : public TransportAdapter, */ DeviceUID GetNextRetryDevice(); + /** + * @brief Remove a connection from the list without triggering + *the connection's destructor inside of a list lock + * + * @param device_handle Device unique identifier. + * @param app_handle Handle of application. + */ + void RemoveConnection(const DeviceUID& device_id, + const ApplicationHandle& app_handle); + /** * @brief Remove specified device * @param device_handle Device unique identifier. diff --git a/src/components/transport_manager/src/transport_adapter/transport_adapter_impl.cc b/src/components/transport_manager/src/transport_adapter/transport_adapter_impl.cc index fcae508747..36f6dd98d0 100644 --- a/src/components/transport_manager/src/transport_adapter/transport_adapter_impl.cc +++ b/src/components/transport_manager/src/transport_adapter/transport_adapter_impl.cc @@ -242,11 +242,9 @@ TransportAdapter::Error TransportAdapterImpl::Connect( const TransportAdapter::Error err = server_connection_factory_->CreateConnection(device_id, app_handle); if (TransportAdapter::OK != err) { - connections_lock_.AcquireForWriting(); if (!pending_app) { - connections_.erase(std::make_pair(device_id, app_handle)); + RemoveConnection(device_id, app_handle); } - connections_lock_.Release(); } LOG4CXX_TRACE(logger_, "exit with error: " << err); return err; @@ -719,14 +717,12 @@ void TransportAdapterImpl::DeviceDisconnected( listener->OnDisconnectDeviceDone(this, device_uid); } - connections_lock_.AcquireForWriting(); for (ApplicationList::const_iterator i = app_list.begin(); i != app_list.end(); ++i) { ApplicationHandle app_handle = *i; - connections_.erase(std::make_pair(device_uid, app_handle)); + RemoveConnection(device_uid, app_handle); } - connections_lock_.Release(); RemoveDevice(device_uid); LOG4CXX_TRACE(logger_, "exit"); @@ -776,9 +772,7 @@ void TransportAdapterImpl::DisconnectDone(const DeviceUID& device_handle, listener->OnDisconnectDeviceDone(this, device_uid); } } - connections_lock_.AcquireForWriting(); - connections_.erase(std::make_pair(device_uid, app_uid)); - connections_lock_.Release(); + RemoveConnection(device_uid, app_uid); if (device_disconnected) { RemoveDevice(device_uid); @@ -973,9 +967,7 @@ void TransportAdapterImpl::ConnectFailed(const DeviceUID& device_handle, LOG4CXX_TRACE(logger_, "enter. device_id: " << &device_uid << ", app_handle: " << &app_uid << ", error: " << &error); - connections_lock_.AcquireForWriting(); - connections_.erase(std::make_pair(device_uid, app_uid)); - connections_lock_.Release(); + RemoveConnection(device_uid, app_uid); for (TransportAdapterListenerList::iterator it = listeners_.begin(); it != listeners_.end(); ++it) { @@ -989,12 +981,13 @@ void TransportAdapterImpl::RemoveFinalizedConnection( const DeviceUID device_uid = device_handle; LOG4CXX_AUTO_TRACE(logger_); { - sync_primitives::AutoWriteLock lock(connections_lock_); + connections_lock_.AcquireForWriting(); auto it_conn = connections_.find(std::make_pair(device_uid, app_handle)); if (connections_.end() == it_conn) { LOG4CXX_WARN(logger_, "Device_id: " << &device_uid << ", app_handle: " << &app_handle << " connection not found"); + connections_lock_.Release(); return; } const ConnectionInfo& info = it_conn->second; @@ -1002,9 +995,21 @@ void TransportAdapterImpl::RemoveFinalizedConnection( LOG4CXX_WARN(logger_, "Device_id: " << &device_uid << ", app_handle: " << &app_handle << " connection not finalized"); + connections_lock_.Release(); return; } + // By copying the info.connection shared pointer into this local variable, + // we can delay the connection's destructor until after + // connections_lock_.Release. + LOG4CXX_DEBUG( + logger_, + "RemoveFinalizedConnection copying connection with Device_id: " + << &device_uid << ", app_handle: " << &app_handle); + ConnectionSPtr connection = info.connection; connections_.erase(it_conn); + connections_lock_.Release(); + LOG4CXX_DEBUG(logger_, + "RemoveFinalizedConnection Connections Lock Released"); } DeviceSptr device = FindDevice(device_handle); @@ -1019,6 +1024,28 @@ void TransportAdapterImpl::RemoveFinalizedConnection( } } +void TransportAdapterImpl::RemoveConnection( + const DeviceUID& device_id, const ApplicationHandle& app_handle) { + ConnectionSPtr connection; + connections_lock_.AcquireForWriting(); + ConnectionMap::const_iterator it = + connections_.find(std::make_pair(device_id, app_handle)); + if (it != connections_.end()) { + // By copying the connection from the map to this shared pointer, + // we can erase the object from the map without triggering the destructor + LOG4CXX_DEBUG(logger_, + "Copying connection with Device_id: " + << &device_id << ", app_handle: " << &app_handle); + connection = it->second.connection; + connections_.erase(it); + } + connections_lock_.Release(); + LOG4CXX_DEBUG(logger_, "Connections Lock Released"); + + // And now, "connection" goes out of scope, triggering the destructor outside + // of the "connections_lock_" +} + void TransportAdapterImpl::AddListener(TransportAdapterListener* listener) { LOG4CXX_TRACE(logger_, "enter"); listeners_.push_back(listener); -- cgit v1.2.1