Event handlers not thread safe? [duplicate]

Question

So i've read around that instead of calling a event directly with

if (SomeEvent != null)
   SomeEvent(this, null);

i should be doing

SomeEventHandler temp = SomeEvent;
if (temp != null)
    temp(this, null);

Why is this so? How does the second version become thread safe? What is the best practice?

Ravish

Reading over the tentative, qualified answers here I get the sense that event handling in C# is tightly-coupled, error-prone, and not understood very well. – Hortense

Answer 1

Events are really syntactic sugar over a list of delegates. When you invoke the event, this is really iterating over that list and invoking each delegate with the parameters you have passed.

The problem with threads is that they could be adding or removing items from this collection by subscribing/unsubscribing. If they do this while you are iterating the collection this will cause problems (I think an exception is thrown)

The intent is to copy the list before iterating it, so you are protected against changes to the list.

Note: It is however now possible for your listener to be invoked even after you unsubscribed, so you should make sure you handle this in your listener code.

Stocking

Actually, it will not cause a problem while you iterate over the collection. The delegates in play here are immutable. The only problem is the split second before checking if anyone has subscribed and actually calling the event handlers. Once you've started executing those, no background changes will affect the current invocation. – Luigi

Adding to Lasse V. Karlsen's comment, you can eliminate that last piece of race condition by using SomeEvent?.Invoke(...) instead of checking for null before the invokation. – Anabel

Answer 2

IMO, the other answers miss one key detail - that delegates (and therefore events) are immutable. The significance of this is that subscribing or unsubscribing an event handler doesn't simply append/remove to a list - rather, it replaces the list with a new one with an extra (or one less) item on it.

Since references are atomic, this means that at the point you do:

var handler = SomeEvent;

you now have a rigid instance that cannot change, even if in the next picosecond another thread unsubscribes (causing the actual event field to become null).

So you test for null and invoke it, and all is well. Note of course that there is still the confusing scenario of the event being raised on an object that thinks it unsubscribed a picosecond ago!

Pisci

That addresses one concern, but is the replacement itself done safely in such a way to guarantee that all requested operations happen? I.e., if two threads try to subscribe distinct delegates at the same time, is it guaranteed that both will be subscribed in the end or is it possible for one of those subscriptions to silently fail? – Michael

@Michael yes; it uses an interlocked exchange loop (modern compiler), or a synchronized (lock) region (older compilers) – Pisci

Answer 3

Best practice is the second form. The reason is that another thread might null or alter SomeEvent between the 'if' test and the invocation.

Linsang

Why can't that happen in the second statement? – Ravish

The read is of SomeEvent is atomic, i.e. it happens all the way or nothing. Thus temp cannot be modified outside that thread due to being a local. – Inter

Answer 4

Here is a good write up about .NET events and race conditions with threads. It covers some common scenarios and has some good references in it.

Hope this helps.

Frank