views:

402

answers:

6

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);
    }
}
+1  A: 
lock (@lock)
{
    handlerCopy = handler;
}

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

codymanix
So why does Jon Skeet's example use a lock? He also mentions that the `add`/`remove` accessors for events should use the same lock.
Will Vousden
Atomicity isn't always enough - using a lock means that you get the most recent value, rather than a potentially cached one.
Jon Skeet
@Jon: Since the delegate field's reference is changed by the `+=` and `-=` operators (as you explained below), is the use of the lock in my extension method still valid and will it still achieve what it is supposed to? Passing the delegate field as a parameter to the extension method will simply pass the reference, so if that reference is mutated by another thread by `+=` or `-=`, the extension method won't know about it, will it?
Will Vousden
+5  A: 

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

Aaronaught
This is nonsense. The compiler generates add and remove methods for a plain event, no different from the hand-written ones. The _click delegate instance is just as thread-safe.
Hans Passant
Thanks for the answer. I still don't quite understand why it's necessary to synchronize read/write access to the backing delegate, though. You say that `clickHandler = _click` *is* atomic, but what about `_click += value` and `_click -= value`? Are these not also atomic?Also, Mark Simpson mentioned in a comment on my question that the act of calling the extension method with the handler as a parameter would in fact perform the read access to the delegate itself, which would render the lock inside the method ineffectual. Is this a problem?
Will Vousden
@Zakalwe: Storing the reference is atomic. However, two threads could both call Delegate.Combine (which is what += turns into) then both could read the same original value (x) and produce different values (y and z) each with one extra delegate subscribed. Each would then assign the value to the field, resulting in one of the handlers being "lost".
Jon Skeet
@nobugz: Then you are saying Jon Skeet is wrong? He says, and I quote: **not only do many books and articles recommend locking on `this`: the C# compiler does it for you automatically if you declare an event without specifying the add/remove code.**
Aaronaught
Speak of the devil! Ha ha.
Aaronaught
I stand corrected, the compiler generated ones have the synchronized attribute. Yuck.
Hans Passant
@Jon: Thanks, I get it now.
Will Vousden
@Aaronaught: I'd just like to point out that there's nothing wrong with claiming that I'm wrong. It happens, and I don't want anyone to feel "scared" of claiming it. In this particular case I'm right (as nobugz has acknowledged) but it's always worth quibbling if you disagree :)
Jon Skeet
@Jon: See my response to your other comment!
Will Vousden
@Jon Skeet: Of course, and I answered the question because I knew the info to be correct, but it was so much easier to invoke you as an authority than it would have been to argue. :-P
Aaronaught
@Zakalwe: I think I can field that one, if you're referring to Mark Simpson's comment. Extension methods aren't real instance methods, as evidenced by the fact that you can (successfully) invoke them on a null reference depending on how you write them. They are merely syntactic sugar for external static methods and I am 99% positive that using one here will **not** circumvent the lock.
Aaronaught
@Aaronaught: I'm not so sure; see my update.
Will Vousden
Your answer is inconsistent. Since delegates are immutable and assignment is atomic, the field cannot be in an inconsistent state.
SLaks
+1  A: 

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 { };
Shea
A: 

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.

Steve Cooper
You're advocating catching a `NullReferenceException`? I'm going to have to *strongly* disagree. Your version throws a first-chance `NullReferenceException` **every time** the delegate is invoked if nobody has subscribed to it, and 90% of control events are never subscribed to! FYI, the Microsoft version you posted uses the `EventHandlerList` which is already synchronized, so that's why it doesn't need an explicit lock (the event properties themselves do not use a special lock).
Aaronaught
Uh, no, I'm only suggesting a NullReferenceException if the handler becomes null _after the null check_. Not for every invocation. The point was that taking a copy of the delegate ensures that you call back to objects which have explicity asked not to be informed, which is dangerous.
Steve Cooper
+1  A: 

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

Mark Brackett
Thanks for clarifying. I suspected that the lock in the extension method might be redundant, but wasn't quite sure. What I've actually done now is shuffled the method around a bit to accept the *sender* as `this` and the handler as a `ref` parameter, as Aaronaught suggested. Not quite as intuitive, but it seems like it should work. I've updated my question with the code I'm using.
Will Vousden
A: 

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.

Lasse V. Karlsen