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?
I gave it 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. Any class that needs to be able to modify the game state can store a a pointer to the TreadSafeGame object so that it can access the mutex in order to create an atomic scope.
My question is (to anyone with a good understanding of multithreaded programming):
- Are there any errors in my reasoning throughout this post?
- Am I approaching a good design now?
- Or am I overengineering?
- Are there any interesting alternative approaches that you would recommend?
Thanks.
Update
Thanks to @FuleSnabel's suggestion, I eliminated the drawback that the user must do the locking from the outside. ThreadSafeGame is now still the class that holds the game. But to access the game object you must make a WritableGame or a ReadOnlyGame object. These objects keep the Game object locked during their lifetime. Access to the actual Game object is enabled through "operator->". Here's the code:
class Game;
struct GameAndMutex
{
GameAndMutex(std::auto_ptr<Game> inGame) : mGame(inGame.release()) { }
boost::scoped_ptr<Game> mGame;
mutable boost::mutex mMutex;
};
class ThreadSafeGame
{
public:
ThreadSafeGame(std::auto_ptr<Game> inGame) :
mGameAndMutex(new GameAndMutex(inGame))
{
}
private:
friend class WritableGame;
friend class ReadOnlyGame;
boost::shared_ptr<GameAndMutex> mGameAndMutex;
};
class WritableGame
{
public:
WritableGame(ThreadSafeGame & inThreadSafeGame) :
mLock(inThreadSafeGame.mGameAndMutex->mMutex),
mGame(inThreadSafeGame.mGameAndMutex->mGame.get())
{
}
Game * operator->() { return mGame; }
private:
boost::mutex::scoped_lock mLock;
Game * mGame;
};
class ReadOnlyGame
{
public:
ReadOnlyGame(const ThreadSafeGame & inThreadSafeGame) :
mLock(inThreadSafeGame.mGameAndMutex->mMutex),
mGame(inThreadSafeGame.mGameAndMutex->mGame.get())
{
}
const Game * operator->() const { return mGame; }
private:
boost::mutex::scoped_lock mLock;
const Game * mGame;
};
Some sample code:
void test()
{
ThreadSafeGame threadSafeGame(std::auto_ptr<Game>(new Game));
// Enable the gravity timer
TimedGame timedGame(threadSafeGame);
timedGame.start();
// Rotate repeatedly while the block is falling.
while (!ReadOnlyGame(threadSafeGame)->isGameOver())
{
WritableGame game(threadSafeGame);
Block & block = game->activeBlock();
block.rotate();
Poco::Thread::sleep(500);
}
}