« Events and Threads (Part 2) | Main | Implementing Clone »
May 29, 2008
Events and Threads (Part 3)
We've discussed reasonable mechanisms for subscribing to events and for raising events, but we skirted the issue of "thread-safe" events until now.
What is a thread-safe event? A good definition would be "an event that may be subscribed, unsubscribed, and/or raised simultaneously on arbitrary threads." In that case, what must we do to create a thread-safe event?
Certainly it must be true that if you add an event handler, it is added, and if you remove an event handler, it is removed. As discussed earlier, the default implementation of the add and remove methods accomplishes this by locking the object, but I'd recommend using your own lock:
public event EventHandler Click
{
add
{
lock (m_lockClick)
m_click += value;
}
remove
{
lock (m_lockClick)
m_click -= value;
}
}
EventHandler m_click;
object m_lockClick = new object();
It is also certain that a thread-safe event must not throw a null reference exception when raising the event. The problem is that another thread could remove the last event handler at any moment, which sets the event delegate to null. In the following naïve implementation, Click could become null after the check but before the call:
private void RaiseClick()
{
if (m_click != null)
m_click(this, EventArgs.Empty);
}
The most common solution is to make a copy of the event delegate before calling it:
private void RaiseClick()
{
EventHandler handler = m_click;
if (handler != null)
handler(this, EventArgs.Empty);
}
However, I learned from Juval Lowy's book that aggressive compiler inlining could theoretically eliminate the copy, which would bring us back to the same problem. His solution is to write a non-inlined method that raises the event, something like this:
private void RaiseClick()
{
RaiseEvent(m_click);
}
[MethodImpl(MethodImplOptions.NoInlining)]
private void RaiseEvent(EventHandler handler)
{
if (handler != null)
handler(this, EventArgs.Empty);
}
Another good solution is to add a do-nothing event handler; follow the link for an explanation of that approach.
Of course, the most "correct" solution is probably to use the lock that's already there:
private void RaiseClick()
{
EventHandler handler;
lock (m_lockClick)
handler = m_click;
if (handler != null)
handler(this, EventArgs.Empty);
}
Perhaps the last solution helped you think of another aspect of thread-safe events that isn't discussed very often. A problem common to all of these solutions is that a subscriber's event handler may be called even after it has been unsubscribed!
I found this behavior very surprising when I was writing thread-safe objects with events. For example, the Dispose method of one object might unsubscribe from an event of another object, assuming that the event handler won't be called again; but, in fact, that event handler might actually be called after the object has been disposed, which can obviously cause problems.
If you want to guarantee that an event handler won't be called after it is unsubscribed, as well as guarantee that an event handler can't be unsubscribed until the event is done being raised, the most direct solution is to call the event handler from within the lock:
private void RaiseClick()
{
lock (m_lockClick)
{
if (m_click != null)
m_click(this, EventArgs.Empty);
}
}
This is a bit hair-raising, of course, because you're calling arbitrary code from within a lock, which is a good recipe for deadlock. I don't have enough experience with this pattern to know how common a problem that might be.
One final note about thread-safe events – make sure that your clients understand that their event handler will be invoked on an arbitrary thread, so that they know to dispatch to their UI thread if necessary.
I wish I had more solid conclusions as regards thread-safe events, but I'm still working through these issues. Hopefully I've at least given you some things to think about when you're considering adding events to a thread-safe class – it might be easier to just avoid them altogether.
Posted by Ed Ball at May 29, 2008 2:46 PM
Trackback Pings
TrackBack URL for this entry:
http://blog.logos.com/mt-cgi/mt-tb.cgi/212
Comments
Since we're soundly in the realm of threading, why not fire your events with BeginInvoke? That will allow your last solution to return quickly, and not have direct problems due to bad subscribers. The subscribers will be executed on threads from the .NET thread pool.
You would of course need to send safe copies of the event arguments, but in the world of threading, that is already a prime concern.
Posted by: John Fisher at May 29, 2008 6:12 PM
Raising events asynchronously is a topic I considered discussing, but I didn't feel like I could do it justice, since I don't have any experience with it. Lowy's book (linked above) discusses asynchronous events in chapter 7, where he talks about why BeginInvoke can't generally be invoked directly on an event delegate, though you can work around that problem with GetInvocationList.
Raising events asynchronously doesn't solve the problem of an event handler being called after removing it from the event, because we could call BeginInvoke and release the lock, then the client could unsubscribe, and finally the client's event handler would be executed on the thread pool, after he unsubscribed.
Perhaps the best and easiest solution to that whole problem is simply to document the fact that event handlers must be written to do nothing if they are called after they are unsubscribed.
Posted by: Ed Ball at May 30, 2008 8:20 AM
In one of my library I had to take care of writing Thread Safe code and I had such thing happening.
Finally I found the "best looking" solution was to lock the whole event handler method. I'd like very much to know better idea, my hairs did raise as these methods are calling into arbitrary event handler!! But I saw no better solution...
I'm curious to know any idea you'll come up with! ;-)
Posted by: Lloyd at May 30, 2008 8:34 AM
A side note - I found an overload of your previously mentioned Null-propagating extension method which takes an Action to be useful when firing an event, as it implicitly makes a copy of the event handler. I hadnt considered the compiler inlining the call and Im not sure its appropriate in all cases to add the MethodImpl(MethodImplOptions.NoInlining) attribute to the extension method
Enjoying the series.
Posted by: Scott at May 30, 2008 1:34 PM
Lloyd: Thanks for the comment! If we come up with any better patterns, I'll be sure to post them!
Scott: We have added an Action overload for IfNotNull as well! Using it to raise events is a cool idea. It's hard to imagine the C# compiler optimizing out the delegate, though I suppose it might some day. We actually have special extension methods for EventHandler events:
[MethodImpl(MethodImplOptions.NoInlining)]
public static void Raise(this EventHandler fn, object sender)
{
if (fn != null)
fn(sender, EventArgs.Empty);
}
[MethodImpl(MethodImplOptions.NoInlining)]
public static void Raise<T>(this EventHandler<T> fn, object sender, T e) where T : EventArgs
{
if (fn != null)
fn(sender, e);
}
Posted by: Ed Ball at May 30, 2008 1:42 PM
It seems that the solution to this problem requires implementation of an asynchronous pattern.
1. Start firing events.
2. Lock out any new event firings.
3. Lock the subscribe/unsubscribe capability.
4. BeginInvoke for each subscriber, calling through a wrapper method.
5. Wrapper method calls real delegate.
6. When delegate completes, wrapper method checks to see if all delegates are done.
7. If all delegates are done, release above locks.
8. Done notifying, in a thread-friendly way!
Posted by: John Fisher at June 4, 2008 8:58 AM
Nice series. I've blogged about a parallel in the Java world. We can use a CopyOnWriteArrayList to get around the locking-while-firing issue.
Posted by: Dave Dunkin at June 21, 2008 7:59 AM