A better way to raise events

Intended audience: beginning and intermediate C# developers - although I've seen this bug in code from developers more experienced than I am.

Events are used a lot in .NET, but almost always I see a subtle bug in the code that raises the event. This bug is present in almost all the code that you'll find on the internet.

Typically, if your class publishes an event you'll have code like this:

// Warning - do not use this code - it contains a subtle bug
public event EventHandler SomethingHappens;

protected void OnSomethingHappens(EventArgs e)
{
     if (SomethingHappens != null) SomethingHappens(this, e);
}

Seems perfect, right? We have a dedicated method for raising the event, and this method first checks if the event is null (that means there are no subscribers). The event will only be raised if there are subscribers.

This is all very well, except if multiple threads are used. Look at a rewrite of the same code:

// Warning - do not use this code - it contains a subtle bug
protected void OnSomethingHappens(EventArgs e)
{
     if (SomethingHappens != null)
     {
         // What if a threadswitch occurs at this point, and the
        // other thread unsubribes from the event?
        // --> The event will become null and we get
        // a NullReferenceException
        SomethingHappens(this, e);
     }
}

This code is exactly the same as the first, but now you can see that there is actually a subtle bug present. The chances of it occuring are rather small, because several conditions have to be met:

• there must be a threadswitch between the check if the event is null, and the actual raising of the event (on a single-processor system the chance for this is small).
• the new thread that is scheduled unsubscribes the last subscriber for the event, so it becomes null.

Small as the chances are, I've seen this occur in the wild. And remember, the smaller the chance that a bug occurs, the more difficult it is to diagnose and solve it (this is the sort of bug that will occur when you give a presentation to your boss's boss).

So how can we fix this? The first solution that springs into mind is to protect this code using a lock(this) statement. However, there are several reasons why I don't like this (not only performance - but that deserves its own post).

This is (according to me) the best pattern to raise events in C#:

// This code is safe to use
public event EventHandler SomethingHappens;

protected void OnSomethingHappens(EventArgs e)
{
    EventHandler copy = SomethingHappens;
    // if at this point another thread unsubscribes,
    // our copy remains unaffected --> no exceptions
    if (copy != null) copy(this, e);
}

As you can see, we first take a copy of the event before we raise it. If another thread decides to unsubscribe (and possible make the event null), our copy is unaffected and it is still safe to use it. We don't have to use a lock-statement, because assigning the content of the event to the copy-variable is an atomic operation. I like this pattern because it is always safe to use and has absolutely no performance penalty.

P.S. Most of the time when you see events used in code, they are used in GUI code. In that case there can be no threading-problems because all GUI code is executed on a single thread. However, given that the pattern I propose has no disadvantages I always use it - even when not strictly necessary. You never know when your code is reused and suddenly becomes vulnerable to threading-issues that you didn't anticipate when you were writing the code.

Kristof

[edit] After writing this article, I found out that the same issue is also raised here. Just want to give credit where it's due...

[update] From .NET 2 onwards, there is an even simpler pattern to solve this problem. You can read more about it here.