Checking for null before event dispatching... thread safe?

Question

Something that confuses me, but has never caused any problems... the recommended way to dispatch an event is as follows:

public event EventHandler SomeEvent;
...
{
    ....
    if(SomeEvent!=null)SomeEvent();
}

In a multi-threaded environment, how does this code guarantee that another thread will not alter the invocation list of SomeEvent between the check for null and the invocation of the event?

Answer 1

The recommended way is a little different and uses a temporary as follows:

EventHandler tmpEvent = SomeEvent;
if (tmpEvent != null)
{
    tmpEvent();
}

Answer 2

As you point out, where multiple threads can access SomeEvent simultaneously, one thread could check whether SomeEvent is null and determine that it isn't. Just after doing so, another thread could remove the last registered delegate from SomeEvent. When the first thread attempts to raise SomeEvent, an exception will be thrown. A reasonable way to avoid this scenario is:

void SomeEventInvoke(object sender, EventArgs args) 
{
    EventHandler ev = SomeEvent;
    if (ev != null) ev(sender, args);
}

This works because whenever a delegate is added to or removed from an event using the default implementations of the add and remove accessors, the Delegate.Combine and Delegate.Remove static methods are used. Each of these methods returns a new instance of a delegate, rather than modifying the one passed to it.

In addition, assignments of object references in .NET are thread-safe, and the default implementations of the add and remove event accessors are synchronised. So the code above succeeds by first copying the multicast delegate from the event to a temporary variable. Any changes to SomeEvent after this point will not affect the copy you've made and stored. Thus you can now safely test whether any delegates were registered and subsequently invoke them.

UPDATE: Note that this solution solves one race problem, namely that of an event handler being null when it's invoked. It doesn't handle the problem where an event handler is defunct when it's invoked. For example, if the event handler depends on state that's destroyed as soon as the handler is un-subscribed, then this solution might invoke code that cannot run properly. See Eric Lippert's excellent blog entry for more details. Also, see this StackOverflow question and answers.

Answer 3

Safer approach:

public class Test
{
    private EventHandler myEvent;
    private object eventLock = new object();

    private void OnMyEvent()
    {
        EventHandler handler;

        lock(this.eventLock)
        {
            handler = this.myEvent;
        }
        if (handler != null)
        {
            handler(this, EventArgs.Empty);
        }
    }

    public event MyEvent
    {
        add
        {
            lock(this.eventLock)
            {
                this.myEvent += value;
            }
        }
        remove
        {
            lock(this.eventLock)
            {
                this.myEvent -= value;
            }
        }

    }
}

-bill

Answer 4

The simplest way remove this null check is to assign the eventhandler to an anonymous delegate. The penalty incurred in very little and relieves you of all the null checks, race conditions etc.

public event EventHandler SomeEvent = delegate {};

Related question: http://stackoverflow.com/questions/170907/is-there-a-downside-to-adding-an-anonymous-empty-delegate-on-event-declaration