Share via

Events and Races

  • Article

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

  • Anonymous
    April 29, 2009
    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.

  • Anonymous
    April 29, 2009
    The comment has been removed

  • Anonymous
    April 29, 2009
    +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.

  • Anonymous
    April 29, 2009
    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#...

  • Anonymous
    April 29, 2009
    @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

  • Anonymous
    April 29, 2009
    The comment has been removed

  • Anonymous
    April 29, 2009
    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?

  • Anonymous
    April 29, 2009
    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});

  • Anonymous
    April 29, 2009
    The comment has been removed

  • Anonymous
    April 29, 2009
    Should this pattern be applied to not thread-safe classes? What is the benefit in this case?

  • Anonymous
    April 29, 2009
    The comment has been removed

  • Anonymous
    April 29, 2009
    Thank you for submitting this cool story - Trackback from DotNetShoutout

  • Anonymous
    April 30, 2009
    @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.

  • Anonymous
    April 30, 2009
    "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.

  • Anonymous
    April 30, 2009
    @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.

  • Anonymous
    April 30, 2009
    The comment has been removed

  • Anonymous
    April 30, 2009
    The comment has been removed

  • Anonymous
    April 30, 2009
    @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

  • Anonymous
    April 30, 2009
    The comment has been removed

  • Anonymous
    May 22, 2009
    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

  • Anonymous
    June 29, 2009
    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

  • Anonymous
    August 06, 2009
    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.

  • Anonymous
    March 08, 2010
    The comment has been removed

  • Anonymous
    September 08, 2010
    The comment has been removed

  • Anonymous
    October 09, 2010
    The comment has been removed

  • Anonymous
    April 20, 2011
    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?

  • Anonymous
    May 03, 2011
    So why doesn't the compiler generate this boilerplate code automatically?

  • Anonymous
    February 09, 2012
    The comment has been removed

  • Anonymous
    May 06, 2012
    The comment has been removed

  • Anonymous
    June 14, 2012
    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.