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

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

  • 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

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

  • @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!

  • 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

  • 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

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

  • @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);

    }

  • 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?

  • @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();

  • 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?

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

  • 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;

    }

  • 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

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

Page 2 of 2 (30 items) 12