Problems implementing the "Observer" pattern

Question

I have met an interesting problem while implementing the Observer pattern with C++ and STL. Consider this classic example:

class Observer {
public:
   virtual void notify() = 0;
};

class Subject {
public:
   void addObserver( Observer* );
   void remObserver( Observer* );
private:
   void notifyAll();
};

void Subject::notifyAll() {
   for (all registered observers) { observer->notify(); }
}

This example can be found in every book on design patterns. Unfortunately, real-life systems are more complex, so here is the first problem: some observers decide to add other observers to the Subject on being notified. This invalidates the "for" loop and all the iterators, that I use. The solution is rather easy - I make a snapshot of the registered observers list and iterate over the snapshot. Adding new observers does not invalidate the snapshot, so everything seems ok. But here comes another problem: observers decide to destroy themselves on being notified. Even worse, one single observer can decide to destroy all other observers (they are controlled from the scripts), and that invalidates both the queue and a snapshot. I find myself iterating over de-allocated pointers.

My question is how should I handle the situations, when observers kill each other? Are there any ready-to-use patterns? I always thought that "Observer" is the easiest design pattern in the world, but now it seems it is not that easy to implement it correctly...

Thank you, everyone for your interest. Let us have a decisions summary:

[1] "Don't do it" Sorry, but it is a must. Observers are controlled from the scripts and are garbage-collected. I cannot control the garbage collection to prevent their de-allocation;

[2] "Use boost::signal" The most promising decision, but I cannot introduce boost on the project, such decisions must be made by the project leader only (we are writing under Playstation);

[3] "Use shared__ptr" That will prevent observers from de-allocation. Some sub-systems may rely on memory pool cleanup, so I don't think I can use shared_ptr.

[4] "Postpone observer deallocation" Queue observers for removal while notifying, then use the second cycle to delete them. Unfortunately, I cannot prevent the deallocation, so I use a trick of wrapping observer with some kind of "adaptor", keeping actually the list of "adaptors". On destructor, observers unassign from their adaptors, then I take my second cycle to destroy empty adaptors.

p.s. is it ok, that I edit my question to summarize all the post? I am noob on StackOverflow...

Answer 1

Very interesting issue.

Try this:

1. Change remObserver to null out the entry, rather than just removing it (and invalidating the list iterators).

2. Change your notifyAll loop to be:

   for (all registered observers) { if (observer) observer->notify(); }

3. Add another loop at the end of notifyAll to remove all null entries from your observer list

Answer 2

Personally, I use boost::signals to implement my observers; I'll have to check, but I believe it handles the above scenarios (edited: found it, see "When can disconnections occur"). It simplifies your implementation, and it doesn't rely on creating custom class:

class Subject {
public:
   boost::signals::connection addObserver( const boost::function<void ()>& func )
   { return sig.connect(func); }

private:
   boost::signal<void ()> sig;

   void notifyAll() { sig(); }
};

void some_func() { /* impl */ }

int main() {
   Subject foo;
   boost::signals::connection c = foo.addObserver(boost::bind(&some_func));

   c.disconnect(); // remove yourself.
}

Answer 3

A man goes to the doctor and says, "Doc when I raise my arm like this it hurts real bad!" The doctor says, "Don't do that."

The simplest solution is to work with your team and tell them to not do that. If observers "really need" to kill themselves, or all observers, then schedule the action for when the notification is over. Or, better yet, change the remObserver function to know if there's a notify process happening and just queue up the removals for when everything is done.

Answer 4

Here's a variation on the idea T.E.D. already presented.

As long as remObserver can null an entry instead of immediately removing it, then you could implement notifyAll as:

void Subject::notifyAll()
{
    list<Observer*>::iterator i = m_Observers.begin();
    while(i != m_Observers.end())
    {
        Observer* observer = *i;
        if(observer)
        {
            observer->notify();
            ++i;
        }
        else
        {
            i = m_Observers.erase(i);
        }
    }
}

This avoids the need for a second clean-up loop. However it does mean that if some particular notify() call triggers the removal of itself or an observer located earlier in the list, then the actual removal of the list element will be deferred until the next notifyAll(). But as long as any functions which operate over the list take proper care to check for null entries when appropriate, then this shouldn't be a problem.

Answer 5

The problem is that of ownership. You could use smart pointers, for instance the boost::shared_ptr and boost::weak_ptr classes, to extend the lifetime of your observers past the point of "de-allocation".

Answer 6

There are several solutions for this problem:

1. Use boost::signal it allows automatic connection removal when the object destroyed. But you should be very careful with thread safety
2. Use boost::weak_ptr or tr1::weak_ptr for managment of observers, and boost::shared_ptr or tr1::shared_ptr for observers them self -- reference counting would help you for invalidating objects, weak_ptr would let you know if object exists.

3. If you are running over some event loop, make sure, that each observer does not destroy itself, add itself or any other in the same call. Just postpone the job, meaning

   SomeObserver::notify()
   {
      main_loop.post(boost::bind(&SomeObserver::someMember,this));
   }
   

Answer 7

How about using a linked list in your for loop ?

Answer 8

If your program is multi-threaded you may need to use some locking here.

Anyway, from your description it seems that the issue is not concurrency (multi-thrading) but rather mutations induced by the Observer::notify() call. If this is the case then you can solve the problem by using a vector and traversing it via an index rather than an iterator.

for(int i = 0; i < observers.size(); ++i)
  observers[i]->notify();

Answer 9

How about having a member iterator called current (initialised to be the end iterator). Then

void remObserver(Observer* obs)
{
    list<Observer*>::iterator i = observers.find(obs);
    if (i == current) { ++current; }
    observers.erase(i);
}

void notifyAll()
{
    current = observers.begin();
    while (current != observers.end())
    {
        // it's important that current is incremented before notify is called
        Observer* obs = *current++;
        obs->notify(); 
    }
}

Answer 10

Define and use a heavy duty iterator over the container of notifiers that is resilient to deletion (eg, nulling out, as previously mentioned) and can handle addition (eg appending)

On the other hand if you want to enforce keeping the container const during notification, declare notifyAll and the container being iterated on as const.

Answer 11

I was searching for a solution to this problem when I came across this article a few months back. It got me to thinking about the solution and I think I have one that doesn't rely on boost, smart pointers, etc.

In short, here is the sketch of the solution:

1. The Observer is a singleton with keys for Subjects to register interest in. Because it is a singleton, it always exists.
2. Each subject is derived from a common base class. The base class has an abstract virtual function Notify(...) which must be implemented in derived classes, and a destructor that removes it from the Observer (which it can always reach) when it is deleted.
3. Inside the Observer itself, if Detach(...) is called while a Notify(...) is in progress, any detached Subjects end up on a list.
4. When Notify(...) is called on the Observer, it creates a temporary copy of the Subject list. As it iterates over it, it compare it to the recently detached. If the target is not on it, Notify(...) is called on the target. Otherwise, it is skipped.
5. Notify(...) in the Observer also keeps track of the depth to handle cascading calls (A notifies B, C, D, and the D.Notify(...) triggers a Notify(...) call to E, etc.)

This seems to work well. The solution is posted on the web here along with the source code. This is a relatively new design, so any feedback is greatly appreciated.

Answer 12

I just wrote a complete observer class. I'll include it after it's been tested.

But my answer to your question is: handle the case!

My version does allow notify loops to be triggered inside of notify loops (they run immediately, think of this as depth first recursion), but there is a counter so that the Observable class knows that a notify is running and how many deep.

If an observer gets deleted, its destructor tells all observables that it's subscribed to about the destruction. If they're not in a notify loop that the observer is in, then that observable is deleted from a std::list<pair<Observer*, int>> for that event if it IS in a loop, then its entry in the list is invalidated and a command is pushed into a queue that will be run when the notify counter is down to zero. That command will delete the invalidated entry.

So basically, if you can't safely delete (because there might be an iterator holding the entry that will notify you), then you invalidate the entry instead of deleting it.

So like all concurrent no-wait systems the rule is - handle the case if you're not locked out, but if you are then you queue the work and whoever holds the lock will do the work when he releases the lock.

Answer 13

This is a bit slower since you are copying the collection, but I think it's simpler too.

class Subject {
public:
   void addObserver(Observer*);
   void remObserver(Observer*);
private:
   void notifyAll();
   std::set<Observer*> observers;
};

void Subject::addObserver(Observer* o) {
  observers.insert(o);
}

void Subject::remObserver(Observer* o) {
  observers.erase(o);
}

void Subject::notifyAll() {
  std::set<Observer*> copy(observers);
  std::set<Observer*>::iterator it = copy.begin();
  while (it != copy.end()) {
    if (observers.find(*it) != observers.end())
      (*it)->notify();
    ++it;
  }
}

Answer 14

You can never avoid observers being removed while iterating.

The observer can even be removed WHILE you are trying to call its notify() function.

Therefore I suppose you need a try/catch mechanism.

The lock is to ensure observerset is not changedd while copying the set of observers

  lock(observers)
  set<Observer> os = observers.copy();
  unlock(observers)
  for (Observer o: os) {
    try { o.notify() }
    catch (Exception e) {
      print "notification of "+o+"failed:"+e
    }
  }