Self de-/registering Observer over multiple threads

Question

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

I would highly suggest NOT rolling your own observer/notification library. I would highly suggest checking out Boost::signals2 — Mgetz
@Mgetz I'm using Poco::NotificationCenter in combination with Poco::Observer, Poco::NotificationQueue, etc. This problem arised while implementing my structure with those classes, I just created a simple example using plain c++, but the situation is the same. I will have a look at boost::signals2 however; maybe the design prevents problems like that. — MatthiasB
How much control do you have on the rest of the code? Can you use shared pointers for classes that derive from Observer? — Yochai Timmer
@YochaiTimmer Subclasses of this will be used by other developers, which do not have experience with shared pointers. If there is no other way then I could live with it, but I would prefer it to work with RAII on local objects as well. — MatthiasB

mariusm mariusm · Accepted Answer · 2014-09-04T15:12:21

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.

Self de-/registering Observer over multiple threads

1 Answers