Problem
UPDATE
The answer to this question in C# 6 is:
SomeEvent?.Invoke(this, e);
The following advice is something I commonly hear/read:
Before you check for null and fire an event, always make a copy of it. This eliminates a potential threading problem if the event gets null in between where you check for null and where you fire the event:
// Copy the event delegate before checking/calling
EventHandler copy = TheEvent;
if (copy != null)
copy(this, EventArgs.Empty); // Call any handlers on the copied list
Update: After reading about optimizations, I assumed that the event member would have to be volatile as well, however Jon Skeet notes in his response that the CLR does not optimize away the copy.
But, in the meantime, another thread must have done something similar to cause this problem:
// Better delist from event - don't want our handler called from now on:
otherObject.TheEvent -= OnTheEvent;
// Good, now we can be certain that OnTheEvent will not run...
The real sequence could be something like this:
// Copy the event delegate before checking/calling
EventHandler copy = TheEvent;
// Better delist from event - don't want our handler called from now on:
otherObject.TheEvent -= OnTheEvent;
// Good, now we can be certain that OnTheEvent will not run...
if (copy != null)
copy(this, EventArgs.Empty); // Call any handlers on the copied list
The point is that OnTheEvent runs after the author has unsubscribed, even if they deliberately unsubscribed to avoid this. Surely, a custom event implementation with proper synchronization in the add and remove accessors is required. Furthermore, if a lock is held while an event is fired, there is the risk of a deadlock.
Is this programming from the Cargo Cult? It appears that way – many people must be taking this precaution to shield their code from many threads, but events, in my opinion, require far more consideration before they can be utilized in a multi-threaded design. As a result, those who aren’t taking extra precautions should ignore this advice; it simply isn’t an issue for single-threaded applications, and given the lack of volatile in most online sample code, it’s possible that the advice isn’t having any effect at all.
(And isn’t it much easier to simply assign the empty delegate on the member declaration, eliminating the need to check for null in the first place?)
Updated: In case it wasn’t clear, I understood the advice’s objective – to prevent a null reference exception at all costs. My argument is that this null reference exception can only happen if another thread delists from the event, and the only reason for doing so is to assure that no more calls are received via that event, which this technique plainly does not provide. You’d be hiding a racial condition; it’s better to be open about it! This null exception aids in the detection of component abuse. If you wish to safeguard your component from exploitation, you may follow WPF’s lead and save the thread ID in your constructor, then throw an exception if it’s used.
As a result, I believe that simply using the copy/check idiom is cargo cult programming, as it adds clutter and noise to your code. It takes a lot more effort to actually guard against other threads.
In response to Eric Lippert’s blog entries, here’s an update:
So there’s a key point regarding event handlers that I’d overlooked: “event handlers are supposed to be robust in the face of being called even after the event has been unsubscribed,” and we only need to be concerned about the event delegate being null. Is there any documentation on the requirement for event handlers?
And so: “There are other ways to solve this problem; for example, initializing the handler to have an empty action that is never removed. The typical approach, however, is to perform a null check.”
So the last piece of my puzzle is why is explicit-null-check considered the “standard pattern”? Assigning the empty delegate, on the other hand, takes only the addition of = delegate to the event declaration, and thus eliminates those little mounds of stinking ceremony from every location where the event is raised. It would be simple to ensure that the empty delegate is inexpensive to create. Is there anything more I’m missing?
Surely this is just.NET 1.x advice that hasn’t died off, as Jon Skeet said, as it should have done in 2005?
Asked by Daniel Earwicker
Solution #1
Because of the condition, the JIT is unable to perform the optimization you mentioned in the first part. I know this has been raised as a spectre before, but it isn’t true. (I checked with Joe Duffy or Vance Morrison a while back; I’m not sure which.)
It’s possible that the local copy taken without the volatile modifier will be out of date, but that’s it. There will be no NullReferenceException.
And, yes, there is a race condition – but there will always be one. Let’s say we update the code to:
TheEvent(this, EventArgs.Empty);
Assume that the delegate’s invocation list contains 1000 entries. It’s entirely feasible that the action at the beginning of the list will have completed before another thread unsubscribes a handler near the end. However, because there will be a new list, that handler will still be performed. (Delegates cannot be changed.) As far as I can see this is unavoidable.
Although using an empty delegate eliminates the nullity check, it does not resolve the race issue. It also doesn’t guarantee that you’ll always “see” the variable’s most recent value.
Answered by Jon Skeet
Solution #2
I’ve seen that a lot of individuals are opting for the extension technique…
public static class Extensions
{
public static void Raise<T>(this EventHandler<T> handler,
object sender, T args) where T : EventArgs
{
if (handler != null) handler(sender, args);
}
}
This provides a friendlier syntax for raising the event…
MyEvent.Raise( this, new MyEventArgs() );
In addition, because the local copy is recorded at method call time, it is no longer necessary.
Answered by JP Alioto
Solution #3
I believe this is due to the fact that the null-check is more performant.
There will be certain overheads if you always subscribe an empty delegate to your events as they are created:
(It’s worth noting that UI controls frequently include a lot of events, the majority of which are never subscribed to.) Creating a dummy subscriber for each event and then invoking it would very certainly result in a large performance hit.)
I did some quick performance testing to check how the subscribe-empty-delegate technique affected performance, and here are the results:
Executing 50000000 iterations . . .
OnNonThreadSafeEvent took: 432ms
OnClassicNullCheckedEvent took: 490ms
OnPreInitializedEvent took: 614ms <--
Subscribing an empty delegate to each event . . .
Executing 50000000 iterations . . .
OnNonThreadSafeEvent took: 674ms
OnClassicNullCheckedEvent took: 674ms
OnPreInitializedEvent took: 2041ms <--
Subscribing another empty delegate to each event . . .
Executing 50000000 iterations . . .
OnNonThreadSafeEvent took: 2011ms
OnClassicNullCheckedEvent took: 2061ms
OnPreInitializedEvent took: 2246ms <--
Done
Note that the event pre-initialized with an empty delegate is noticeably slower in the case of zero or one subscribers (typical for UI controllers, where events are frequent) (over 50 million iterations…)
Visit this blog post on.NET Event invocation thread safety, which I published just the day before this question was posed (!) for more information and source code.
(My test setup could be incorrect, so get the source code and investigate it for yourself.) Any and all input is much welcomed.)
Answered by Daniel Fortunov
Solution #4
This was a fantastic read – not! Despite the fact that I require it in order to use the C# functionality known as events!
Why don’t you just fix it in the compiler? Please don’t flame this because I know there are MS folks who read these posts!
1 – The Null Problem) Why not make events.Empty instead of null from the start? How many lines of code would be saved by not having to do a null check or adding a = delegate to the declaration? Allow the compiler to deal with the Empty case; in other words, do nothing! If the event creator cares about it, they can look for.Empty and do anything they want with it! Otherwise, all the null checks and delegate adds are just workarounds!
To be honest, I’m sick of doing this with every event – aka boilerplate code!
public event Action<thisClass, string> Some;
protected virtual void DoSomeEvent(string someValue)
{
var e = Some; // avoid race condition here!
if(null != e) // avoid null condition here!
e(this, someValue);
}
2 – the issue of the race situation) I read Eric’s blog post, and while I agree that the H (handler) should manage when it dereferences itself, can’t the event be made immutable and thread-safe? Set a lock flag on its construction, for example, so that whenever it is called, it locks all subscribing and unsubscribing to it while it is running?
Conclusion,
Isn’t it true that modern languages are designed to help us tackle challenges like these?
Answered by Chuck Savage
Solution #5
The new?. operator in C# 6 and higher can be used to simplify code, as in:
TheEvent?.Invoke(this, EventArgs.Empty);
The MSDN documentation may be found here.
Answered by Phil1970
Post is based on https://stackoverflow.com/questions/786383/c-sharp-events-and-thread-safety