Raise event thread safely - best practice

Question

In order to raise an event we use a method OnEventName like this:

protected virtual void OnSomethingHappened(EventArgs e) 
{
    EventHandler handler = SomethingHappened;
    if (handler != null) 
    {
        handler(this, e);
    }
}

But what is the difference with this one ?

protected virtual void OnSomethingHappened(EventArgs e) 
{
    if (SomethingHappened!= null) 
    {
        SomethingHappened(this, e);
    }
}

Apparently the first is thread-safe, but why and how ?

It's not necessary to start a new thread ?

Answer 1

Declare your event like this to get thread safety:

public event EventHandler<MyEventArgs> SomethingHappened = delegate{};

And invoke it like this:

protected virtual void OnSomethingHappened(MyEventArgs e)   
{  
    SomethingHappened(this, e);
} 

Although the method is not needed anymore..

Answer 2

Actually, no, the second example isn't considered thread-safe. The SomethingHappened event could evaluate to non-null in the conditional, then be null when invoked. It's a classic race condition.

Answer 3

There is a tiny chance that SomethingHappened becomes null after the null check but before the invocation. However, MulticastDelagates are immutable, so if you first assign a variable, null check against the variable and invoke through it, you are safe from that scenario (self plug: I wrote a blog post about this a while ago).

There is a back side of the coin though; if you use the temp variable approach, your code is protected against NullReferenceExceptions, but it could be that the event will invoke event listeners after they have been detached from the event. That is just something to deal with in the most graceful way possible.

In order to get around this I have an extension method that I sometimes use:

public static class EventHandlerExtensions
{
    public static void SafeInvoke<T>(this EventHandler<T> evt, object sender, T e) where T : EventArgs
    {
        if (evt != null)
        {
            evt(sender, e);
        }
    }
}

Using that method, you can invoke the events like this:

protected void OnSomeEvent(EventArgs e)
{
    SomeEvent.SafeInvoke(this, e);
}

Answer 4

Actually, the first is thread-safe, but the second isn't. The problem with the second is that the SomethingHappened delegate could be changed to null between the null verification and the invocation. For a more complete explanation, see http://blogs.msdn.com/b/ericlippert/archive/2009/04/29/events-and-races.aspx.

Answer 5

For either of these to be thread safe, you are assuming that all the objects that subscribe to the event are also thread safe.

Answer 6

It depends on what you mean by thread-safe. If your definition only includes the prevention of the NullReferenceException then the first example is more safe. However, if you go with a more strict definition in which the event handlers must be invoked if they exist then neither is safe. The reason has to do with the complexities of the memory model and barriers. It could be that there are, in fact, event handlers chained to the delegate, but the thread always reads the reference as null. The correct way of fixing both is to create an explicit memory barrier at the point the delegate reference is captured into a local variable. There are several ways of doing this.

• Use the lock keyword (or any synchronization mechanism).
• Use the volatile keyword on the event variable.
• Use Thread.MemoryBarrier.

Despite the awkward scoping problem which prevents you from doing the one-line initializer I still prefer the lock method.

protected virtual void OnSomethingHappened(EventArgs e)           
{          
    EventHandler handler;
    lock (this)
    {
      handler = SomethingHappened;
    }
    if (handler != null)           
    {          
        handler(this, e);          
    }          
}          

It is important to note that in this specific case the memory barrier problem is probably moot because it is unlikely that reads of variables will be lifted outside method calls. But, there is no guarentee especially if the compiler decides to inline the method.

Answer 7

I keep this snippet around as a reference for safe multithreaded event access for both setting and firing:

    /// <summary>
    /// Lock for SomeEvent delegate access.
    /// </summary>
    private readonly object someEventLock = new object();

    /// <summary>
    /// Delegate variable backing the SomeEvent event.
    /// </summary>
    private EventHandler<EventArgs> someEvent;

    /// <summary>
    /// Description for the event.
    /// </summary>
    public event EventHandler<EventArgs> SomeEvent
    {
        add
        {
            lock (this.someEventLock)
            {
                this.someEvent += value;
            }
        }

        remove
        {
            lock (this.someEventLock)
            {
                this.someEvent -= value;
            }
        }
    }

    /// <summary>
    /// Raises the OnSomeEvent event.
    /// </summary>
    public void RaiseEvent()
    {
        this.OnSomeEvent(EventArgs.Empty);
    }

    /// <summary>
    /// Raises the SomeEvent event.
    /// </summary>
    /// <param name="e">The event arguments.</param>
    protected virtual void OnSomeEvent(EventArgs e)
    {
        EventHandler<EventArgs> handler;

        lock (this.someEventLock)
        {
            handler = this.someEvent;
        }

        if (handler != null)
        {
            handler(this, e);
        }
    }