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

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 declarationthreadsafe_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 declarationthreadsafe_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.

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.

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).