Need some feedback on how to make a class "thread-safe"

Question

I'm currently learning how to do multithreading in C++. One of my learning projects is a Tetris game. In this project I have a Game class that contains all game state data. It has methods for moving the block around and a few other things. This object will be accessed by the user (who will use the arrow keys to move the block, from the main thread) and at the same time a threaded timer is implementing the gravity on the active block (periodically lowering it).

At first I thought that I could make the Game class thread safe by adding a mutex member variable and lock it inside each method call. But problem with this is that it only protects individual method calls, not changes that involve multiple method calls. For example:

// This is not thread-safe.
while (!game.isGameOver())
{
    game.dropCurrentBlock();
}

One solution that I tried is adding an accessor method for the mutex variable to lock it also from the outside:

// Extra scope added to limit the lifetime of the scoped_lock.    
{
    // => deadlock, unless a recursive mutex is used
    boost::mutex::scoped_lock lock(game.getMutex());
    while (!game.isGameOver())
    {
        game.dropCurrentBlock();
    }
}

However, this will deadlock unless a recursive mutex is used. Now, looking at some posts on StackOverflow, there seems to be a majority that strongly disapproves the use of recursive mutexes.

But if recursive mutexes are a non-option, doesn't that mean that it becomes impossible to create a thread-safe class (that supports coordinated changes)?

The only valid solution seems to be to never lock the mutex inside the method calls, and instead always rely on the user to do the locking from the outside.

However, if that is the case, then wouldn't it be better to simply leave the Game class as it is, and create a wrapper class that pairs a Game object with a mutex?

Update

I gave the wrapper idea a try and created a class called ThreadSafeGame (cpp) that looks like this:

class ThreadSafeGame
{
public:
    ThreadSafeGame(std::auto_ptr<Game> inGame) : mGame(inGame.release) {}

    const Game * getGame() const
    { return mGame.get(); }

    Game * getGame()
    { return mGame.get(); }

    boost::mutex & getMutex() const
    { return mMutex; }

private:
    boost::scoped_ptr<Game> mGame;
    mutable boost::mutex mMutex;
};

// Usage example, assuming "threadSafeGame" is pointer to a ThreadSafeGame object.    
{
    // First lock the game object.
    boost::mutex::scoped_lock lock(threadSafeGame->getMutex());

    // Then access it.
    Game * game = threadSafeGame->getGame();
    game->move(Direction_Down);
}

It has the same drawback in that it depends on the user to lock the mutex from the outside. But apart from that this seems like a workable solution to me.

Am I doing it right?

Answer 1

In your case you have a big class Game that requires synchronization. You've already noted the problem where each method is synchronized but one stil can't perform the operations safely.

If we look at the class ThreadSafeGame I think the interface for it could be improved so that we only can access the game state if we are in synchronized mode. There are several ways to do it. One way is to make getGame return a class that holds both a lock and the instance at the same time. You define operator-> on that class so that it returns Game*. When the class is destroyed the lock is released.

My examples uses some C++0x features (lambdas, move semantics, auto and decltype) but it's not impossible to make it C++98 compatible.

I will demonstrate another way to do it as well using a visit method:

template<typename TValue>
struct threadsafe_container : boost::noncopyable
{
   explicit threadsafe_container (TValue && value)
      :  m_value (std::move (value))
   {
   }

   // visit executes action when have the lock
   template<typename TAction>
   auto visit (TAction action) -> decltype (action (m_value))
   {
      boost::mutex::scope_lock lock (&m_mutex);

      TValue & value (m_value);

      return action (value);
   }

private:
   boost::mutex m_mutex;
   TValue m_value;
};

// Extra paranthesis necessary otherwise c++ interprets it as a function declaration
threadsafe_container<game> s_state ((ConstructAGameSomehow ())); 

void EndTheGame ()
{
   s_state.visit ([](game & state)
      {
         // In here we are synchronized
         while (!state.is_game_over ()) 
         { 
            state.drop_current_block (); 
         } 
      });
}

bool IsGameOver ()
{
   return s_state.visit ([](game & state) {return state.is_game_over ();});
}

And the lock class method:

template<typename TValue>
struct threadsafe_container2 : boost::noncopyable
{
   struct lock : boost::noncopyable
   {
      lock (TValue * value, mutex * mtx)
         :  m_value  (value)
         ,  m_lock   (mtx)
      {
      }

      // Support move semantics
      lock (lock && l);

      TValue * get () const 
      {
         return m_value;
      }

      TValue * operator-> () const
      {
         return get ();
      }
   private:
      TValue *                   m_value;
      boost::mutex::scope_lock   m_lock;
   };

   explicit threadsafe_container2 (TValue && value)
      :  m_value (std::move (value))
   {
   }

   lock get ()
   {
      return lock (&m_value, &m_mutex);
   }

private:
   boost::mutex   m_mutex;
   TValue         m_value;
};

// Extra paranthesis necessary otherwise c++ interprets it as a function declaration
threadsafe_container2<game> s_state ((ConstructAGameSomehow ())); 

void EndTheGame ()
{
   auto lock = s_state2.get ();
   // In here we are synchronized
   while (!lock->is_game_over ()) 
   { 
      lock->drop_current_block ();   
   } 
}

bool IsGameOver ()
{
   auto lock = s_state2.get ();
   // In here we are synchronized
   reutrn lock->is_game_over ();
}

But the basic idea is the same. Make sure we can only access the Game state when we have a lock. Of course this is C++ so we can always find ways to break the rules but to quote Herb Sutter: Protect against Murphy not against Machiavelli ie. protect yourself from mistake not from programmers that set out to break the rules (they will always find a way to do it)

Now to the second part of the comment:

Coarse grained locking versus fine grained locking? Coarse grained is rather easy to implement but suffers from performance issues, fine-grained locking is very tricky to get right but might have better performance.

I would say; do your best to avoid locking alltogether. With that I don't mean; cross my thumbs and hope I don't get race conditions. I mean structure your program so that only one thread manages mutable state and isolate this mutable state so it can't be mutated by mistake by several threads.

In your case you have an input thread accepting user inputs and updates the state. One thread updates the game state on timer.

Instead what about the input thread accepting user state posts a message to Game state manager thread saying : "This is what did user did". The game state thread then consumes messages and acts appropriately. That way the game state is only accessed by that thread and no race conditions and dead-locks can occurs.

This is sometimes called the "Active Object Pattern".

Alert readers say: But hey the message queue must be thread-safe! That's true but a message queue is comparativly trivial to make thread-safe.

IMO this pattern is one of the most important to build maintainble concurrent projects.

Answer 2

Verifying that an object is "thread-safe" is pointless, fundamentally. You can't just get any old object and stick a mutex in and claim to have multithreadable code. The correct design is to, well, design your program. Nobody can tell you how your program should be designed, but you'll need to work out an actual threading design, and you've taken the wrong approach to gravity, which can't help.

What you should have is something like this:

__int64 begin, end, frequency;
double elapsedtime = 0;
QueryPerformanceFrequency((LARGE_INTEGER*)&frequency);
while(true) {
    QueryPerformanceCounter((LARGE_INTEGER*)&begin);
    DoMessageLoop(); // grabs user input and moves the block, etc.
    QueryPerformanceCounter((LARGE_INTEGER*)&end);
    elapsedtime += (((double)end - (double)begin)/frequency) * 1000);
    if (elapsedtime > gravitytimeinMS) {
        MoveBlockDown();
        elapsedtime -= gravitytimeinMS;
    }
}

Assuming that your message loop runs at a reasonable framerate (a given on modern hardware), you will have very accurate gravity and there's no threading involved.

Now, that code was pretty Windows specific, and it's not quite perfect, as I've got little experience on other platforms. However, the fundamental concept is the same - get a timer, measure the time of your main loop, move if the time was long enough. There's no need or benefit to introducing threading here at all. Threads should be reserved for when you really, REALLY need large computational loads done on other threads- either because your current one is saturated or because you need it to be responsive to the user. Using them as a timing mechanism is a total waste.

Answer 3

I'd personally just lock from the outside. But that's based on my limited experience - I'm not claiming to be a threading guru, and I'd appreciate any comments from people that know better than me.

I have often found that getting a class to be responsible for its own thread safety is nigh-on impossible in many cases. Even if you get to a state where it appears your class cannot have its invariants violated, you'll run into issues where you want to perform combinations of operations, as you are now finding.

I have found that pushing responsibility for thread safety completely onto consuming classes has resulted in much easier to understand code, and your domain classes will be much easier to design.

By trying to make your classes thread safe by default, you'll be reasoning about situations that may never even arise in practise (although this can often be a good exercise educationally - I've found that by asking myself two questions in my short career, I have improved my coding. One is how am I going to unit test this, the other is what happens if multiple threads get a hold of this).

Your most concurrent operations seem to be a user agent that will move a block, and a timer that will drag it down to the floor. That sounds like two mutex acquires to me. What does your tetris block class look like at the moment? It sounds like it's probably much more complicated than that.

In the interest of doing the simplest thing possible, I'd just expose the mutex and allow your consuming system to lock when it deems necessary.

(By the way, the default MO for .NET developers (including in the BCL) is to make instance members non-thread safe by default, pushing the responsibility onto consuming classes).

Answer 4

Is there a problem with moving isGameOver check to the dropCurrentBlock method?

void Game::dropCurrentBlock()
{
   boost::mutex::scoped_lock lock( getMutex() );
   if ( isGameOver() ) return; // game over

   // implement dropCurrentBlock
}