Is Extension method thread safe?

Question

Is this Extension method thread safe?

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

or do I need to change it to this?

   public static class Extensions
   {
      public static void Raise<T>(this EventHandler<T> handler,
        object sender, T args) where T : EventArgs
      {
         var h = handler;
         if (h!= null) h(sender, args);
      }
   }

design & implementation details of events with respect to threading http://stackoverflow.com/questions/786383/c-events-and-thread-safety — user44298, Jan 06 '11 at 19:33

score 9 · Answer 1 · answered Jan 06 '11 at 19:56

9

You found an interesting loop hole, it tripped everybody up. No, it is not thread-safe.

While it looks like the EventHandler<> reference is copied through the method argument, this is not what happens at runtime. Extension methods are subject to being inlined, just like a regular instance method. In fact, it is extremely likely to get inlined since it is so small. There's no copy, you have to make one yourself.

answered Jan 06 '11 at 19:56

Hans Passant

922,412
146
1,693
2,536

1

I'm not positive you are correct. This would only apply when the calling function was calling the method on a value which would be subject to be modified by another thread (a class field for instance instead of a local variable). I don't know if the JIT would inline such a call in that case. – tster Jan 06 '11 at 20:04
However, this is a very interesting topic. I wish I wasn't at work and had the time to dig in to this more. – tster Jan 06 '11 at 20:06
1

Well, that's what the null test in event raising code is all about. – Hans Passant Jan 06 '11 at 20:06
@Hans Passant, Yes I understand. I'm saying I bet the JIT will not inline the method because of that. – tster Jan 06 '11 at 20:10
It's just a plain delegate object, it isn't volatile. – Hans Passant Jan 06 '11 at 20:22
@Hans, Would adding MethodImpl.NoInlining help? – tster Jan 06 '11 at 20:25
Yes. But that's *more* expensive than just making the copy. – Hans Passant Jan 06 '11 at 20:42
What is the solution then? Wrapping each extension method's content with lock? – Jacob Feb 09 '17 at 23:01
Make the copy, it is a very well-known C# requirement. Second snippet. – Hans Passant Feb 09 '17 at 23:15

score 7 · Answer 2 · answered Jan 07 '11 at 00:43

Neither version is threadsafe, depending on what you mean by "threadsafe". Consider your second version:

   var h = handler;          
   if (h!= null) 
       h(sender, args);

"handler" is a copy of some field that has an immutable delegate in it. Suppose that field is mutated to "null" on another thread after the null check. Your code does not crash in that case, because you've made a copy of the original not-null value. But merely not crashing does not make the program thread safe. A program that doesn't crash but still produces the wrong results is still not threadsafe.

Suppose when the other thread set the event field to null, it also mutated some state that the previous contents needed to run correctly. You are now going to run an event handler that depends on state that was just mutated on another thread; you're running a stale event handler.

There is no easy way to protect against this problem; if that's the situation you're in, then you're going to have to design your threading logic extremely carefully in order to deal with the situation.

Is Extension method thread safe?

2 Answers2

Linked