Is this a valid pattern for raising events in C#?

Question

Update: For the benefit of anyone reading this, since .NET 4, the lock is unnecessary due to changes in synchronization of auto-generated events, so I just use this now:

public static void Raise<T>(this EventHandler<T> handler, object sender, T e) where T : EventArgs
{
    if (handler != null)
    {
        handlerCopy(sender, e);
    }
}

And to raise it:

SomeEvent.Raise(this, new FooEventArgs());

---

Having been reading one of Jon Skeet's articles on multithreading, I've tried to encapsulate the approach he advocates to raising an event in an extension method like so (with a similar generic version):

public static void Raise(this EventHandler handler, object @lock, object sender, EventArgs e)
{
    EventHandler handlerCopy;
    lock (@lock)
    {
        handlerCopy = handler;
    }

    if (handlerCopy != null)
    {
        handlerCopy(sender, e);
    }
}

This can then be called like so:

protected virtual void OnSomeEvent(EventArgs e)
{
    this.someEvent.Raise(this.eventLock, this, e);
}

Are there any problems with doing this?

Also, I'm a little confused about the necessity of the lock in the first place. As I understand it, the delegate is copied in the example in the article to avoid the possibility of it changing (and becoming null) between the null check and the delegate call. However, I was under the impression that access/assignment of this kind is atomic, so why is the lock necessary?

Update: With regards to Mark Simpson's comment below, I threw together a test:

static class Program
{
    private static Action foo;
    private static Action bar;
    private static Action test;

    static void Main(string[] args)
    {
        foo = () => Console.WriteLine("Foo");
        bar = () => Console.WriteLine("Bar");

        test += foo;
        test += bar;

        test.Test();

        Console.ReadKey(true);
    }

    public static void Test(this Action action)
    {
        action();

        test -= foo;
        Console.WriteLine();

        action();
    }
}

This outputs:

Foo
Bar

Foo
Bar

This illustrates that the delegate parameter to the method (action) does not mirror the argument that was passed into it (test), which is kind of expected, I guess. My question is will this affect the validity of the lock in the context of my Raise extension method?

Update: Here is the code I'm now using. It's not quite as elegant as I'd have liked, but it seems to work:

public static void Raise<T>(this object sender, ref EventHandler<T> handler, object eventLock, T e) where T : EventArgs
{
    EventHandler<T> copy;
    lock (eventLock)
    {
        copy = handler;
    }

    if (copy != null)
    {
        copy(sender, e);
    }
}

Answer 1

lock (@lock)
{
    handlerCopy = handler;
}

Assignment of basic types like a references are atomic, so there is no point of using a lock here.

Answer 2

The purpose of the lock is to maintain thread-safety when you are overriding the default event wire-ups. Apologies if some of this is explaining things you were already able to infer from Jon's article; I just want to make sure I'm being completely clear about everything.

If you declare your events like this:

public event EventHandler Click;

Then subscriptions to the event are automatically synchronized with a lock(this). You do not need to write any special locking code to invoke the event handler. It is perfectly acceptable to write:

var clickHandler = Click;
if (clickHandler != null)
{
    clickHandler(this, e);
}

However, if you decide to override the default events, i.e.:

public event EventHandler Click
{
    add { click += value; }
    remove { click -= value; }
}

Now you have a problem, because there's no implicit lock anymore. Your event handler just lost its thread-safety. That's why you need to use a lock:

public event EventHandler Click
{
    add
    {
        lock (someLock)      // Normally generated as lock (this)
        {
            _click += value;
        }
    }
    remove
    {
        lock (someLock)
        {
            _click -= value;
        }
    }
}

Personally, I don't bother with this, but Jon's rationale is sound. However, we do have a slight problem. If you're using a private EventHandler field to store your event, you probably have code internal to the class that does this:

protected virtual void OnClick(EventArgs e)
{
    EventHandler handler = _click;
    if (handler != null)
    {
        handler(this, e);
    }
}

This is bad, because we are accessing the same private storage field without using the same lock that the property uses.

If some code external to the class goes:

MyControl.Click += MyClickHandler;

That external code, going through the public property, is honouring the lock. But you aren't, because you're touching the private field instead.

The variable assignment part of clickHandler = _click is atomic, yes, but during that assignment, the _click field may be in a transient state, one that's been half-written by an external class. When you synchronize access to a field, it's not enough to only synchronize write access, you have to synchronize read access as well:

protected virtual void OnClick(EventArgs e)
{
    EventHandler handler;
    lock (someLock)
    {
        handler = _click;
    }
    if (handler != null)
    {
        handler(this, e);
    }
}

UPDATE

Turns out that some of the dialog flying around the comments was in fact correct, as proven by the OP's update. This isn't an issue with extension methods per se, it is the fact that delegates have value-type semantics and get copied on assignment. Even if you took the this out of the extension method and just invoked it as a static method, you'd get the same behaviour.

You can get around this limitation (or feature, depending on your perspective) with a static utility method, although I'm pretty sure you can't using an extension method. Here's a static method that will work:

public static void RaiseEvent(ref EventHandler handler, object sync,
    object sender, EventArgs e)
{
    EventHandler handlerCopy;
    lock (sync)
    {
        handlerCopy = handler;
    }
    if (handlerCopy != null)
    {
        handlerCopy(sender, e);
    }
}

This version works because we aren't actually passing the EventHandler, just a reference to it (note the ref in the method signature). Unfortunately you can't use ref with this in an extension method so it will have to remain a plain static method.

(And as stated before, you have to make sure that you pass the same lock object as the sync parameter that you use in your public events; if you pass any other object then the whole discussion is moot.)

Answer 3

I realize I'm not answering your question, but a simpler approach to eliminating the possibility of a null reference exception when raising an event is to set all events equal to delegate { } at the site of their declaration. For example:

public event Action foo = delegate { };

Answer 4

I'm not convinced about the validity of taking a copy to avoid nulls. The event goes null when all subscribers tell your class not to talk to them. null means no-one wants to hear your event. Maybe the object listening has just been disposed. In this case, copying the handler just moves the problem about. Now, instead of getting a call to a null, you get a call to an event handler that's tried to unsubscribe from the event. Calling the copied handler just moves the problem from the publisher to the subscriber.

My suggestion is just to put a try/catch around it;

    if (handler != null)
    {
        try
        {
            handler(this, e);
        }
        catch(NullReferenceException)
        {
        }
    }

I also thought I'd check out how microsoft raise the most important event of all; clicking a button. They just do this, in the base Control.OnClick;

protected virtual void OnClick(EventArgs e)
{
    EventHandler handler = (EventHandler) base.Events[EventClick];
    if (handler != null)
    {
        handler(this, e);
    }
}

so, they copy the handler but don't lock it.

Answer 5

"Thread-safe" events can become pretty complicated. There's a couple of different issues that you could potentially run into:

NullReferenceException

The last subscriber can unsubscribe between your null check and calling the delegate, causing a NullReferenceException. This is a pretty easy solution, you can either lock around the callsite (not a great idea, since you're calling external code)

// DO NOT USE - this can cause deadlocks
void OnEvent(EventArgs e) {
    // lock on this, since that's what the compiler uses. 
    // Otherwise, use custom add/remove handlers and lock on something else.
    // You still have the possibility of deadlocks, though, because your subscriber
    // may add/remove handlers in their event handler.
    //
    // lock(this) makes sure that our check and call are atomic, and synchronized with the
    // add/remove handlers. No possibility of Event changing between our check and call.
    // 
    lock(this) { 
       if (Event != null) Event(this, e);
    }
}

copy the handler (recommended)

void OnEvent(EventArgs e) {
    // Copy the handler to a private local variable. This prevents our copy from
    // being changed. A subscriber may be added/removed between our copy and call, though.
    var h = Event;
    if (h != null) h(this, e);
}

or have a Null delegate that's always subscribed.

EventHandler Event = (s, e) => { }; // This syntax may be off...no compiler handy

Note that option 2 (copy the handler) doesn't require a lock - as the copy is atomic, there's no chance for inconsistency there.

To bring this back to your extension method, you're doing a slight variation on option 2. Your copy is happening on the call of the extension method, so you can get away with just:

void Raise(this EventHandler handler, object sender, EventArgs e) {
    if (handler != null) handler(sender, e);
}

There is possibly an issue of the JITter inlining and removing the temporary variable. My limited understanding is that it's valid behavior for < .NET 2.0 or ECMA standards - but .NET 2.0+ strengthened the guarantees to make it a non-issue - YMMV on Mono.

Stale Data

Okay, so we've solved the NRE by taking a copy of the handler. Now, we have the second issue of stale data. If a subscriber unsubscribes between us taking a copy and invoking the delegate, then we will still call them. Arguably, that's incorrect. Option 1 (locking the callsite) solves this problem, but at the risk of deadlock. We're kind of stuck - we have 2 different problems requiring 2 different solutions for the same piece of code.

Since deadlocks are really hard to diagnose and prevent, it's recommended to go with option 2. That requires that the callee must handle being called even after unsubscribing. It should be easy enough for the handler to check that it still wants/is able to be called, and exit cleanly if not.

Okay, so why does Jon Skeet suggest taking a lock in OnEvent? He's preventing a cached read from being the cause of stale data. The call to lock translates to Monitor.Enter/Exit, which both generate a memory barrier that prevents re-ordering of reads/writes and cached data. For our purposes, they essentially make the delegate volatile - meaning it can't be cached in a CPU register and must be read from main memory for the updated value each time. This prevents the issue of a subscriber unsubscribing, but the value of Event being cached by a thread who never notices.

Conclusion

So, what about your code:

void Raise(this EventHandler handler, object @lock, object sender, EventArgs e) {
     EventHandler handlerCopy;
     lock (@lock) {
        handlerCopy = handler;
     }

     if (handlerCopy != null) handlerCopy(sender, e);
}

Well, you're taking a copy of the delegate (twice actually), and performing a lock that generates a memory barrier. Unfortunately, your lock is taken while copying your local copy - which won't do anything for the stale data problem Jon Skeet is attempting to solve. You would need something like:

void Raise(this EventHandler handler, object sender, EventArgs e) {
   if (handler != null) handler(sender, e);
}

void OnEvent(EventArgs e) {
   // turns out, we don't really care what we lock on since
   // we're using it for the implicit memory barrier, 
   // not synchronization     
   EventHandler h;  
   lock(new object()) { 
      h = this.SomeEvent;
   }
   h.Raise(this, e);
}

which doesn't really look like much less code to me.

Answer 6

There's multiple issues at hand here, and I'll deal with them one at a time.

Issue #1: Your code, do you need to lock?

First of all, the code you have in your question, there is no need for a lock in that code.

In other words, the Raise method can be simply rewritten to this:

public static void Raise(this EventHandler handler, object sender, EventArgs e)
{
    if (handler != null)
        handler(sender, e);
}

The reason for this is that a delegate is an immutable construct, which means that the delegate you get into your method, once you're in that method, will not change for the lifetime of that method.

Even if a different thread is mucking around with the event simultaneously, that will produce a new delegate. The delegate object you have in your object will not change.

So that's issue #1, do you need to lock if you have code like you did. The answer is no.

Issue #3: Why did the output from your last piece of code not change?

This goes back to the above code. The extension method has already received its copy of the delegate, and this copy will never change. The only way to "make it change" is by not passing the method a copy, but instead, as shown in the other answers here, pass it an aliased name for the field/variable containing the delegate. Only then will you observe changes.

You can look at this in this manner:

private int x;

public void Test1()
{
    x = 10;
    Test2(x);
}

public void Test2(int y)
{
    Console.WriteLine("y = " + y);
    x = 5;
    Console.WriteLine("y = " + y);
}

Would you expect y to change to 5 in this case? No, probably not, and it's the same with delegates.

Issue #3: Why did Jon use locking in his code?

So why did Jon use locking in his post: Choosing What To Lock On? Well, his code differs from yours in the sense that he didn't pass a copy of the underlying delegate anywhere.

In his code, which looks like this:

protected virtual OnSomeEvent(EventArgs e)
{
    SomeEventHandler handler;
    lock (someEventLock)
    {
        handler = someEvent;
    }
    if (handler != null)
    {
        handler (this, e);
    }
}

there is a chance that if he didn't use locks, and instead wrote the code like this:

protected virtual OnSomeEvent(EventArgs e)
{
    if (handler != null)
        handler (this, e);
}

then a different thread could change "handler" between the expression evaluation to figure out if there are any subscribers, and up to the actual call, in other words:

protected virtual OnSomeEvent(EventArgs e)
{
    if (handler != null)
                         <--- a different thread can change "handler" here
        handler (this, e);
}

If he had passed handler to a separate method, his code would've been similar to yours, and thus required no locking.

Basically, the act of passing the delegate value as a parameter makes the copy, this "copy" code is atomic. If you time a different thread right, that different thread will either make its change in time to get the new value with the call, or not.

One reason to use a lock even when you call might be to introduce a memory barrier, but I doubt this will have any impact on this code.

So that's issue #3, why Jon's code actually needed the lock.

Issue #4: What about changing the default event accessor methods?

Issue 4, which have been brought up in the other answers here revolve around the need for locking when rewriting the default add/remove accessors on an event, in order to control the logic, for whatever reason.

Basically, instead of this:

public event EventHandler EventName;

you want to write this, or some variation of it:

private EventHandler eventName;
public event EventHandler EventName
{
    add { eventName += value; }
    remove { eventName -= value; }
}

This code does need locking, because if you look at the original implementation, without overridden accessor methods, you'll notice that it employs locking by default, whereas the code we wrote does not.

We might end up with an execution scenario that looks like this (remember that "a += b" really means "a = a + b"):

Thread 1              Thread 2
read eventName
                      read eventName
add handler1
                      add handler2
write eventName
                      write eventName  <-- oops, handler1 disappeared

To fix this, you do need locking.