Correct event handling
This code has a very subtle bug:
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:
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.
Comments
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.
Stood out like a sore thumb :-)
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.
I use a macro in Nemerle to generate this, so all I have to type is:
raiseevent Completed(this)
:)
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.
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.
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);
}
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
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)
{
}
}
(And an overload for generic EventHandlers, and you're all set.)
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){
}
Not what you want...
(Read that somewhere long ago, remembered it, googled and found http://blog.quantumbitdesigns.com/tag/events/ )
There's another solution that both makes the code simpler and solves the multi-threaded issue:
public Action <iapplicationlifecycle 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.
john,
Event handlers are immutables, so you don't need to look them.
The object that you get is always in consistent state.`
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
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.
Rafal,
I write about what I do.
Nobody has to write that stuff. Use postsharp for god's sake...
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
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.
Bart,
stackoverflow.com/.../is-there-a-downside-to-ad...
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