Alex Rivera | Logout

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

Asked 2010-01-23T15:17:01.110
10

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 = () => C
c# multithreading events locking thread-safety
Edit
Report

1 Answer

7

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 s

answered 2010-01-23T16:34:24.737

Your Answer