Events and Races

Events and Races

Rate This
  • Comments 30

Here’s a question similar to one I saw on stackoverflow the other day. Suppose you have an event:

public event Action Foo;

The standard pattern for firing this event is:

Action temp = Foo;
if (temp != null)
      temp();

What the heck is up with that? Why not just call “Foo()” ?

First off, this pattern ensures that the thing that is invoked is not the null delegate reference, which would cause a null reference exception to be thrown. But if that’s what we want, then surely we could have skipped the temporary variable – just “if (Foo != null) Foo();” would do. Why the temp?

The temporary variable ensures that this is “thread safe”. If the event’s handlers are being modified on another thread it is possible for Foo to be non-null at the point of the check, then set to null on a different thread, and then the invocation throws.

Using the temporary variable effectively makes a copy of the current set of event handlers. Remember, multi-cast delegates are immutable; when you add or remove a handler, you replace the existing multi-cast delegate object with a different delegate object that has different behaviour. You do not modify an existing object, you modify the variable that stores the event handler. Therefore, stashing away the current reference stored in that variable into a temporary variable effectively makes a copy of the current state.

Is that clear? Make sure it is, because now things start to get really confusing.

A common criticism of this pattern is that it trades one race condition for another. Let’s consider that scenario again more carefully:

The event handler contains a single handler, H. Thread Alpha makes a copy of the delegate to H in temp and determines that it is not null. Thread Beta decides that H must no longer be called when the event is fired, so it sets the handler to null. Thread Beta then assumes that H will never be called, and destroys a bunch of state that H needs to execute successfully. Thread Alpha then regains control and executes H, which behaves crazily since its necessary state has been destroyed. Thread Beta’s attempt to ensure that H is not called has been defeated by the race condition.

A frequently-stated principle of good software design is that code which calls attention to bugs by throwing exceptions is better than code which hides bugs by muddling on through and doing the wrong thing. This code has a race condition that results in incorrect behaviour. Perhaps an application of that principle is to stop using a temporary and instead crash if it races. That is to say, this code has a failure mode; surely it is better to highlight that failure with a crisp exception than to turn it into crazy wrong behaviour.

That seems plausibly argued, I agree, but the conclusion is wrong.

Suppose we remove the temporary variable but keep the null check. Does that solve the problem? No! We still have the same race conditions. Suppose the event handler delegate contains a reference to H. Thread Alpha checks to see whether it is null; it is not. Thread Alpha pushes the object to invoke on the runtime stack. Between the push of the delegate value and the call to invoke it, thread Beta sets the event handler to null. And once again, H will be invoked after thread Beta has removed the handler.

Suppose we remove the temporary and the null check and just invoke the delegate directly. Does that help? No, we still have the same race condition. The contents of the event handling variable can still be changed between the push of the delegate object onto the stack and the invocation of the delegate.

Removing the code around the call site does not decrease the number of race conditions in the code, and it does increase the number of crashes. Worse, doing so makes the race condition harder to detect by shrinking the window in which the race can occur without eliminating it.

Removing the null check and/or the temporary is a bad idea; the already-bad situation is only made worse.

Essentially there are two problems here that are being conflated. The two problems are:

1) The event handler delegate can be null at any time.
2) A “stale” handler can be invoked even after it has been removed.

These two problems are actually orthogonal and have different solutions. The onus for solving the first problem is laid upon the code which does the invocation; it is required to ensure that it does not dereference null. The store-in-temporary-and-test pattern ensures that null is never dereferenced. (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.)

The onus for solving the second problem is laid upon the code being invoked; event handlers are required to be robust in the face of being called even after the event has been unsubscribed. In the scenario I described, the bug is actually in H. It needs to be robust enough to check to see whether the state it needs is still there, and bail out cleanly if it is not. (Or, alternatively, some additional locking mechanism needs to be implemented which ensures that the code which fires the event cooperates with the code that changes the event handlers to ensure the desired behaviour.)

The point though is that the "null ref problem" and the "stale handler problem" are two separate problems that are easily confused because the symptoms of both arise at the exact same call site code. You've got to solve both problems if you want to do threadsafe events. How you do so is up to you; just don't confuse the solution of one problem for a solution of the other.

(Thanks to Microsofties Levi Broderick, Chris Burrows, Curt Hagenlocher and Wolf Logan; this article is based on a conversation about their analysis of this pattern.)

  • Personally, I find it frustrating that events aren't automatically created.  I can't think of a reason to not _always_ create an empty list, so that you don't have to do the null check in the first place.  Removing one event from the multidelegate should leave you with a list with count 0, not a null reference.

  • It's possible to run into the 2nd race condition without even having multiple threads. Here was my take on it a while ago -- the behavior surprised me when I realized it. The point of my article is to call it out, as I really do not think many developers realize a handler could be called after it is already removed. Even, as I say, if there's no multi threading.

    http://weblogs.asp.net/infinitiesloop/archive/2009/01/14/the-event-handler-that-cried-wolf.aspx

  • +1 to Andrew's comment

    The null check thing is rather annoying. Now that we have things the way they are, it would be nice if the language made it easy to do this right and harder to do it wrong.

  • In VB.NET there is a RaiseEvent keyword that is used, as the name implies, to raise events. There is no need to check whether the delegate is null. Eric, do you have any idea how it is implemented ? Is it thread safe ? Maybe there's a way to mimic the behavior in C#...

  • @Thomas,

    If you use a disassembler to examine the code that is generated by RaiseEvent, you will discover that it is EXACTLY the same as the pattern Eric posted above: assignment to a temporary variable, then a null check, then invocation of the delegate.

    This is one of the few things I like better about VB.NET than C#. If events were truly first class in C#, we wouldn't have to write this pattern each and every time we invoke them.

    "Each design pattern is a sign of a weakness in the programming language to which it applies." - http://blog.plover.com/prog/johnson.html

  • public event Action Foo = ()=>{}; works but is fugly. Any other solutions except the pattern (which is what I currently use)?

  • It would have been nice if, in C# 1.0, multicast delegates behaved as if they were actually ICollection<delegate>; one would be able to ask for its Count, etc. As it is, the null return from an empty multicast delegate is the only way to determine whether it's empty, so strictly speaking I assume that changing its behaviour to that of an empty list instead would be a breaking change.

    Still, I wonder how much code is depending on the null return for some logic other than deciding not to invoke? That is, if multicast delegates a) never returned null and b) were always safe to invoke, even when empty, is there any code that would really break?

  • This is one of those times when I find a few extension methods invaluable:

       public static void SafeInvoke(this EventHandler handler, object sender) {

           if(handler != null) handler(sender,EventArgs.Empty);

       }

    then it becomes:

       SomeEvent.SafeInvoke(this);

    For those times when I *do* use the temp variable, I am massively, massively grateful for implicit typing...

       var handler = SomeEvent;

       if(handler != null) handler(this, {some args});

  • configurator is correct, the compiled empty lambda means there is no chance in getting the race conditions mentioned by this article.

    The  only ones I can think of is that because event handlers are called synchronously and one at a time there is still a slight chance that by then the subscriber is no longer an interested party.

  • Should this pattern be applied to not thread-safe classes? What is the benefit in this case?

  • @commongenius : thanks for the explanation, I didn't think of using Reflector for that...

    @configurator : actually I kind of like this trick... it's really short to write, and it protects against the "null ref" race condition, which is the most commonly encountered. However you still have the "slate handler" issue, so the handler must still check the state of its environment

  • Thank you for submitting this cool story - Trackback from DotNetShoutout

  • @Eric - thanks, I've added some more questions to the Stack Overflow question... :)

    @Dave Reed - beautiful example of the single-threaded problem case!

    @configurator - this is one situation where the 2.0 syntax is better than lambdas, because the empty delegate doesn't need to specify the parameters:

       public event EventHandler<MyEventArgs> MyEvent = delegate {};

    I can't understand why that's not the "standard pattern", as long as the firing code doesn't need to do anything special (besides "nothing") for the case where there are no subscribers.

  • "Event handlers are required to be robust in the face of being called even after the event has been unsubscribed." This is a non-obvious rule that I hope becomes more well known. In my attempt to write thread-safe events, the only solution I could think of was to call the event handler from within the event handler's lock, which is dangerous, I know.

    http://code.logos.com/blog/2008/05/events_and_threads_part_3.html

    Better to simply not make the promise. An unsubscribed event handler may still be called.

  • @Daniel, It's not accurate to say that @configurator's solution has no chance of failing. It has a reduced chance of failing. The object that owns the event can still explicitly set the event handler to null at any time.

    Additionally, there's a reason that the default pattern has the sender (ie the this pointer) as the first argument so that the recipient has a chance to check if it's still an object they care about.

    In retrospect it would have been nicer to have a Null Object Pattern and not allow nulls in the first place.

Page 1 of 2 (30 items) 12