How to avoid deadlock when the called method uses the same lock that the caller already locked?

Question

How to avoid deadlock when the called method uses the same lock that the caller already locked?

I have and method called closeUnusedConnections(), that creates a std::unique_lock but its caller already created a std::unique_lock with the same std::mutex: Foo::m_myMutex.

Do I have to release the lock before calling the subroutine?

Obs.: I can't integrate both methods because the closeUnusedConnections is also called independently.

Some code:

void Foo::closeUnusedConnections()
{   
   std::unique_lock< std::mutex > lock( m_mtx );
   // do stuff
}

Foo::CommNodePtr Foo::getConnection()
{   
   std::unique_lock< std::mutex > lock( m_mtx );
   // do stuff
   if( true /*some condition*/ )
   {
      lock.unlock();     // The goal is to avoid this unlock
                         // someone could get the lock between
                         // this unlock until closeUnusedConnections's lock.
      closeUnusedConnections(); 
   }
   // etc
}

Answer 1

The pattern of "my class owns a mutex, it locks it on all operations" is a poor one. It doesn't compose or scale.

Using recursive mutexes patches over some problems, but both has a higher cost and doesn't solve the larger problem that mutexes should be either extremely narrow or extremely explicit.

Mutexes are dangerous. Deadlocks are hard to avoid. Shared state is a minefield.

One approach is to write a wrapper around a class like this:

template<class T>
struct mutex_guarded {
  template<class F>
  auto operator->*( F&& f )
  -> decltype( std::forward<F>(std::declval<T&>()) )
  {
    std::unique_lock<std::mutex> l(m_mtx);
    return std::forward<F>(f)(t);
  }
  template<class F>
  auto operator->*( F&& f ) const
  -> decltype( std::forward<F>(std::declval<T const&>()) )
  {
    std::unique_lock<std::mutex> l(m_mtx);
    return std::forward<F>(f)(t);
  }
  mutex_guarded(T tin):t(std::move(tin)) {}
  T copy_out() const {
    return (*this)->*[](auto& t){ return t; };
  }
  T move_out() {
    return (*this)->*[](auto& t){ return std::move(t); };
  }
  template<class U>
  void assign(U&& u) {
    return (*this)->*[&u](auto& t) { t = std::forward<U>(u); }
  }
  mutex_guarded(T const& tin):t(tin) {}
  mutex_guarded(T && tin):t(std::move(tin)) {}
  mutex_guarded(mutex_guarded const& tin):
    mutex_guarded(tin.copy_out())
  {}
  mutex_guarded(mutex_guarded && tin):
    mutex_guarded(tin.move_out())
  {}
  mutex_guarded& operator=(T const& tin) {
    assign( tin );
    return *this;
  }
  mutex_guarded& operator=(T&& tin) {
    assign( std::move(tin) );
    return *this;
  }
  mutex_guarded& operator=(mutex_guarded const& tin) {
    return *this = tin.copy_out();
  }
  mutex_guarded& operator=(mutex_guarded&& tin) {
    return *this = tin.move_out();
  }
private:
  std::mutex m_mtx;
  T t;
};

with some bonus augmentation for copy/move/construct.

Now a mutex_guarded<Foo> foo can be interacted with by doing

foo->*[&](auto& foo){ foo.closeUnusedConnections(); };

and the locking happens in the mutex_guarded, not within Foo itself. Foo lives without any thread safety itself, and its own calls don't cause problems.

This is still dangerous. Mutexes are in general dangerous, and the less tightly you control them the more dangerous they are.

Answer 2

I believe it is quite dangerous to have an accessor to an object outside of a single owner thread it that object can be deleted, closed in your case, by other threads. The mutex is supposed to prevent these things from happening while they are in possession of another thread.

nos's solution would work for you in what you describe but I feel you'll end up running into other problems down the road since nothing would stop one thread from closing the connection while it is being used by another thread after a call to getConnection.

I kind of wonder what you are attempting to accomplish by calling getConnection with the possibility that the call will actually return a connection that is closed.

My advice is to rethink your work flow so that only a single thread has access to your port at any given moment even if that means making it where only a single thread can ever use the port and other threads must make work requests.

Answer 3

Create a private function that does the work without grabbing the lock

//call with m_mtx locked
void Foo::unlockedCloseUnusedConnections()
{     
   // do stuff
}

Your public function just becomes

 void Foo::closeUnusedConnections()
 {
     std::unique_lock< std::mutex > lock( m_mtx );
     unlockedCloseUnusedConnections();
 }

Your Foo::getConnection() function calls unlockedCloseUnusedConnections() , since it has already taken the lock.