remove deadlock danger when removing Connections from a ConnectionMap #1952
Conversation
| @@ -756,25 +748,53 @@ void TransportAdapterImpl::RemoveFinalizedConnection( | |||
| const DeviceUID& device_handle, const ApplicationHandle& app_handle) { | |||
| const DeviceUID device_uid = device_handle; | |||
| LOG4CXX_AUTO_TRACE(logger_); | |||
| sync_primitives::AutoWriteLock lock(connections_lock_); | |||
| connections_lock_.AcquireForWriting(); | |||
There was a problem hiding this comment.
@fronneburg why did you remove AutoLock in that case?
There was a problem hiding this comment.
@LuxoftAKutsan we need to make sure that the lock is released first, and THEN let "connection" go out of scope.
| void TransportAdapterImpl::RemoveConnection( | ||
| const DeviceUID& device_id, const ApplicationHandle& app_handle) { | ||
| ConnectionSPtr connection; | ||
| connections_lock_.AcquireForWriting(); |
There was a problem hiding this comment.
@fronneburg I would prefer using RAII here
There was a problem hiding this comment.
Same response here.
|
Can one of the admins verify this patch? |
|
@fronneburg Why is it important that the calling the connection destructor is delayed until after the lock is released? |
|
@JackLivio, we found a deadlock when the connection destructor (actually the "ThreadedConnection" Xevo implementation) tried to "join" a thread that was waiting on the "connections_lock_" in TransportAdapterImpl::ConnectionFinished. |
|
Closed in favor of #3371 which is based on latest develop and changes the log type from TRACE to DEBUG. |
Fixes #1951
This PR is [ready] for review.
Risk
This PR makes [no] API changes.
Summary
Created a new "RemoveConnection" method which safely removes a Connection object from the ConnectionMap. Before calling "erase" on the connection object, the object's ref-count is incremented by assigning it to a local "ConnectionSPtr" variable. We then call "erase", NOT triggering the destructor, then release the "connections_lock_", and then let the local variable go out of scope, thus delaying the destructor until after the lock was released.
CLA