C# Events and Thread Safety

Question

UPDATE

As of C# 6, the answer to this question is:

SomeEvent?.Invoke(this, e);

---

I frequently hear/read the following advice:

Always make a copy of an event before you check it for null and fire it. This will eliminate a potential problem with threading where the event becomes null at the location right between where you check for null and where you fire the event:

// Copy the event delegate before checking/calling
EventHandler copy = TheEvent;

if (copy != null)
    copy(this, EventArgs.Empty); // Call any handlers on the copied list

Updated: I thought from reading about optimizations that this might also require the event member to be volatile, but Jon Skeet states in his answer that the CLR doesn't optimize away the copy.

But meanwhile, in order for this issue to even occur, another thread must have done something like this:

// Better delist from event - don't want our handler called from now on:
otherObject.TheEvent -= OnTheEvent;
// Good, now we can be certain that OnTheEvent will not run...

The actual sequence might be this mixture:

// Copy the event delegate before checking/calling
EventHandler copy = TheEvent;

// Better delist from event - don't want our handler called from now on:
otherObject.TheEvent -= OnTheEvent;    
// Good, now we can be certain that OnTheEvent will not run...

if (copy != null)
    copy(this, EventArgs.Empty); // Call any handlers on the copied list

The point being that OnTheEvent runs after the author has unsubscribed, and yet they just unsubscribed specifically to avoid that happening. Surely what is really needed is a custom event implementation with appropriate synchronisation in the add and remove accessors. And in addition there is the problem of possible deadlocks if a lock is held while an event is fired.

So is this Cargo Cult Programming? It seems that way - a lot of people must be taking this step to protect their code from multiple threads, when in reality it seems to me that events require much more care than this before they can be used as part of a multi-threaded design. Consequently, people who are not taking that additional care might as well ignore this advice - it simply isn't an issue for single-threaded programs, and in fact, given the absence of volatile in most online example code, the advice may be having no effect at all.

(And isn't it a lot simpler to just assign the empty delegate { } on the member declaration so that you never need to check for null in the first place?)

Updated: In case it wasn't clear, I did grasp the intention of the advice - to avoid a null reference exception under all circumstances. My point is that this particular null reference exception can only occur if another thread is delisting from the event, and the only reason for doing that is to ensure that no further calls will be received via that event, which clearly is NOT achieved by this technique. You'd be concealing a race condition - it would be better to reveal it! That null exception helps to detect an abuse of your component. If you want your component to be protected from abuse, you could follow the example of WPF - store the thread ID in your constructor and then throw an exception if another thread tries to interact directly with your component. Or else implement a truly thread-safe component (not an easy task).

So I contend that merely doing this copy/check idiom is cargo cult programming, adding mess and noise to your code. To actually protect against other threads requires a lot more work.

Update in response to Eric Lippert's blog posts:

So there's a major thing I'd missed about event handlers: "event handlers are required to be robust in the face of being called even after the event has been unsubscribed", and obviously therefore we only need to care about the possibility of the event delegate being null. Is that requirement on event handlers documented anywhere?

And so: "There are other ways to solve this problem; for example, initializing the handler to have an empty action that is never removed. But doing a null check is the standard pattern."

So the one remaining fragment of my question is, why is explicit-null-check the "standard pattern"? The alternative, assigning the empty delegate, requires only = delegate {} to be added to the event declaration, and this eliminates those little piles of stinky ceremony from every place where the event is raised. It would be easy to make sure that the empty delegate is cheap to instantiate. Or am I still missing something?

Surely it must be that (as Jon Skeet suggested) this is just .NET 1.x advice that hasn't died out, as it should have done in 2005?

Answer 1

I see a lot of people going toward the extension method of doing this ...

public static class Extensions   
{   
  public static void Raise<T>(this EventHandler<T> handler, 
    object sender, T args) where T : EventArgs   
  {   
    if (handler != null) handler(sender, args);   
  }   
}

That gives you nicer syntax to raise the event ...

MyEvent.Raise( this, new MyEventArgs() );

And also does away with the local copy since it is captured at method call time.

Answer 2

Thanks for a useful discussion. I was working on this problem recently and made the following class which is a bit slower, but allows to avoid callings to disposed objects.

The main point here is that invocation list can be modified even event is raised.

/// <summary>
/// Thread safe event invoker
/// </summary>
public sealed class ThreadSafeEventInvoker
{
    /// <summary>
    /// Dictionary of delegates
    /// </summary>
    readonly ConcurrentDictionary<Delegate, DelegateHolder> delegates = new ConcurrentDictionary<Delegate, DelegateHolder>();

    /// <summary>
    /// List of delegates to be called, we need it because it is relatevely easy to implement a loop with list
    /// modification inside of it
    /// </summary>
    readonly LinkedList<DelegateHolder> delegatesList = new LinkedList<DelegateHolder>();

    /// <summary>
    /// locker for delegates list
    /// </summary>
    private readonly ReaderWriterLockSlim listLocker = new ReaderWriterLockSlim();

    /// <summary>
    /// Add delegate to list
    /// </summary>
    /// <param name="value"></param>
    public void Add(Delegate value)
    {
        var holder = new DelegateHolder(value);
        if (!delegates.TryAdd(value, holder)) return;

        listLocker.EnterWriteLock();
        delegatesList.AddLast(holder);
        listLocker.ExitWriteLock();
    }

    /// <summary>
    /// Remove delegate from list
    /// </summary>
    /// <param name="value"></param>
    public void Remove(Delegate value)
    {
        DelegateHolder holder;
        if (!delegates.TryRemove(value, out holder)) return;

        Monitor.Enter(holder);
        holder.IsDeleted = true;
        Monitor.Exit(holder);
    }

    /// <summary>
    /// Raise an event
    /// </summary>
    /// <param name="args"></param>
    public void Raise(params object[] args)
    {
        DelegateHolder holder = null;

        try
        {
            // get root element
            listLocker.EnterReadLock();
            var cursor = delegatesList.First;
            listLocker.ExitReadLock();

            while (cursor != null)
            {
                // get its value and a next node
                listLocker.EnterReadLock();
                holder = cursor.Value;
                var next = cursor.Next;
                listLocker.ExitReadLock();

                // lock holder and invoke if it is not removed
                Monitor.Enter(holder);
                if (!holder.IsDeleted)
                    holder.Action.DynamicInvoke(args);
                else if (!holder.IsDeletedFromList)
                {
                    listLocker.EnterWriteLock();
                    delegatesList.Remove(cursor);
                    holder.IsDeletedFromList = true;
                    listLocker.ExitWriteLock();
                }
                Monitor.Exit(holder);

                cursor = next;
            }
        }
        catch
        {
            // clean up
            if (listLocker.IsReadLockHeld)
                listLocker.ExitReadLock();
            if (listLocker.IsWriteLockHeld)
                listLocker.ExitWriteLock();
            if (holder != null && Monitor.IsEntered(holder))
                Monitor.Exit(holder);

            throw;
        }
    }

    /// <summary>
    /// helper class
    /// </summary>
    class DelegateHolder
    {
        /// <summary>
        /// delegate to call
        /// </summary>
        public Delegate Action { get; private set; }

        /// <summary>
        /// flag shows if this delegate removed from list of calls
        /// </summary>
        public bool IsDeleted { get; set; }

        /// <summary>
        /// flag shows if this instance was removed from all lists
        /// </summary>
        public bool IsDeletedFromList { get; set; }

        /// <summary>
        /// Constuctor
        /// </summary>
        /// <param name="d"></param>
        public DelegateHolder(Delegate d)
        {
            Action = d;
        }
    }
}

And the usage is:

    private readonly ThreadSafeEventInvoker someEventWrapper = new ThreadSafeEventInvoker();
    public event Action SomeEvent
    {
        add { someEventWrapper.Add(value); }
        remove { someEventWrapper.Remove(value); }
    }

    public void RaiseSomeEvent()
    {
        someEventWrapper.Raise();
    }

Test

I tested it in the following manner. I have a thread which creates and destroys objects like this:

var objects = Enumerable.Range(0, 1000).Select(x => new Bar(foo)).ToList();
Thread.Sleep(10);
objects.ForEach(x => x.Dispose());

In a Bar (a listener object) constructor I subscribe to SomeEvent (which is implemented as shown above) and unsubscribe in Dispose:

    public Bar(Foo foo)
    {
        this.foo = foo;
        foo.SomeEvent += Handler;
    }

    public void Handler()
    {
        if (disposed)
            Console.WriteLine("Handler is called after object was disposed!");
    }

    public void Dispose()
    {
        foo.SomeEvent -= Handler;
        disposed = true;
    }

Also I have couple of threads which raise event in a loop.

All these actions are performed simultaneously: many listeners are created and destroyed and event is being fired at the same time.

If there were a race conditions I should see a message in a console, but it is empty. But if I use clr events as usual I see it full of warning messages. So, I can conclude that it is possible to implement a thread safe events in c#.

What do you think?

Answer 3

Wire all your events at construction and leave them alone. The design of the Delegate class cannot possibly handle any other usage correctly, as I will explain in the final paragraph of this post.

First of all, there's no point in trying to intercept an event notification when your event handlers must already make a synchronized decision about whether/how to respond to the notification.

Anything that may be notified, should be notified. If your event handlers are properly handling the notifications (i.e. they have access to an authoritative application state and respond only when appropriate), then it will be fine to notify them at any time and trust they will respond properly.

The only time a handler shouldn't be notified that an event has occurred, is if the event in fact hasn't occurred! So if you don't want a handler to be notified, stop generating the events (i.e. disable the control or whatever is responsible for detecting and bringing the event into existence in the first place).

Honestly, I think the Delegate class is unsalvageable. The merger/transition to a MulticastDelegate was a huge mistake, because it effectively changed the (useful) definition of an event from something that happens at a single instant in time, to something that happens over a timespan. Such a change requires a synchronization mechanism that can logically collapse it back into a single instant, but the MulticastDelegate lacks any such mechanism. Synchronization should encompass the entire timespan or instant the event takes place, so that once an application makes the synchronized decision to begin handling an event, it finishes handling it completely (transactionally). With the black box that is the MulticastDelegate/Delegate hybrid class, this is near impossible, so adhere to using a single-subscriber and/or implement your own kind of MulticastDelegate that has a synchronization handle that can be taken out while the handler chain is being used/modified. I'm recommending this, because the alternative would be to implement synchronization/transactional-integrity redundantly in all your handlers, which would be ridiculously/unnecessarily complex.

Answer 4

I don't believe the question is constrained to the c# "event" type. Removing that restriction, why not re-invent the wheel a bit and do something along these lines?

Raise event thread safely - best practice

• 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 delegate

Answer 5

"Why is explicit-null-check the 'standard pattern'?"

I suspect the reason for this might be that the null-check is more performant.

If you always subscribe an empty delegate to your events when they are created, there will be some overheads:

• Cost of constructing the empty delegate.
• Cost of constructing a delegate chain to contain it.
• Cost of invoking the pointless delegate every single time the event is raised.

(Note that UI controls often have a large number of events, most of which are never subscribed to. Having to create a dummy subscriber to each event and then invoke it would likely be a significant performance hit.)

I did some cursory performance testing to see the impact of the subscribe-empty-delegate approach, and here are my results:

Executing 50000000 iterations . . .
OnNonThreadSafeEvent took:      432ms
OnClassicNullCheckedEvent took: 490ms
OnPreInitializedEvent took:     614ms <--
Subscribing an empty delegate to each event . . .
Executing 50000000 iterations . . .
OnNonThreadSafeEvent took:      674ms
OnClassicNullCheckedEvent took: 674ms
OnPreInitializedEvent took:     2041ms <--
Subscribing another empty delegate to each event . . .
Executing 50000000 iterations . . .
OnNonThreadSafeEvent took:      2011ms
OnClassicNullCheckedEvent took: 2061ms
OnPreInitializedEvent took:     2246ms <--
Done

Note that for the case of zero or one subscribers (common for UI controls, where events are plentiful), the event pre-initialised with an empty delegate is notably slower (over 50 million iterations...)

For more information and source code, visit this blog post on .NET Event invocation thread safety that I published just the day before this question was asked (!)

(My test set-up may be flawed so feel free to download the source code and inspect it yourself. Any feedback is much appreciated.)

Answer 6

I truly enjoyed this read - not! Even though I need it to work with the C# feature called events!

Why not fix this in the compiler? I know there are MS people who read these posts, so please don't flame this!

1 - the Null issue) Why not make events be .Empty instead of null in the first place? How many lines of code would be saved for null check or having to stick a = delegate {} onto the declaration? Let the compiler handle the Empty case, IE do nothing! If it all matters to the creator of the event, they can check for .Empty and do whatever they care with it! Otherwise all the null checks / delegate adds are hacks around the problem!

Honestly I'm tired of having to do this with every event - aka boilerplate code!

public event Action<thisClass, string> Some;
protected virtual void DoSomeEvent(string someValue)
{
  var e = Some; // avoid race condition here! 
  if(null != e) // avoid null condition here! 
     e(this, someValue);
}

2 - the race condition issue) I read Eric's blog post, I agree that the H (handler) should handle when it dereferences itself, but cannot the event be made immutable/thread safe? IE, set a lock flag on its creation, so that whenever it is called, it locks all subscribing and un-subscribing to it while its executing?

Conclusion,

Are not modern day languages supposed to solve problems like these for us?