Search code examples
c++multithreadingobservers

Self de-/registering Observer over multiple threads


I have a problem with correctly locking a notification center with self-registering observers. Specifically I run into already destroyed objects within event dispatching.

Consider the following structure:

  • A Notification center is broadcasting events to multiple observers
  • Observers register/derigister themselves on on creation/deletion.
    • this can happen at any time
  • Event dispatch may be on a different thread than observer registration.
  • To synchronize Observer registration and dispatch, both functions are locked.

In the end it should be something like this minimal example:

#include <future>
#include <functional>
#include <chrono>
#include <thread>
#include <iostream>
#include <mutex>
#include <map>

class Event {};
class Observer;
class Center {
public:
  void addObserver(Observer &o);
  void removeObserver(Observer &o);

  void sendEvent(Event*);
  static Center& instance() {static Center center; return center;}
private:
  std::map<Observer*, std::function<void(Event*)>> m_observers;
  std::mutex m_mutex;
};
class Observer {
public:
  virtual void onEvent(Event*) = 0;
  void call(Event * event) {onEvent(event);}
  Observer() {Center::instance().addObserver(*this);}
  virtual ~Observer() {Center::instance().removeObserver(*this);}
};
void Center::addObserver(Observer &o) {
  std::lock_guard<std::mutex> l(m_mutex);
  m_observers[&o] = std::bind(&Observer::call,&o,std::placeholders::_1);
}
void Center::removeObserver(Observer &o) {
  std::lock_guard<std::mutex> l(m_mutex);
  m_observers.erase(&o);
}
void Center::sendEvent(Event* e) {
  std::lock_guard<std::mutex> l(m_mutex);
  for(auto i : m_observers) 
    i.second(e);
}
class testObs : public Observer {
  virtual void onEvent(Event*) {std::cout << "do work" << std::endl;}
};

int main()
{
  auto handle = std::async(std::launch::async, []() {
    Event e;
    while(true) {Center::instance().sendEvent(&e);}
  });
  for(;;){testObs t1,t2,t3,t4;}
  handle.get();
}

This will run into a runtime error (R6025 -pure virtual function call) after some time. The problem is that Observer::~Observer() is called only after testObs was destroyed, so the lock is too late and a running the event dispatch will call a destroyed object.

My question is if I can still create a reliable lock (or implement another way of removing the object cleanly) while keeping most of the existing structure:

  • Automatic Observer registration/deregistration
  • No Observer-ownership within the notification center

Solution

  • You have a race between notification and constructor of Observer for the virtual method onEvent:

    the testObs is not yet fully constructed (the virtual method initialized), but it is already registered in the Center by the Observer constructor.

    To debug this try adding printing statements in your constructors.

    To fix it: register your testObs in the constructor of testObs rather than Observer's constructor.

    There is also the same problem with destructor: move the deregistration to destructor of testObs.

    Alternatively provide an empty implementation of onEvent:

    virtual void onEvent(Event*){ };
    

    But at the end of the day, it is a bad design to expose object instance before it is fully constructed.