Avoid Deadlock using Non-virtual Public Interface and Scoped Locks in C++

Question

I've run into a problem which seems troubling to me. It seems I've found a situation that's easy enough to work-around, but that could lead to problems if a) I have a lapse in concentration while programming or b) somebody else starts implementing my interfaces and doesn't know how to handle this situation.

Here's my basic setup:

I've got an abstract class that I'm using as a generic interface to several data types. I've adopted the non-virtual public interface paradigm (Sutter, 2001) along with scoped locking to provide some thread safety. An example interface class would look something like this (I've left out details about scoped locking and the mutex implementation, as I don't think they're relevant):

class Foo
{
public:
    A( )
    {
        ScopedLock lock( mutex );
        aImp( );
    }
    B( )
    {
        ScopedLock lock( mutex );
        bImp( );
    }
protected:
    aImp( ) = 0;
    bImp( ) = 0;
}

It is then up to the user to implement aImp and bImp, which is where the problem comes in. If aImp performs some operation which uses bImp, it's extremely easy (and almost logical, in some sense) to do this:

class Bar
{
protected:
    aImp( )
    {
        ...
        B( );
        ...
    }
    bImp( )
    {
        ...
    }
}

Deadlock. Of course, the easy solution to this is to always call the protected virtual functions rather than their public variants (replace B( ) with bImp( ) in the above snippet). But it still seems far to easy to hang myself if I make a mistake, or worse yet allow others to hang themselves.

Does anybody have some way to attempt to either stop an implementer of the abstract class from calling those public functions at compile-time, or otherwise help to avoid the deadlock solution?

Just for kicks, some mutexes allow for operation which will avoid deadlock problems. As an example, if I implement this using the windows functions EnterCriticalSection and LeaveCriticalSection, there's no issue. But I'd rather avoid platform specific functionality. I'm currently using boost::mutex and boost::shared_mutex in my scoped lock implementation, and as far as I've seen it doesn't attempt to avoid deadlock (which I think I almost prefer).

Answer 1

Your mutex must not be a recursive mutex. If its not a recursive mutex, a second attempt to lock the mutex in the same thread will result in that thread blocking. Since that thread locked the mutex, but is blocked on that mutex, you have a deadlock.

You probably want to look at:

boost::recursive_mutex

http://www.boost.org/doc/libs/1_32_0/doc/html/recursive_mutex.html

It's supposed to implement recursive mutex behavior cross platform.Note Win32 CRITICAL_SECTION's (used via Enter/LeaveCriticalSection) are recursive, which would create the behavior you describe.

Answer 2

Using private inheritance will potentially solve your problem:

class Foo
{
public:
  void A( )
    {
      ScopedLock lock( mutex );
      aImp( );
    }
  void B( )
    {
      ScopedLock lock( mutex );
      bImp( );
    }

protected:
  virtual void aImp( ) = 0;
  virtual void bImp( ) = 0;
};

class FooMiddle : private Foo
{
public:
  using Foo::aImp;
  using Foo::bImp;
};

class Bar : public FooMiddle
{
  virtual void aImpl ()
  {
    bImp ();
    B ();                   // Compile error - B is private
  }
};

Deriving from Foo privately, and then using FooMiddle ensures that Bar doesn't have access to A or B. However, bar is still able to override aImp and bImp, and the using declarations in FooMiddle mean that these can still be called from Bar.

Alternatively, an option that will help but not solve the problem is to use the Pimpl pattern. You'd end up with something as follows:

class FooImpl
{
public:
  virtual void aImp( ) = 0;
  virtual void bImp( ) = 0;
};

class Foo
{
public:
  void A( )
    {
      ScopedLock lock( mutex );
      m_impl->aImp( );
    }
  void B( )
    {
      ScopedLock lock( mutex );
      m_impl->bImp( );
    }

private:
  FooImpl * m_impl;
}

The benefit is that in the classes deriving from FooImpl, they no longer have a "Foo" object and so cannot easily call "A" or "B".

Answer 3

While a recursive lock would solve your problem, I've always felt that, while sometimes necessary, in many situations a recursive lock is used as an easy way out, locking way too much.

Your posted code is obviously simplified for demonstration purposes, so I'm not sure if it'll apply.

As an example, let's say using resource X is not threadsafe. You've got something like.

A() {
   ScopedLock
   use(x)
   aImp()
   use(x)
}

aImp() {
   ScopedLock
   use(x)
}

Obviously, this would result in a deadlock.

Using your locks much narrower however, would eliminate the problem. Using locks in an as small scope as possible is always a good idea, both for performance reasons, as deadlock avoidance.

A() {
   {
      ScopedLock
      use(x)
   }
   aImp()
   {
      ScopedLock
      use(x)
   }
}

You get the idea.

I'm aware this is not always possible (or would lead to horribly innefficient code), without knowing more details I don't know if it applies to your problem. But thought it was worth posting anyway.