multithreading - C#: Thread-safe events

Question

Is the implementation below thread-safe? If not what am I missing? Should I have the volatile keywords somewhere? Or a lock somewhere in the OnProcessingCompleted method? If so, where?

public abstract class ProcessBase : IProcess
{
    private readonly object completedEventLock = new object();

    private event EventHandler<ProcessCompletedEventArgs> ProcessCompleted;

    event EventHandler<ProcessCompletedEventArgs> IProcess.ProcessCompleted
    {
        add
        {
            lock (completedEventLock)
                ProcessCompleted += value;
        }
        remove
        {
            lock (completedEventLock)
                ProcessCompleted -= value;
        }
    }

    protected void OnProcessingCompleted(ProcessCompletedEventArgs e)
    {
        EventHandler<ProcessCompletedEventArgs> handler = ProcessCompleted;
        if (handler != null)
            handler(this, e);
    }
}

---

Note: The reason why I have private event and explicit interface stuff, is because it is an abstract base class. And the classes that inherit from it shouldn't do anything with that event directly. Added the class wrapper so that it is more clear =)

Answer 1

You need to lock when you fetch the handler too, otherwise you may not have the latest value:

protected void OnProcessingCompleted(ProcessCompletedEventArgs e)
{
    EventHandler<ProcessCompletedEventArgs> handler;
    lock (completedEventLock) 
    {
        handler = ProcessCompleted;
    }
    if (handler != null)
        handler(this, e);
}

Note that this doesn't prevent a race condition where we've decided we're going to execute a set of handlers and then one handler unsubscribed. It will still be called, because we've fetched the multicast delegate containing it into the handler variable.

There's not a lot you can do about this, other than making the handler itself aware that it shouldn't be called any more.

It's arguably better to just not try to make the events thread-safe - specify that the subscription should only change in the thread which will raise the event.