Raise event thread safely - best practice [duplicate]

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);
}

Since C# 6.0 you can use monadic Null-conditional operator ?. to check for null and raise events in easy and thread-safe way.

SomethingHappened?.Invoke(this, args);

It’s thread-safe because it evaluates the left-hand side only once, and keeps it in a temporary variable. You can read more here in part titled Null-conditional operators.

Update: Actually Update 2 for Visual Studio 2015 now contains refactoring to simplify delegate invocations that will end up with exactly this type of notation. You can read about it in this announcement.

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);
        }
    }

For .NET 4.5 it's better to use Volatile.Read to assign a temp variable.

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

Update:

It's explained in this article: http://msdn.microsoft.com/en-us/magazine/jj883956.aspx. Also, it was explained in Fourth edition of "CLR via C#".

Main idea is that JIT compiler can optimize your code and remove the local temporary variable. So this code:

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

will be compiled into this:

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

This happens in certain special circumstances, however it can happen.

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.