<< Who wrote this code?! | Home | The fallacy of IRepository >>

Correct event handling

This code has a very subtle bug:

200811152339.jpg

What is the problem with this code? It is an obvious try at bridging an annoying design issue in C#, the fact that invoking an event with no subscribers throws a null reference exception. It should have been a no op, but that is another issue.

The copying to a separate variable is also common, because this allow you to avoid subtle mutli threading bug. The problem is this code it that it contains a bug.

The nullability check should have been made on the handler local variable, like this:

200811152344.jpg

As the code stands, it has a very subtle bug, that will only appear in multi threaded scenarios, and even then, not always. The fixed code handle the issue correctly. But on first glance, they both seem to be doing the same thing.

Print | posted on Sunday, November 16, 2008 12:03 AM

Feedback

Gravatar

# re: Correct event handling 11/16/2008 12:13 AM Rik Hemsley

Just to note that R# can generate this code for you, which is handy as it's something I have to write many times a day.

Gravatar

# re: Correct event handling 11/16/2008 12:40 AM Peter Morris

Stood out like a sore thumb :-)

Gravatar

# re: Correct event handling 11/16/2008 12:43 AM Thomas Krause

That's an easy one. If the other thread unsubscribes from the event after the condition has been checked, but before you assign the variable and there are no other subscribers, you'll assign a null reference and get a NullReferenceException.

Gravatar

# re: Correct event handling 11/16/2008 1:15 AM Andrew Davey

I use a macro in Nemerle to generate this, so all I have to type is:

raiseevent Completed(this)

:)

Gravatar

# re: Correct event handling 11/16/2008 1:17 AM Russ

There's still a threading issue, because an even is a multicast delegate. If there are two subscribers, the second could unsubscribe in another thread from Completed whilst handler is invoking the first; in which case handler will suffer an ObjectDisposedException. I'm not aware of any way to avoid it.

Gravatar

# re: Correct event handling 11/16/2008 1:25 AM Phil Murphy

I hate to have to say it, but this seems to be one of those cases where VB.NET's behaviour is better than C# - apparently "RaiseEvent" will do the multi-threaded null checking for you.

Apparently. I wouldn't use VB.NET myself, you understand.

Gravatar

# re: Correct event handling 11/16/2008 1:35 AM john

I was told you should lock the value during the copy since it could change at that point. Sorry I forgot where I read it.

static readonly object lockObject = new object();
lock(lockObject)
{
handler = Completed;
}

if(hander != null)
{
handler(this);
}

Gravatar

# re: Correct event handling 11/16/2008 2:57 AM Mark Nijhof

Ayende,

Would you keep both null checks or just the later one?

John,

Should the lock not be around the whole code block to be effective? And also around any subscribing and un-subscribing code blocks? because you are not locking the variables inside the lock, you are locking that part of the code, if I understand it correctly? Please correct me if I am wrong.

-Mark

Gravatar

# re: Correct event handling 11/16/2008 3:13 AM Judah

Oren,

You're right, an event with no subscribers should not throw when we attempt to raise the event. Poor design choice by the .NET guys.

One workaround is a simple extension method:

stackoverflow.com/.../evil-use-of-extension-met...

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

(And an overload for generic EventHandlers, and you're all set.)

Gravatar

# re: Correct event handling 11/16/2008 5:23 AM alwin

Shouldn't you use [MethodImpl(MethodImplOptions.NoInlining)] for your Raise() method? Else its possible that it isn't thread safe.

When inlining, the local variable disappears and you could get sort of
if (Completed != null){
Completed(this);
}
Not what you want...

(Read that somewhere long ago, remembered it, googled and found http://blog.quantumbitdesigns.com/tag/events/ )

Gravatar

# re: Correct event handling 11/16/2008 6:49 AM Simon Gillbee

There's another solution that both makes the code simpler and solves the multi-threaded issue:

public Action Completed = delegate {};

void OnCompleted ()
{
Completed (this);
}

This solution works nicely in that Completed can *never* be null, and since we don't have to juggle the null-check, we don't need to worry about the multi-threaded implications (except about the subscription during invocation mentioned in an earlier comment).

There is some minor runtime overhead with this pattern, but personally I think the trade-off is worth it.

Gravatar

# re: Correct event handling 11/16/2008 8:20 AM Ayende Rahien

john,
Event handlers are immutables, so you don't need to look them.
The object that you get is always in consistent state.`

Gravatar

# re: Correct event handling 11/16/2008 10:09 AM Krzysztof Kozmic

You may also want to read this series of blogposts on the very same topic:
blogs.msdn.com/.../92787.aspx
blogs.msdn.com/.../158636.aspx
blogs.msdn.com/.../230681.aspx

Gravatar

# re: Correct event handling 11/16/2008 1:27 PM RafalG

Ayende, I enjoyed your posts about boo, brail, rhinos and other interesting, sophisticated, nontrivial subjects that encouraged discussion. But lately you seem to have switched to less demanding mode - short, unrelated, frequent and unsignificant stories in fire & forget manner. Maybe that's because you're very busy doing real projects, but I had to point it out. Remember, quality * quantity = const, even if the const is very big.


Gravatar

# re: Correct event handling 11/16/2008 8:54 PM Ayende Rahien

Rafal,
I write about what I do.

Gravatar

# re: Correct event handling 11/16/2008 9:47 PM Liviu

Nobody has to write that stuff. Use postsharp for god's sake...

Gravatar

# re: Correct event handling 11/17/2008 4:16 AM Andrés G. Aragoneses

The best thing to do about this is having a tool (a post-compiler) like Gendarme or FxCop that warns you about it.

For example:
www.mono-project.com/Gendarme.Rules.Concurrency

Gravatar

# re: Correct event handling 11/17/2008 4:30 AM Bart Czernicki

I use the anonymous method trick I saw on DotnetKicks several months ago also its in Skeet's C# 3.0...its the same what Simon posted.
handler = delegate {};

Just to add to this, this handler cannot be removed in C# 3.0. Not sure if 4.0 changes this, but you are guaranteed that there will always be one subscriber.

Gravatar

# re: Correct event handling 11/17/2008 12:13 PM Cherian

Bart,
I used to use the anonymous delegate pattern.This one is a bit tricky though. See stackoverflow.com/.../is-there-a-downside-to-ad...

Gravatar

# re: Correct event handling 11/20/2008 6:43 PM jOHN

Ayende,

If you lock when sending the handler than you could get a deadlock.

I am only trying to lock the value so it is not in an inconsitant state. I am unable to find the place where I was told this in the first place.

If I recall what I was told orginally. You lock the varible and what you are coping from so the new value is in a know state. Then if the handle is released after the copy/lock the original value is still valid.

If you attempt to use "handler(...)" during the call the handler could be release which would generate a error.

Regards,
John

Title  
Name  
Email
Url
Comments   
Please add 4 and 4 and type the answer here: