multithreading - 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)
    {
        handler(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

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