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

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 2

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.

Answer 3

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

Answer 4

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.

Answer 5

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..

Update 2021-09-01

Today I would simply do (which do not require the emty delegate):

SomethingHappened?.Invoke(e);

Someone pointed out that using an empty delegate has a larger overhead. That's true. But from an application perspetive, the performance impact is minimal. Therefore, it's much more important to choose the solution that has the cleanest code.

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

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 8

I tried to pimp out Jesse C. Slicer's answer with:

• Ability to sub/unsubscribe from any thread while within a raise (race condition removed)
• Operator overloads for += and -= at the class level

• Generic caller defined delegates

  public class ThreadSafeEventDispatcher<T> where T : class
  {
      readonly object _lock = new object();
  
      private class RemovableDelegate
      {
          public readonly T Delegate;
          public bool RemovedDuringRaise;
  
          public RemovableDelegate(T @delegate)
          {
              Delegate = @delegate;
          }
      };
  
      List<RemovableDelegate> _delegates = new List<RemovableDelegate>();
  
      Int32 _raisers;  // indicate whether the event is being raised
  
      // Raises the Event
      public void Raise(Func<T, bool> raiser)
      {
          try
          {
              List<RemovableDelegate> raisingDelegates;
              lock (_lock)
              {
                  raisingDelegates = new List<RemovableDelegate>(_delegates);
                  _raisers++;
              }
  
              foreach (RemovableDelegate d in raisingDelegates)
              {
                  lock (_lock)
                      if (d.RemovedDuringRaise)
                          continue;
  
                  raiser(d.Delegate);  // Could use return value here to stop.                    
              }
          }
          finally
          {
              lock (_lock)
                  _raisers--;
          }
      }
  
      // Override + so that += works like events.
      // Adds are not recognized for any event currently being raised.
      //
      public static ThreadSafeEventDispatcher<T> operator +(ThreadSafeEventDispatcher<T> tsd, T @delegate)
      {
          lock (tsd._lock)
              if (!tsd._delegates.Any(d => d.Delegate == @delegate))
                  tsd._delegates.Add(new RemovableDelegate(@delegate));
          return tsd;
      }
  
      // Override - so that -= works like events.  
      // Removes are recongized immediately, even for any event current being raised.
      //
      public static ThreadSafeEventDispatcher<T> operator -(ThreadSafeEventDispatcher<T> tsd, T @delegate)
      {
          lock (tsd._lock)
          {
              int index = tsd._delegates
                  .FindIndex(h => h.Delegate == @delegate);
  
              if (index >= 0)
              {
                  if (tsd._raisers > 0)
                      tsd._delegates[index].RemovedDuringRaise = true; // let raiser know its gone
  
                  tsd._delegates.RemoveAt(index); // okay to remove, raiser has a list copy
              }
          }
  
          return tsd;
      }
  }
  

Usage:

    class SomeClass
    {   
        // Define an event including signature
        public ThreadSafeEventDispatcher<Func<SomeClass, bool>> OnSomeEvent = 
                new ThreadSafeEventDispatcher<Func<SomeClass, bool>>();

        void SomeMethod() 
        {
            OnSomeEvent += HandleEvent; // subscribe

            OnSomeEvent.Raise(e => e(this)); // raise
        }

        public bool HandleEvent(SomeClass someClass) 
        { 
            return true; 
        }           
    }

Any major problems with this approach?

The code was only briefly tested and edited a bit on insert.
Pre-acknowledge that List<> not a great choice if many elements.

Answer 9

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 10

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