Observer Design pattern - Getting a segfault

Question

I am writing an Observer design pattern where Subject knows what Observer it has (same as the original design) and Observer also knows what Subject it's attached to, mainly to tackle scenarios like Observer going out of scope and Subject has a reference to a destroyed object.

So if an Observer watches a Subject, both know about each other. This is handled via observer->removeSubject and observer->addSubject.

In the following snippet, I am testing a case where Observers go out of scope, meaning ~Observer() gets invoked first however I seem to be getting a segfault on the following line when I debug in an online gdb tool in the second iteration of ~Observer()

std::cout << "Observers size = " << _observers.size() << "\n";

template<typename T>
class Subject;

template<typename T>
class Observer
{
    std::set<Subject<T>*> _subjects;

    public:
    virtual void update(T val) = 0;

    void addSubject(Subject<T>* subject)
    {
        printf ("[Observer] Adding Subject to observer - ");
        _subjects.insert(subject);
        std::cout << "Subject size = " << _subjects.size() << "\n\n";
    }

    void removeSubject(Subject<T>* subject)
    {
        printf ("[Observer] Removing Subject from observer - ");
        _subjects.erase(subject);
        std::cout << "Subject size = " << _subjects.size() << "\n";
    }

    virtual ~Observer()
    {
        printf ("\n~Observer\nSubject Size = %ld\n", _subjects.size());
        for (auto& subject : _subjects)
        {
            subject->detach(this); 
        }
    }
};

template<typename T>
class Subject
{
    std::set<Observer<T>*> _observers;

    protected:
    Subject() = default;

    public:
    void attach(Observer<T>* observer)
    {
        printf ("~~ [Subject] Attaching observer ~~\n");
        _observers.insert(observer);
        observer->addSubject(this);
    }
    
    void detach(Observer<T>* observer)
    {
        printf ("~~ [Subject] Detaching observer ~~\n");
        std::cout << "Observers size = " << _observers.size() << "\n";
        _observers.erase(observer);
        observer->removeSubject(this);
    }

    void notify(const T& val)
    {
        for (auto& observer : _observers)
        {
            observer->update(val);
        }
    }
    
    virtual ~Subject()
    {
        printf ("\n~Subject\n");
        printf ("Observer size = %ld\n", _observers.size());
        for (auto& obs : _observers)
        {
            obs->removeSubject(this); 
        }
    }
};

template<typename T>
class ConcreteSubject : public Subject<T>
{
    T _value;

    public:
    void set(const T& value)
    {
        _value = value;
        this->notify(value);
    }
};

template<typename T>
class ConcreteObserver : public Observer<T>
{
    public:
    void update(T value)
    {
        std::cout << "Observer Notified: " << value << "\n";
    }
};

int main() 
{
    ConcreteSubject<int> sub;
    {
        ConcreteObserver<int> obs;
        ConcreteObserver<int> obs1;
        sub.attach(&obs);
        sub.attach(&obs1);
        sub.set(5);
    }   
}

One part that I am concerned about is the following where subject is removed from _subjects while _subjects is being iterated over. Should a copy of _subjects be made to be iterated over in ~Observer() instead?

    void removeSubject(Subject<T>* subject)
    {
        printf ("[Observer] Removing Subject from observer - ");
        _subjects.erase(subject);
        std::cout << "Subject size = " << _subjects.size() << "\n";
    }

    virtual ~Observer()
    {
        printf ("\n~Observer\nSubject Size = %ld\n", _subjects.size());
        for (auto& subject : _subjects)
        {
            subject->detach(this); 
        }
     }

Answer 1

There may be more problems in your code but I can see at last this one:

When Observer is being destroyed in destructor, it iterates over _subjects collection. You call Subject::detach() on each subject. Subject::detach() in turn calls Observer::removeSubject() on the very same observer and Observer::removeSubject() removes the subject from the very same _subjects collection which you are currently iterating upon. This is undefined behaviour which later probably leads to the segfault you are having.

Solution 1

In Observer::~Observer() you can make a copy of the _subjects collection and iterate on this copy. This seems to be the simplest solution.

Solution 2

virtual ~Observer()
{
  while (!_subjects.empty())
  {
    (*_subjects.begin())->detach(this); 
  }
}

Solution 3

This solution may actually be the most performant one. If you are destroying the Observer you do not need to clear the _subjects collection one by one because the observer object will be destroyed anyway. Therefore you may introduce a private bool _destroying member variable. Its default value will be false but in the destructor it would be set to true. And when true, it would prevent removing from _subjects collection in removeSubject().

void removeSubject(Subject<T>* subject)
{
  if (!_destroying)
  {
    _subjects.erase(subject);
  }
}

virtual ~Observer()
{
  _destroying = true;
  for (auto& subject : _subjects)
  {
    subject->detach(this); 
  }
}