Field-like Events Considered Harmful

Hi all, my name's Chris Burrows, and I'm a developer on the C# Compiler team here in Redmond. I'll introduce myself further in a subsequent post, but I just want to start off with a note that has at least a little substance.

So let's talk about C# events as they've existed for a long time now, and their implementation in the compiler. Last week, I was responding to an issue involving events, I was surprised to discover that the language spec tries to make some guarantee that field-like events are, as it says, "thread safe." When I say "field-like events," I mean events for which you have not defined your own add and remove accessors. Let's have a look at the C# 3.0 specification (10.8.1):

In order to be thread-safe, the addition or removal operations are done while holding the lock (8.12) on the containing object for an instance event, or the type object (7.5.10.6) for a static event.

Thus, an instance event declaration of the form:

 class X
{
    public event D Ev;
}

could be compiled to something equivalent to:

 class X
{
    private D __Ev; // field to hold the delegate
    public event D Ev
    {
        add
        {
            lock (this) { __Ev = __Ev + value; }
        }

        remove
        {
            lock (this) { __Ev = __Ev - value; }
        }
    }
}

Ok. Let's start at the beginning. You should be wary of the language here that implies that the implementation of events is "thread safe." Thread safety, if you care about it at all for this event, is still something that you need to worry about on your own. What happens if you have some other method in the class that adds or removes delegates from the event field? Are those in a context where you've gotten a lock on the proper object? How about the invocations of that delegate? And what if thread safety isn't even a concern for you in this case? If so, it's a little weird that you have to pay for it.

There are bigger concerns though--in a sense I suppose it's kind of obvious that if you want thread safety you're going to have to worry about it in the rest of your code. But look at the lock. It uses the containing object's sync block! You're never supposed to lock(this)! It's one of those things on .NET style lists marked "never do." The big issue here is that the containing object's sync block is essentially public. If anyone else happens to use it elsewhere (for instance, your class's consumers), then you've set yourself up for threading problems.

And it gets worse. If you read on in the spec, you'll see that if we had declared that event static, then the lock would not have been on this, because there is no this. It would have been on X's type:

 class X
{
    private static D __Ev; // field to hold the delegate
    public static event D Ev
    {
        add
        {
            lock (typeof(X)) { __Ev = __Ev + value; }
        }
        remove
        {
            lock (typeof(X)) { __Ev = __Ev - value; }
        }
    }
}

This has the potential to be a huge problem! It's got all the issues associated with lock(this), except that the type object can be loaded in a domain-neutral context, so that it's shared across all app domains. There, now you've just busted through the app domain isolation and you can cause issues across your entire process for anyone using this event. Potentially.

But let's back up a second. The language in the spec is a little weasely. It says that these forms "could be compiled to something equivalent to..." What does that mean? In fact, in the most up-to-date C# compiler (as well as previous versions), the code is compiled to something equivalent to these add/remove blocks, but only in the case where X is a reference type. If we look at the IL for X.Ev.add, it does not contain the lock or any mention of the Monitor class, but the method's metadata looks like this:

 .method public hidebysig specialname instance void
    add_Ev(class D 'value') cil managed synchronized
    { ... }

See that "synchronized?" That tells the runtime to wrap the method in the equivalent of the two lock statements that the spec mentions (depending on whether the method is instance or static). If you want to accomplish the same thing explicitly (which maybe you don't), you would get System.Runtime.CompilerServices in scope and decorate the method with:

 [MethodImpl(MethodImplOptions.Synchronized)]

The compiler only inserts this for events on reference types because if you've defined the same event on a value type, it can't work--the sync block will be different for each boxing of the value.

So if we're telling people never to use lock(this), and we're telling people really never ever pretty please don't use lock(typeof(X)), then what should we say about field-like events and whether you should use them? Well, they are an awfully convenient syntax for something that would ordinarily be a lot of boilerplate code, which is evidently a good thing if we're to judge by the introduction of automatic properties in C# 3.0. Events do seem to be a likely surface area for inter-thread communication. Perhaps that explains why the original language designers settled on this behavior so many years ago. I wasn't on the team at the time, so I can't say.

The title of this post is a joke. I'm not telling you not to use field-like events. However...

If I were writing C# code, and I were a stickler for style and correctness, I would define my own event accessors. There are other reasons to do so, but I'll leave myself something else to talk about in a future post. If I knew about this behavior and didn't care, I wouldn't change anything. And in the meantime, you can make your own decisions about your own code using whatever unique requirements you have for it.