I didn’t even know about #3 till recently, so time for a quick recap:
Race Between Null Check and Invocation
Since an event with no subscribers appears as a ‘null’, you have to check the event has been wired before you call it, right. Which typically is done like this:
// post an event
if(ThingChanged != null)
ThingChanged(this, args);
This is the wrong way of doing it. In a multi-threaded environment the last subscriber to the event might unsubscribe between the null check and the invocation, causing a null reference exception:
// post an event
if(ThingChanged != null)
// other thread unsubsubscribes here
// next line now causes null ref exception
ThingChanged(this, args);
Despite the MSDN guidance [1], this is a very, very common mistake to make. I found one the first place I looked (a CodePlex project), and also in the ‘overview’ page for the guidance above :-(. And most of the time you get away with it just fine: limited (if any) concurrency and a tendency to wire events 'for life' means it's very unlikely to happen. But as you ramp up the parallelism, and start hooking and unhooking events dynamically during execution, this will eventually bite you.
The easy fix is to cache the delegate locally first:
var handlers = ThingChanged;
if (handlers != null)
handlers(this, args);
(I usually distribute this as a snippet to attempt to make sure people on my team do this automatically, as it’s easy to fall back on bad habits. The snippet also sets this up as a ‘protected virtual OnThingChanged’ method, uses EventHandler<T> and generally tries to encourage correct usage. ReSharper can also generate the correct usage for you)
These days you can ‘wrap up’ the pattern above as an extension method, but it’s not as flexible as actually just creating a member. You don’t have anywhere to put specific pre-event raising logic, and derived classes can’t override OnThingChanged to do their own thing first (something many UI controls and WebForms pages do a lot of).
Finally you can use the field initializer for the event to assign an empty delegate, and prevent the event field from ever being null. This isn’t actually my preference, but it is quite neat:
public event EventHandler<EventArgs<Thing>> ThingChanged = delegate{};
// invoking the event then never needs the null check:
ThingChanged(this, args);I don’t like the idea of a wasted empty delegate call, but I’m just fussy.
Delivery of Event to Stale Subscriber
Unfortunately the pattern above appears to trade one race condition for another, since now the event list that’s invoked is cached (and hence stale). A subscriber can unsubscribe but still subsequently receive an event if the deregistration occurs after the list is cached.
This has been discussed at length on StackOverflow, and on Eric Lippert’s blog, but the salient detail here is that this is unavoidable. The same race occurs if a subscriber unsubscribes during traversal of the event invocation list, but before that subscriber has been notified, or even between taking a reference to an item in the list and invoking it. So even the ‘empty delegate’ version has the same issue.
Eric says:
“event handlers are required to be robust in the face of being called even after the event has been unsubscribed”
…i.e. check your internal state, and act accordingly. In particular, for IDisposable classes, this means that you should not throw an ObjectDisposedException from your event handlers, even if you are disposed. Just don’t do anything.
It is a pity that this requirement is not more widely socialized than just his blog :-(
Race Condition on Event Assignments Within Declaring Class
I had no idea about this until recently when Chris Burrows started updating his blog again, but whilst event assignments are normally ‘thread safe’ (synchronised during the += / –= to avoid races on updating the (immutable) delegate list in-place), referencing the event from within the declaring class doesn’t bind to the event, it binds to the underlying private delegate. And there’s no automatic compiler voodoo synchronisation going on for you when you add and remove things from that.
If you are doing this you must lock on something, and to maintain consistency with the compiler-generated protection for the public event field, you have to lock(this). But again this will only be an issue if multiple threads are (un)subscribing simultaneously anyway, so if your in-class event hook up is in your ctor, before your ‘this’ reference got leaked or you spun off a background worker, you are safe as-is (I think).
For .Net 4 this issue has been fixed: using the += / –= syntax binds to the compiler-generated thread-safe assignment whether you are in the class or not. You can still do unsafe things with the private field if you start explicitly using Delegate.Combine, but that’s just weird anyway.
What’s nice here is the fix was part of removing the locking altogether. Now updates to events occur via a Interlocked.CompareExchange[1] spin, which is a classic no-lock approach:
public void add_Something(EventHandler value)
{
EventHandler handler2;
EventHandler something = this.Something;
do
{
handler2 = something;
EventHandler handler3 = (EventHandler) Delegate.Combine(handler2, value);
something = Interlocked.CompareExchange<eventhandler>(ref this.Something, handler3, handler2);
}
while (something != handler2);
}
This is actually a pretty good pattern to copy if you are targeting very high parallelism, because these atomic compare-and-swap operations are considerably faster than Monitor.Enter (which is what lock() does), so it’s nice to see a good ‘reference’ implementation to crib off (and one that will be pretty ubiquitous too).
Bonus: Robust Event Delivery
Nothing to do with race conditions per-se, but sometimes a subscriber to your event will throw an exception, and by default this will prevent all the subsequent subscribers from receiving the notification. This can be a real swine to diagnose sometimes, especially as the order of event invocation isn’t something you have any control over (strictly speaking it’s non-deterministic, however it always appears to be FIFO in my experience).
Anyway, if you want robust event delivery you should broadcast the event yourself, in a loop, collect the exceptions as you go and raise some kind of MultipleExceptionsException at the end:
protected virtual void OnSomething(EventArgs e)
{
var handlers = Something;
if (handlers != null)
{
var exceptions = new List<Exception>();
foreach (EventHandler handler in handlers.GetInvocationList())
{
try
{
handler(this, e);
}
catch (Exception err)
{
exceptions.Add(err);
}
}
if (exceptions.Count > 0)
throw new MultipleExceptionsException(exceptions);
}
}
At this point the extension method approach beckons because this just blew right out.
Bonus: Lifetime Promotion Via Event Registration
Remember that subscribing to an event is giving someone a reference to you, i.e. an extra root that can prevent garbage collection. You are tying your lifetime to that of the objects that you are listening to.
Typically this isn’t a problem, because the publisher is a more short lived object than the subscriber, but if the publisher sticks around a while (or for ever, if its a static event) it’s very important that subscribers unsubscribe themselves when they are done or they will never get GC’d.
Posts
