Events and Races


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

Comments (30)

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

  2. Dave Reed says:

    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

  3. Brad Dodson says:

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

  4. 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#…

  5. commongenius says:

    @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

  6. configurator says:

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

  7. Wolf Logan says:

    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?

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

  9. Daniel Fernandes says:

    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.

  10. Viacheslav Ivanov says:

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

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

  12. Thank you for submitting this cool story – Trackback from DotNetShoutout

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

  14. Ed Ball says:

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

  15. AC says:

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

  16. AC says:

    And if there’s a vote, I vote for Mark Gravell’s explicit extension method over the empty delegate.

    I would be fine with the empty delegate and increased overhead if it was bulletproof, but it’s not. Everyone goes on and on about how they’ve saved 14 characters and I just don’t see why it’s such a big deal.

  17. GregUzelac says:

    What about "Unfortunately, the JIT compiler may optimize the code, eliminate the temporary variable, and use the original delegate. (as per Juval Lowy – Programming .NET Components) So to avoid this problem, you could use method which accepts a delegate as parameter:" from http://stackoverflow.com/questions/292820/how-to-correctly-deregister-an-event-handler

  18. Ed Ball says:

    @Greg, AFAIK, under CLR 2.0’s stronger memory model, the optimization issue is no longer a potential problem.

    http://code.logos.com/blog/2008/11/events_and_threads_part_4.html

  19. @AC – I bet I could invalidate any solution to any problem by sticking a null in the wrong place – what does that prove? 🙂 The concern here is more subtle IMO. Two separate classes are each written correctly under their own assumptions, but those assumptions are invalidated in a subtle way by trying to combine them – and the question is, what is the easiest, simplest, least messy way to eliminate the problem, given that it is a "lurking" problem rather than an obvious one?

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

    That’s not a good reason for making it the default. How often is construction of the event arguments expensive enough to warrant caring about it? And so cause millions of coders to hand-craft a little pile of ceremony-code, all based on premature optimization, every time they call through an event field?

    I think the present situation with the language is fine (the only thing that’s wrong is the widespread out-of-date advice). In the very rare case where event argument construction is expensive enough to worry about, the coder can use null to represent the empty list if it’s really beneficial.

    Everyone else can put "= delegate { }" after their event declarations, completely free from concern about this stuff – just call events directly, succinctly and safely, as if they were methods.

    Fortunately it’s not a vote – you are completely free to do this in an unnecessarily hard way if you like!

  20. Lee Campbell says:

    I see this whole probelm as a failing of the C# language. Technically .NET 4.0 is still in beta, so can we all kick and scream enough to get this solved?

    I am a fan of C# but @commongenius is right that this “pattern” really indicates that something needs to be fixed.

    OK, let’s say for the sake of argument that we’ve found a problem. What implies that it NEEDS to be fixed? A fix is a change, every change has both costs and benefits, and the benefits do not necessarily outweigh the costs. And every unneecssary fix is time and effort stolen from more important changes. This doesn’t NEED to be fixed; people work with imperfect tools every day and will continue to do so.  — Eric

     Gosh, imagine all of the If(handler!=null) handerl(this, EventArgs.Empty) code that is out there.

    I am imagining it. Imagine the cost if a fix to the problem introduced a breaking change that broke even some of that code. The devil you know is better than the devil you don’t know. — Eric

  21. Dave says:

    Is there a reason why this pattern isn’t used in very many places in the Framework code itself?  It took about two minutes with Reflector to find lots of events being raised with the check but no temporary in code in System.Drawing, System.Net, and System.Windows and these were the first and only three I checked.  Are there good reasons why so many Framework classes ignore the temporary pattern or are these all bugs?  If these are bugs, it really argues that this is broken at the language level.  If Microsoft can’t get it right in the Framework, how do you expect us to?

    Are the classes in question documented as being threadsafe? If not, then why do you expect that they’d be written to be threadsafe in their event code? — Eric

  22. Tom says:

    Great article.  I’d never even heard about the temp argument until today.  On that, some comments:

    @Marc Gravelll:  Brilliant solution.  I’ve implemented it in my current project and will continue to do so from here on, or until C# x.0 fixes it.

    @Dave:  Excellent research, and an equally excellent question to which I’d like an answer as well.  Is there a reason for this, or is it being fixed in C# 4.0?

    As for the mutable state data issue, as a general rule I pass all necessary event data immutably through the event args construct.  A handler can, of course, request other data, but I try not to do that.  Handlers exist to forward, create, or alter data; they aren’t supposed to retrieve it except through event args.  Naturally there will be exceptions, but this is a good design philosophy.

  23. Gilles says:

    @Daniel – In the very rare case where event argument construction is expensive enough to worry about, the coder can use null to represent the empty list if it’s really beneficial.

    I’d gladly trade the crashing potential of the current state of affairs for the equally unlikely downside possibility of having to unnecessarily construct my event arguments.

    // New pattern which now has a race condition though with only CPU cycles wasted when it happens

    if (handler.SubscriberCount != 0) {

       var expensiveArgs = … // expensive argument creation

       handler(this, expensiveArgs);

    }

  24. Brian Gideon says:

    Is there not a subtle memory barrier problem with that pattern? Capturing the Foo variable into a local variable will definitely prevent the NullReferenceException, but unless there is some magic going on that I am unaware of then the thread could reorder the read of Foo and cause it to persistently see a null value even though event handlers have been recently chained.  In most cases it would be moot since you typically only extract the event once during a method call.  However, consider a scenario where you periodically refresh your local copy in a while loop.  The naive solution is to refactor the event invocation into a separate method that does the local copy and null check under the assumption that the compiler will not lift the read outside of method calls, but the method could be inlined circumventing your strategy.  Comments?

  25. andras says:

    @Brian: you are perfectly right in that to be absolutely correct there should be a memory barrier between the load and the conditional. This would make it similar to a volatile read:

    Action temp = Foo;

    Thread.MemoryBarrier();

    if (temp != null)

         temp();

  26. anant says:

    hi,

    I am facing similar kind of problem. In my application i am getting registered event unregistered automatically. There is no place in application where i am unregistering event. The scenario is if modal form is opened then that event becomes null, and if that form is closed it is working fine. Can you please help me in this?

  27. Martin says:

    So why doesn't the compiler generate this boilerplate code automatically?

  28. frosted says:

    may be "try-catch" could solve the problem?

    like this way in pseudocode:

    try

    {

         Foo();

    }

    catch(NullReferenceException ex)

    {

         if (!ex.stackTrace.UpperMethod != Reflection.CurrentMethod)

                throw;

    }

  29. Avi Farah says:

    A pattern that I understand was proposed by Juval Lowe is:

    public event Action Foo = delegate {};

    Thereafter you need not check if Foo or if temp is null or not, just use it in a threat safe manor…

    Cheers,

    Avi

  30. oleh.zheleznyak@gmail.com says:

    It is crystal clear what is going on, but sometimes the exact phrasing needs to be more precise, for example:

    "Therefore, stashing away the current reference stored in that variable into a temporary variable effectively makes a copy of the current state."

    "Thread Alpha makes a copy of the delegate to H in temp and determines that it is not null. "

    I want to emphasize that the reference to the delegate is being copied, not the delegate instance itself. Due to the immutability of multicast delegate we can be sure that the object our  temp variable is pointing to will not change, and any "changes" made to the original event will create a new delegate instance. But if no changes are made, the variables will be pointing to the same instance of the multicast delegate.

    The word "copy" is not quite accurate for these scenarios.