Saturday, August 19, 2006

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.

5 comments:

Anonymous said...

You may also note that there is a code snipped for this (at least in VS2008)
just type "invoke" and press TAB TAB

Anonymous said...

Why not locking the event?
The pattern you and (as my college anonymous noted) Microsoft are suggesting exposes a potential bug:
In the case a context switch occurred after you've made a copy of the event handler, and another thread removes an observer from the list. This observer is now not expecting to receive event calls of this event and might have moved to a different operation state. Your pattern will cause undesirable behavior by sending him the event and this might cause unexpected results.

Anonymous said...

many will suggest this is bad design, but to avoid both potential issues, as anon #2 suggest, wrap the event invoke in a try/catch.

you don't have to make a copy, and you don't have to worry that the consumer is in the wrong state. (the consumer would invariably have been the one to unsubscribe from the event if it is now null)

James said...

try/catch is the worst approach! Copying it is best if you have to do events in multi-threaded environment.
But a better approach is to never use events with multi-threading. Use delegates like callback functions directly!
Leave events to the UI thread. And if you aren't on the UI thread, invoke your function onto the UI thread before invoking an event.
This also eliminates potential issues with event handler/memory leaks from unreleased event binding (which can be a pain in multi-threading).

Kristof said...

@James I agree that try/catch is not a good approach.
Your approach (using callback functions) is unconventional but may work. However, if you need to support multiple subscribers, this does not work. But in some situations this will defenitely help to avoid memory leaks due to dangling subscriptions.