C# Events and Thread Safety

Question

The JIT isn't allowed to perform the optimization you're talking about in the first part, because of the condition. I know this was raised as a spectre a while ago, but it's not valid. (I checked it with either Joe Duffy or Vance Morrison a while ago; I can't remember which.)

Without the volatile modifier it's possible that the local copy taken will be out of date, but that's all. It won't cause a NullReferenceException.

And yes, there's certainly a race condition - but there always will be. Suppose we just change the code to:

TheEvent(this, EventArgs.Empty);

Now suppose that the invocation list for that delegate has 1000 entries. It's perfectly possible that the action at the start of the list will have executed before another thread unsubscribes a handler near the end of the list. However, that handler will still be executed because it'll be a new list. (Delegates are immutable.) As far as I can see this is unavoidable.

Using an empty delegate certainly avoids the nullity check, but doesn't fix the race condition. It also doesn't guarantee that you always "see" the latest value of the variable.

I see a lot of people going toward the extension method of doing this ...

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);   
  }   
}

That gives you nicer syntax to raise the event ...

MyEvent.Raise( this, new MyEventArgs() );

And also does away with the local copy since it is captured at method call time.

"Why is explicit-null-check the 'standard pattern'?"

I suspect the reason for this might be that the null-check is more performant.

If you always subscribe an empty delegate to your events when they are created, there will be some overheads:

• Cost of constructing the empty delegate.
• Cost of constructing a delegate chain to contain it.
• Cost of invoking the pointless delegate every single time the event is raised.

(Note that UI controls often have a large number of events, most of which are never subscribed to. Having to create a dummy subscriber to each event and then invoke it would likely be a significant performance hit.)

I did some cursory performance testing to see the impact of the subscribe-empty-delegate approach, and here are my 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 for the case of zero or one subscribers (common for UI controls, where events are plentiful), the event pre-initialised with an empty delegate is notably slower (over 50 million iterations...)

For more information and source code, visit this blog post on .NET Event invocation thread safety that I published just the day before this question was asked (!)

(My test set-up may be flawed so feel free to download the source code and inspect it yourself. Any feedback is much appreciated.)

I truly enjoyed this read - not! Even though I need it to work with the C# feature called events!

Why not fix this in the compiler? I know there are MS people who read these posts, so please don't flame this!

1 - the Null issue) Why not make events be .Empty instead of null in the first place? How many lines of code would be saved for null check or having to stick a = delegate {} onto the declaration? Let the compiler handle the Empty case, IE do nothing! If it all matters to the creator of the event, they can check for .Empty and do whatever they care with it! Otherwise all the null checks / delegate adds are hacks around the problem!

Honestly I'm tired of having to do 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 race condition issue) I read Eric's blog post, I agree that the H (handler) should handle when it dereferences itself, but cannot the event be made immutable/thread safe? IE, set a lock flag on its creation, so that whenever it is called, it locks all subscribing and un-subscribing to it while its executing?

Conclusion,

Are not modern day languages supposed to solve problems like these for us?

With C# 6 and above, code could be simplified using new ?. operator as in:

TheEvent?.Invoke(this, EventArgs.Empty);

Here is the MSDN documentation.