EventHandler<TEventArgs> thread safety in C#?

Question

Using my cusom EventArgs such as :

public event EventHandler<MyEventArgs> SampleEvent;

from msdn e.g :

public class HasEvent
{
// Declare an event of delegate type EventHandler of 
// MyEventArgs.

    public event EventHandler<MyEventArgs> SampleEvent;

    public void DemoEvent(string val)
    {
    // Copy to a temporary variable to be thread-safe.
        EventHandler<MyEventArgs> temp = SampleEvent;
        if (temp != null)
            temp(this, new MyEventArgs(val));
    }
}

I have 2 questions:

1) looking at the marked code :

I don't see a reason why it should be copied to another param ( regarding threads)

since we have the event keyowrd , no one can touch its invocation list ( no outsider code to the class I mean)

2) If I'm not mistaken, the DemoEvent function should be virtual, so it can be overridden in sub classes... (I'm sure I've seen it somewhere)

the strange thing is that resharper also won't add virtual :

so if I have this code:

it suggests me :

and when I press it :

so again my 2 questions :

1) What is the scenario which this line EventHandler<MyEventArgs> temp = SampleEvent; will solve , regarding thread safty?

2) Shouldn't the function be virtual? ( I'm sure I've seen this pattern with virtual)

Answer 1

what is the scenario which this line EventHandler temp = SampleEvent; will solve , regarding thread safty?

Imagine you do this:

if (SampleEvent != null)
    SampleEvent(this, new MyEventArgs());

If another thread will detach the event handler after the if but before the invocation then you'll try to call a null delegate (and you'll get an exception).

shouldn't the function be virtual ? ( im sure ive seen this pattern with virtual)

Yes, if the class is not sealed then you should mark that function virtual (it's not mandatory but it's a well accepted pattern).

EDIT

Time  Thread 1                                      Thread 2
1                                                   obj.SampleEvent += MyHandler;
2      if (SampleEvent != null)                     
3      {                                            obj.SampleEvent -= MyHandler;
4          SampleEvent(this, new MyEventArgs());
5      }

In this case at time 4 you'll call a null delegate and it'll throw a NullReferenceException. Now look this code:

Time  Thread 1                                      Thread 2
1                                                   obj.SampleEvent += MyHandler;
2      var sampleEvent = SampleEvent;
3      if (sampleEvent != null)                     
4      {                                            obj.SampleEvent -= MyHandler;
5          sampleEvent(this, new MyEventArgs());
6      }

Now at time 5 you call sampleEvent and it holds the old content of SampleEvent, in this case it won't throw any exception (but it'll call MyHandler even if it has been removed by the second thread).

Answer 2

 if (SampleEvent != null)
    SampleEvent(this, new MyEventArgs(val));

This is a classic threading race. Another thread could unsubscribe an event handler while this code runs. Which makes the if() statement conclude that there's a subscriber but the event call fail with a NullReferenceException. Copying the delegate object into a local variable ensures that client code changing the delegate object reference by unsubscribing an event handler won't cause a crash. Still a problem, you'll call the event handler after it was unsubscribed, but that's an inevitable race and not necessarily fatal like the NRE and can be dealt with by the event handler, unlike the NRE.

Yes, a method like this is usually made protected virtual and named OnSampleEvent() so a derived class can alter the event raising behavior.