The proper way of raising events in the .NET frame

Question

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 misled by a bad pattern.

Answer 1

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

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.

Answer 2

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

Answer 3

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

Answer 4

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.

Answer 5

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

Answer 6

Just to clarify. The approach using the empty delegate as the initial value for the event works even when used with serialization:

// to run in linqpad:
// - add reference to System.Runtime.Serialization.dll
// - add using directives for System.IO and System.Runtime.Serialization.Formatters.Binary
void Main()
{
    var instance = new Foo();
    Foo instance2;
    instance.Bar += (s, e) => Console.WriteLine("Test");
    var formatter = new BinaryFormatter();
    using(var stream = new MemoryStream()) {
        formatter.Serialize(stream, instance);
        stream.Seek(0, SeekOrigin.Begin);
        instance2 = (Foo)formatter.Deserialize(stream);
    }
    instance2.RaiseBar();
}

[Serializable]
class Foo {
    public event EventHandler Bar = delegate { };
    public void RaiseBar() {
        Bar(this, EventArgs.Empty);
    }
}

// Define other methods and classes here