Race condition when using the observer pattern with a GUI

Question

I'm new to C# & threading and I recently started working on a utility that's using multiple threads. I have some event handling logic being done by one thread, and then a GUI on a separate thread that is observing the event handler and receiving notifications when new events are received.

When the GUI is manually closed by the user I detach it so that it's no longer observing the event handler. However, the next time the event handler receives an event it thinks that it still has something in it's list of observers. I added some print outs/breakpoints and it seems to go into NotifyObservers and hits the foreach loop, then goes into the detach method, empties the observer list, and then when it goes back into NotifyObservers the observer it tries to access has already been disposed and it gets an exception.

I saw on this page that you're supposed to use locks to prevent race conditions from occurring and I tried using one on the observers list before the foreach in NotifyObservers and it still gets the exception. I'm thinking it might have something to do with locking not being able to prevent the GUI from closing on the other thread, so the other thread does not wait when I try to lock, but I'm new to this so I'm not really sure. I tried throwing a bunch of other locks around in these methods as well and nothing seemed to have any effect.

I've included the code for the 3 methods involved below, Detach and NotifyObservers are in my event handler, and HandleClosing is in my observer

    protected void HandleClosing(object sender, EventArgs e)
    {
        handler.Detach(this);
    }

    public void Detach(SubscriberObserver observer)
    {
        observers.Remove(observer);
    }

    public void NotifyObservers()
    {
        foreach (SubscriberObserver observer in observers)
        {
            observer.Invoke(new Action(() => { observer.Notify(); }));
        }
    }

Answer 1

I don't know what type your observer collection has, but I assume it's kind of a thread-safe collection, which might behave the following way when iterating over it via the foreach loop. It locks itself, then creates an IEnumerable copy of itself, and then unlocks itself. The iteration then starts over the elements of the copy. If you remove an element from the collection after the copy has been created, it doesn't matter, the loop will still encounter the removed element.

To fix your race condition you need to lock the collection for the whole iteration and also you should perform the removal inside a lock on the same object. You can create a lock object for this sole purpose or you can lock on ICollection.SyncRoot if your collection implements that.

If observer is Control == true you might be encountering a deadlock when calling Invoke. Try calling BeginInvoke instead. A quote from MSDN: "The difference between the two methods is that a call to Invoke is a blocking call while a call to BeginInvoke is not. In most cases it is more efficient to call BeginInvoke because the secondary thread can continue to execute without having to wait for the primary UI thread to complete its work updating the user interface."