8
private void NotifyFreeChannelsChanged() //1.
{
    if (FreeChannelsChanged != null)
    {
        FreeChannelsChanged(this, null);
    }
}

private void NotifyFreeChannelsChanged() //2.
{
    NotifyCollectionChangedEventHandler h = FreeChannelsChanged ;
     if (h != null)
         h(this, e);
}

out of these which is better and why. or it is just a extra check. not a major difference.

Kamlendra Sharma
  • 687
  • 2
  • 10
  • 37
  • http://stackoverflow.com/questions/3668953/raise-event-thread-safely-best-practice?rq=1 –  Feb 22 '13 at 08:27
  • 1
    Option #2 with implicitly typed variable. I want to add that when performance is not an issue, don't care about it. Prefer readability and maintainability. – jay Feb 22 '13 at 08:35

3 Answers3

6

"Better"? Well, doesn't contain a (specific) race condition, one does. MultiCastDelegate types are immutable and use value type semantics in all of the ways that matter (they are reference types however, see this and, more importantly, this), that's why you assign it first and then check. The problem is that:

// this evaluates to true...
if(SomeEvent != null)
{
    // ...but before this line executes, the last 
    // subscriber detached, and now SomeEvent is null. Oops.
    SomeEvent(this, e);
}

You should have asked "why would anyone use example #2?"

As an aside, this is a great place to use an implicitly typed variable (var). Those delegate type names get long...

Also interesting is that a race condition still exists, it's just more subtle. What happens if a subscriber is removed after the assignment? Well, it will still get called, but there's really nothing (I know of) that you can do about it.

Community
  • 1
  • 1
Ed S.
  • 122,712
  • 22
  • 185
  • 265
1

The second one lends itself easily to a set of extension methods that we use, where the EventHandler is the target of the method.

public static void Raise(this EventHandler handler, object sender)
{
  if (handler != null)
    handler(sender, EventArgs.Empty);
}

// And then...
TheEvent.Raise(this);

As far as I know, the fact that you pass the handler to a method gives you that local copy to avoid a race condition.

Carsten
  • 11,287
  • 7
  • 39
  • 62
flq
  • 22,247
  • 8
  • 55
  • 77
0

The second one is to safeguard against a possible threading bug, where a context switch happens between the null check and the event firing in snippet 1. Some other thread can set it to null in between.

So unless your method is prone to multi-threading, you can use the simpler snippet one.

Gishu
  • 134,492
  • 47
  • 225
  • 308
  • I just opt to avoid the race condition all together, regardless of how I expect the code to be called when I write it. It doesn't relly cost anything. – Ed S. Feb 22 '13 at 08:50