tags:

views:

113

answers:

7

Why event needs to have at least one handler?

I created custom event for my Control and somewhere inside of code of my control, I call this event:

this.MyCustomEvent(this, someArgs);

it throws a NullReferenceException if there is no handler subscribed to it.

When I added a single handler in control's constructor, everything works fine:

this.MyCustomEvent += myCutomEventHandler;

void myCustomEventHandler(object sender, EventArgs e)
{ /* do nothing */ }

Is this normal or maybe I'm doing something wrong? Shouldn't it check automatically if there are any handlers subscribed? It's a little dumb, IMHO.

+1  A: 

This is the normal behavior. To avoid this always test if there's an event subscriber before calling it:

if (MyCustomEvent != null)
{
    MyCustomEvent(this, someArgs);
}
Darin Dimitrov
That's not the right way.
Steven Sudit
@Steven: To be fair, it works well enough that I've never encountered the race condition it leaves open. And, contrary to your own answer, there is no "official right way" - it's just the accepted standard, and just leaves you with a different (though even rarer) race condition.
Dan Puzey
Yes, all race conditions "work well enough" until they fail. That's why we make them impossible. See my response to Henk about the altenrative *not* being a race condition.
Steven Sudit
If you know your app is single-threaded, why bother with fancy alternatives? Use the right method for the job.
siride
@siride: My crystal ball doesn't work well enough to confirm that this class will *never* be used in a multithreaded application.
Steven Sudit
A: 

Yes, you need to check for null. In fact, there's a right way to do it:

var myEvent = MyCustomEvent;
if (myEvent != null)
    myEvent(this, someArgs);

This is the official right way because it avoids a possible race condition when the event is changed after the null check but before the call.

Steven Sudit
Actually, it replaces 1 race-condition with another... But you are right that by consensus this is the best way.
Henk Holterman
I don't consider it a race condition if the event is called "after" it is removed because it is identical to the event having been triggered just before the removal. On the other hand, throwing an exception at random is definitely a race condition.
Steven Sudit
For completeness, I should mention that, if the event is implemented as a property, it becomes feasible to put an actual lock around changes, and then use that same lock when invoking. This is not a particularly common situation, though.
Steven Sudit
@Steven, regarding 'I don't consider it a race condition':No, that's not correct. delegates are immutable reference types. The copy delegate pattern, therefore, does have a race condition.
Ani
@Ani: It's a simple fact that I do not consider it a race condition. Furthermore, I'll once again explain why: because there's no guarantee that, between starting the call to remove your delegate and getting to the next instruction, your delegate will not be invoked. As such, the "race condition" has no effect.
Steven Sudit
Se http://stackoverflow.com/questions/786383/c-events-and-thread-safety for more info
nos
@nos: Thanks for finding a link to some authoritative sources. I believe that my answer here stands up even after reviewing the link.
Steven Sudit
@Steven: I take your point..
Ani
@Ani: Please take a look at Lippert's blog entry. I think it settles the issue quite conclusively.
Steven Sudit
@abat: Thanks for the fix. That's what I get for not compiling my sample!
Steven Sudit
At this point, there's a downvote that I'd like to understand the reason for.
Steven Sudit
+2  A: 

An event at the bottom is MulticastDelegate, which is null if there is no method in the invocation list. Usually, you use a RaiseEvent() method to call an event, the pattern looks like the following:

public void RaiseEvent()
{
  var handler = MyEvent;
  if(handler != null)
    handler(this, new EventArgs());
}

As you assign the event to a variable, it is threadsafe. You can miss a removed or added method though, which was added between the atomic operations (assignment -> null check -> invocation).

Femaref
I follow the same pattern, but it's not truly 'thread safe', as handlers that have unsubscribed between taking a copy and invoking the handlers can still receive notification.
Mark Simpson
@Mark: Read the Lippert blog on this: it's not a problem.
Steven Sudit
I've read that blog post before -- I still don't like the term "thread safe" because the second issue still exists (it may or not be a problem depending on the context).
Mark Simpson
@Mark: It's safe in that it doesn't cause a crash. As for whether getting a event notification after removing your handler, this is not an error.
Steven Sudit
+4  A: 

Note that delegates are reference types, and their default value is null.

The solution proposed by others, i.e. to check for null before firing the event, is not thread-safe because listeners may unsubscribe from the event between the null check and the firing of the event. I have seen solutions that involve copying the delegate to a local variable, and checking it for null before firing, such as

EventHandler myCustomEventCopy = MyCustomEvent;

if (myCustomEventCopy != null)
{
    myCustomEventCopy (this, someArgs);
}

But this has a race-condition, i.e. handlers may fire even after they have unsubscribed from the event, which may corrupt the state of the application.

One solution that handles both problems is to initialize events to a blank handler, e.g.

public event EventHandler MyCustomEvent = delegate { };

and then fire them off without any checks, e.g.

MyCustomEvent(this, someArgs);

Edit: As others have pointed out, this is a complex issue.

http://blogs.msdn.com/b/ericlippert/archive/2009/04/29/events-and-races.aspx

Lippert points out that completely removing the "handler is fired after deregistration" problem requires that the handlers themselves are written in a robust manner.

Ani
So in fact, the think that I did (subscribetd a handler that does nothing) is the safest solution?
Gacek
To my knowledge, yes! The blank delegate pattern is less painful to type though, although its essentially the same thing..
Ani
A blank handler is definitely a plausible alternative to a null check, but I'm not wild about it. For one thing, it makes it very hard to tell whether anyone is hooked into an event.
Steven Sudit
Not really, I think this would work.bool DoesMyCustomEventHaveHandlersOtherThanTheBlankOneIAdded(){return MyCustomEvent.GetInvocationList().Length > 1;}
Ani
Read what I wrote: "makes it very hard". Not impossible, just very hard. I believe your code sample confirms my point.
Steven Sudit
@Steven Sudit, If you're inspecting who is listening to your events, I think you *may* have a problem in your design.
strager
@strager: Do you mean programmatically or in the debugger? I certainly check for null events in the debugger, such as when tracking down leaks caused by the event not being nulled out. I'd hate to have to call `GetInvocationList().Length` from the debugger.
Steven Sudit
This does *not* make it thread-safe.
Hans Passant
@Hans: I believe you are correct. Both this and the local copy solution avoid throwing exceptions, but neither one can guarantee that the event won't be triggered shortly after unsubscribing.
Steven Sudit
@Steven, Hans: Yes, you are correct that this requires correct handling on the subscriber side, as Lippert points out. But not caching the delegate at least prevents the now-unregistered-subscriber from being fired after the unsubscribe call has "gone through." Of course, I understand this is not good enough because when handling race conditions, we need a pattern that offers a guarantee..
Ani
That's not what your answer says, please edit it in.
Hans Passant
Done, thanks to everyone for their views; learnt something new.
Ani
+2  A: 

This is normal behaviour. The conventional pattern for throwing an event in .NET is to have a method called OnMyCustomEvent that you use to throw the event, like so:

    protected void OnMyCustomEvent(MyCustomEventArgs e)
    {
        EventHandler<MyCustomEventArgs> threadSafeCopy = MyCustomEvent;
        if (threadSafeCopy != null)
            threadSafeCopy(this, e);
    }

    public event EventHandler<MyCustomEventArgs> MyCustomEvent;

Then from your code, you would call this.OnMyCustomEvent(someArgs) instead.

Dan Puzey
I used `var` because this is an ideal place for it. The whole point is that your thread-safe copy is of the same type as the event. Using `var` ensures this while protecting you against changes to the event's type and deemphasizing the specifics. Yes, I realize it compiles down to the same thing, but the difference does matter in terms of maintainability.
Steven Sudit
@Steven Sudit, I don't think this is the place for a `var` war.
strager
@strager: Not a war; I upvoted Dan. But it is a fine place for pointing out where `var` is *ideal*, not merely handy or acceptable.
Steven Sudit
@Steven: I have no great feeling either way on the use of `var`. Use whatever your coding standards suggest IMO, as long as you're consistent.
Dan Puzey
@Dan: Well, it's actually my job to *write* the coding standard, so I have to consider what to put in it. Generally, so long as the meaning is either clear or irrelevant, I'm all for `var`.
Steven Sudit
So use it! I never suggested or implied that you shouldn't!
Dan Puzey
+4  A: 

I recommend you to have an extremely useful extension method:

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

which will do the check for you.

Usage:

MyCustomEvent.Raise(this, EventArgs.Empty);
abatishchev
However, if it didn't have a fatal bug, it would be a decent idea. Certainly, an extension method like this would encourage non-crashing code without the overhead of having to manually code a RaiseArgument-style method.
Steven Sudit
@Steven: Sorry but you're wrong. Read Eric Lippert and Jon Skeet carefully ;) Proof me wrong
abatishchev
Ok, I looked at it more carefully, and now I'm convinced that it will work by coincidence. Even though it looks like an instance method (and would fail if it actually were one), it takes a copy of the event handler instead of accessing the member directly, so it can't go null after the check. I will remove my downvote and replace it with an upvote.
Steven Sudit
Or I would if I could. You'll need to edit your answer. I would suggest adding a comment explaining that, because it takes the handler as a parameter, it is thread-safe.
Steven Sudit
@Steven: Anyway considering the thread-safety question, such extension method increases code reuse for any event-handling strategy chosen
abatishchev
@Steven: I will be happy to learn more/something new! Give me know please, I'll edit my post insignificantly and you will be able to upvote it accordingly.
abatishchev
I use the same mechanism and it works very well. Since an extension method is equivalent to SomeStaticType.Method(EventHandler<T>...), a copy is taken. I'm not sure if the "Raise" call might be inlined, though. Taking an extra copy is cheap and explicit, though.
Mark Simpson
@Mark: Well, there's an attribute we can toss on to prevent inlining. Or we could just have a local variable inside the method, which allows us to follow the standard idiom and keep working even if the call is inlined away.
Steven Sudit
@abat: Even before you corrected my misunderstanding of how it incidentally handles the thread safety issue (so long as it's not inlined away), I was on record agreeing that an extension method was a good idea.
Steven Sudit
Ok, my conclusion is that abat is right about the extension method *usually* working, but this is dependent upon inlining, as Mark points out, so it's best to add the local copy. I politely urge abat to modify their answer so that it's always correct.
Steven Sudit
Glad we found a consensus, @Steven! :)
abatishchev
+1  A: 
Richard
This is a good summary, although it doesn't touch on the extension method idea.
Steven Sudit