views:

1044

answers:

5

Currently "Avoid checking for null event handlers" is at the top of the answers to the post titled Hidden Features of C# and it contains severely misleading information.

While I understand that Stack Overflow is a "democracy" and the answer rose to the top by virtue of public voting, I feel that a lot of people that up-voted the answer either did not have a complete understanding of C#/.NET or did not take the time to fully understand the consequences of the practice described in the post.

In short the post is advocating the use of the following construct to avoid having to check for null when invoking the event.

public event EventHandler SomeEvent = delegate {};
// Later..
void DoSomething()
{
   // Invoke SomeEvent without having to check for null reference
    SomeEvent(this, EventArgs.Empty);  
}

At a first glance this may seem like a clever shortcut but it can be the cause for some serious headaches in a large application, especially if concurrency is involved.

Before calling the delegate of an event you have to check for a null reference. Just because you have initialized the event with an empty delegate does not mean that a user of your class won't set it to null at some point and break your code.

Something like this is typical:

void DoSomething()
{
    if(SomeEvent != null) 
        SomeEvent(this, EventArgs.Empty);
}

But even in the above example the possibility exists that while DoSomething() may be run by a thread, another could remove the event handlers, and a race condition could ensue.

Assume this scenario:

      Thread A.                           Thread B.
    -------------------------------------------------------------------------
 0: if(SomeEvent != null)
 1: {                                     // remove all handlers of SomeEvent
 2:   SomeEvent(this, EventArgs.Empty);
 3: }

Thread B removes the event handlers of the SomeEvent event after the code that raises the event has checked the delegate for a null reference, but before it called the delegate. When the SomeEvent(this, EventArgs.Empty); call is made, SomeEvent is null and an exception is raised.

To avoid that situation, a better pattern to raise events is this:

void DoSomething()
{
    EventHandler handler = SomeEvent;
    if(handler != null)
    {
        handler(this, EventArgs.Empty);
    }
}

For an extensive discussion of the topic of EventHandlers in .NET I suggest reading "Framework Design Guidelines" by Krzysztof Cwalina and Brad Abrams, Chapter 5, Section 4 - Event Design. Especially the discussions of the topic by Eric Gunnerson and Joe Duffy.

As suggested by Eric, in one of the answers below, I should point out that a better synchronization solution could be devised that would take care of the problem. My goal with this post was to raise awareness and not to provide a one-and-only-true solution to the problem. As suggested by Eric Lippert and by Eric Gunnerson in the above mentioned book, the particular solution to the problem is up to the programmer but what is important is that the issue not be disregarded.

Hopefully a moderator will annotate the answer in question so that unsuspecting readers won't be mislead by a bad pattern.

+1  A: 

For what it's worth, you should really look into Juval Lowy's EventsHelper class rather than doing things yourself.

Jeff Moser
Jeff, I'm not advocating re0inventig the wheel here. I'm simply trying to show why checking for null is required so that people can understand why it's important to do so (and them maybe to look for a helper class if doing things by ones-self is too hard..)
Miky Dinescu
A: 

Just as a note, http://blogs.msdn.com/ericlippert/archive/2009/04/29/events-and-races.aspx

This is a perma-link to the article Erik was refering to.

LPCRoy
+4  A: 

"Just because you have initialized the event with an empty delegate does not mean that a user of your class won't set it to null at some point and break your code."

Can't happen. Events "can only appear on the left hand side of += or -= (except when used from within the type)" to quote the error you'll get when doing this. Granted, the "except when used from within the type" makes this a theoretical possibility, but not one that any sane developer would be concerned with.

wekempf
+1 - this simple fact invalidates the rest of the original post.
Daniel Earwicker
Well, to be fair, there is one way I can think of to get a null value here: serialization would, I believe, set it to null. Again, though, that's something in control of the class implementation.
wekempf
+9  A: 

I raised the same issue about a week ago and reached the opposite conclusion:

http://stackoverflow.com/questions/786383/c-events-and-thread-safety

Your summary doesn't do anything to persuade me otherwise!

First, clients of the class cannot assign null to the event. That's the whole point of the event keyword. Without that keyword, it would be a field holding a delegate. With it, all operations on it are private except enlisting and delisting.

As a result, assigning delegate {} to the event at construction completely meets the requirements of a correct implementation of an event source.

Of course, within the class there may be a bug where the event is set to null. However, in any class that contains a field of any type, there may be a bug that sets the field to null. Would you advocate that every time ANY member field of a class is accessed, we write code like this?

// field declaration:
private string customerName;

private void Foo()
{
    string copyOfCustomerName = customerName;
    if (copyOfCustomerName != null)
    {
        // Now we can use copyOfCustomerName safely...
    }
}

Of course you wouldn't. All programs would become twice as long and half as readable, for no good reason. The same madness occurs when people apply this "solution" to events. Events are not public for assignment, the same as private fields, and so it is safe to use them directly, as long as you initialize them to the empty delegate on construction.

The one situation you cannot do this in is when you have an event in a struct, but that's not exactly an inconvenience, as events tend to appear on mutable objects (indicating a change in the state) and structs are notoriously trick if allowed to mutate, so are best made immutable, and hence events are of little use with structs.

There may exist another quite separate race condition, as I described in my question: what if the client (the event sink) wants to be sure that their handler will not be called after it has been delisted? But as Eric Lippert pointed out, that is the responsibility of the client to solve. In short: it is impossible to guarantee that an event handler will not be called after it has been delisted. This is an inevitable consequence of delegates being immutable. This is true whether threads are involved or not.

In Eric Lippert's blog post, he links to my SO question, but then states a different but similar question. He did this for a legitimate rhetorical purpose, I think - just in order to set the scene for a discussion about the secondary race condition, the one affecting the handlers of the event. But unfortunately, if you follow the link to my question, and then read his blog post a little carelessly, you might get the impression that he is dismissing the "empty delegate" technique.

In fact, he says "There are other ways to solve this problem; for example, initializing the handler to have an empty action that is never removed", which is the "empty delegate" technique.

He covers "doing a null check" because it is "the standard pattern"; my question was, why is this the standard pattern? Jon Skeet suggested that given that the advice predates anonymous functions being added to the language, it's probably just a hangover from C# version 1, and I think that is almost certainly true, so I accepted his answer.

Daniel Earwicker
One thing this fails to address is performance. Mutlicast is MUCH slower. For an occasional event that's not big deal but if you raise a lot of them it _might_ be an issue.
Joel Coehoorn
I think SO should start with a page that you see before you sign up that says "I understand that premature optimization is the root of all evil, and I undertake never to use guesswork in choosing ways to make my programs run faster, but instead to implement the cleanest possible design and code structure, and use a profiler to identify the bottlenecks that actually require messy optimisztions to be applied."
Daniel Earwicker
Also, when a event only has an empty action on it, it's not multicast. It's only got one handler, and the invocation overhead on a singlecast delegate is unbelievably small.
Daniel Earwicker
Nicely said Earwicker, any copy of a reference should check it is not null (hopefully before the object is copied!) before being used. I've never thuoght, "oh that class has no internal bugs in it."
Russell
A: 

Brumme is the daddy for both Eric and Abrams.. you should read his blog rather than preaching from either of the two publicists. The guy is seriously technical (as opposed to high-level hair-stylists logos ). He will give you a proper explanation without 'Redmond Flowers in 1TB land' on why races and memory models are a challenge for a managed (re: shield-the-children) environment as raised by another poster above.

Btw, it all starts with them, the C++ CLR implementation guys:

blogs.msdn.com/cbrumme

rama-jka toti
To advise anyone interested in this stuff to NOT read Eric Lippert's blog is insanely bad advice. To then suggest that instead they read a blog that is update twice in the last four years (about a technology that moves as fast as .NET and its languages) is verging on the deliberately sarcastic.
Daniel Earwicker
Brumme's blog is worth reading as well, however.
Daniel Earwicker