This is the second and final part of this series. Will focus now a bit on what happen when we add multithreading to our work with events.
Cleaning Up Events and Context Information
In the previous post I mentioned some issues using lambdas and anonymous methods on events. The unsubscribe problem had a tricky "solution" by declaring a "cleanup" method inside the class that holds the event.
public void CleanupSubscribers()
{
SayHelloCompleted = null;
}
The problem here is obvious since the cleanup method will remove all subscribers from the event. Hence, individually removing listeners from the event gets tricky when using lambdas/anonymous methods. So in this case the recommendations would be:
- Do not use lambdas if you need to unsubscribe.
- If you do so, use the cleanup method UNLESS your code is multithreaded.
NOTE: this only applies to the use of lambda expressions and anonymous methods, as for regular named methods you can always remove them easily.
Race condition, late invocation and more...
Now let's keep with multithreading, because it gets really messy. If we take a look at the previous event invocator OnSayHelloCompleted we will notice the code might seem redundant, right?
public void OnSayHelloCompleted(HelloEventArgs e)
{
var handler = SayHelloCompleted;
if (handler != null)
handler(this, e);
}
Questions: why hold a reference to the event, then check for null? why not check it for null directly? The reason behind that is that multicast delegates are immutable; so adding/removing subscribers actually creates a new copy. That being said, in line 3, we hold a reference to the list of subscribers into the variable handler, so no matter if SayHelloCompleted event is modified, the local copy will keep the same value through lines 4 and 5.
So we do that to make it "thread-safe", and it worth noticing that both Visual Studio and Resharper pre-populates the invokers this way.
Problem: We can invoke a listener AFTER the the listener itself was removed form the list. WHAT?! yes,.. just like that. We remove the race condition but we get a kind of "unwanted" call problem.
Some people think they "optimize" the code by doing this:
This is a clear case of a race condition and you should avoid this.
Since the event is never accessed from the outside of the class directly, it will never be null.
Controversy: I have read many comments about people complaining that this will cause a "performance" issue, it does not. There is a performance overhead of course, but is minimal, you can do the tests and see for yourself.
Now, don't you think something like this should already be in C# 5.0? I think so...we already have async and await and all that, but events are a bit behind.(IMO)
So we do that to make it "thread-safe", and it worth noticing that both Visual Studio and Resharper pre-populates the invokers this way.
Problem: We can invoke a listener AFTER the the listener itself was removed form the list. WHAT?! yes,.. just like that. We remove the race condition but we get a kind of "unwanted" call problem.
The Bad Optimization
//WRONG WAY
public void OnSayHelloCompleted(HelloEventArgs e)
{
if (SayHelloCompleted!= null) //is not null at this time
SayHelloCompleted(this, e); //it can be null if another thread modifies the event
}
This is a clear case of a race condition and you should avoid this.
Alternative...
A suitable alternative could be to always instantiate the event with an empty delegate. Thus, removing the need for a null check.
//instantiate with empty delegate
private event SayHello SayHelloCompleted = delegate { };
public void OnSayHelloCompleted(HelloEventArgs e)
{
//no need for a null-check
SayHelloCompleted(this, e);
}
Since the event is never accessed from the outside of the class directly, it will never be null.
Controversy: I have read many comments about people complaining that this will cause a "performance" issue, it does not. There is a performance overhead of course, but is minimal, you can do the tests and see for yourself.
Now, don't you think something like this should already be in C# 5.0? I think so...we already have async and await and all that, but events are a bit behind.(IMO)
