How to implement the observer pattern safely?

Question

I'm implementing a mechanism similar to the observer design pattern for a multithreaded Tetris game. There is a Game class which contains a collection of EventHandler objects. If a class wants to register itself as a listener to a Game object it must inherit the Game::EventHandler class. On state change events a corresponing method is called on the EventHandler interface of each listener. This is what the code looks like:

class Game
{
public:
    class EventHandler
    {
    public:
        EventHandler();

        virtual ~EventHandler();

        virtual void onGameStateChanged(Game * inGame) = 0;

        virtual void onLinesCleared(Game * inGame, int inLineCount) = 0;

    private:
        EventHandler(const EventHandler&);
        EventHandler& operator=(const EventHandler&);
    };

    static void RegisterEventHandler(ThreadSafe<Game> inGame, EventHandler * inEventHandler);

    static void UnregisterEventHandler(ThreadSafe<Game> inGame, EventHandler * inEventHandler);

    typedef std::set<EventHandler*> EventHandlers;
    EventHandlers mEventHandlers;

private:    
    typedef std::set<Game*> Instances;
    static Instances sInstances;
};


void Game::RegisterEventHandler(ThreadSafe<Game> inGame, EventHandler * inEventHandler)
{
    ScopedReaderAndWriter<Game> rwgame(inGame);
    Game * game(rwgame.get());
    if (sInstances.find(game) == sInstances.end())
    {
        LogWarning("Game::RegisterEventHandler: This game object does not exist!");
        return;
    }

    game->mEventHandlers.insert(inEventHandler);
}


void Game::UnregisterEventHandler(ThreadSafe<Game> inGame, EventHandler * inEventHandler)
{
    ScopedReaderAndWriter<Game> rwgame(inGame);
    Game * game(rwgame.get());
    if (sInstances.find(game) == sInstances.end())
    {
        LogWarning("Game::UnregisterEventHandler: The game object no longer exists!");
        return;
    }

    game->mEventHandlers.erase(inEventHandler);
}

There are two problems that I often experience with this kind of pattern:

1. A listener object wants to unregister itself on an already deleted object resulting in a crash.
2. A event is fired to a listener that no longer exists. This happens most often in multithreaded code. Here's a typical scenario:
   • The game state changes in a worker thread. We want the notification to occur in the main thread.
   • The event is wrapped in a boost::function and sent as a PostMessage to the main thread.
   • A short time later this function object is processed by the main thread while the Game object is already deleted. The result is a crash.

My current workaround is the one that you can see in above code sample. I made the UnregisterEventHandler a static method which checks against a list of instances. This does help, but I find it a somewhat hackish solution.

Does anyone know of a set of guidelines on how to cleanly and safely implement notifier/listener system? Any advice on how to avoid the above pitfalls?

PS: If you need more information in order to answer this question you can find the relevant code online here: Game.h, Game.cpp, SimpleGame.h, SimpleGame.cpp, MainWindow.cpp.

Answer 1

1. The rule of thumb is that delete and new for an object should be near each other. E.g. in constructor and destructor or before and after a call where you use the object. So it's a bad practice to delete an object in another object when the latter one didn't create the former one.

2. I don't understand how you pack the events. It seems that you have to check whether the game is still alive before processing an event. Or you can use shared_ptr in events and other places to be sure that games are deleted last.