3

I often see code like the following, and wonder if there is any reason for using a local variable for the event, instead of just using the event itself. Is there?

var handler = OnQueryComplete;
if (handler != null)
    handler(this, new RepositoryEventArgs<T>(results));
Hamid Pourjam
  • 20,441
  • 9
  • 58
  • 74
ProfK
  • 49,207
  • 121
  • 399
  • 775
  • 1
    possible duplicate of [Raise event thread safely - best practice](http://stackoverflow.com/questions/3668953/raise-event-thread-safely-best-practice) – Hamid Pourjam Feb 22 '15 at 08:24

1 Answers1

10

Yes, absolutely - it makes the nullity check safe.

If you just have:

// Bad code, do not use
if (OnQueryComplete != null)
{
    OnQueryComplete(this, ...);
}

then the last subscriber may unsubscribe between the check and the invocation, causing a NullReferenceException.

There are lots of options here:

  • Decide that you don't care about thread safety, even to the extent of it throwing an exception. (In many cases this may be reasonable, particularly if this is all your own codebase.)
  • Use an extension method to make this simple to achieve so the nullity check goes away anyway.

  • In C# 6, use the conditional null operator:

    OnQueryComplete?.Invoke(this, ...);

  • Set up the event with a single empty handler which is never removed:

    public event FooEventHander OnQueryComplete = delegate {};

You may also want to use Interlocked to make sure you've got the most recent value of the variable, to avoid memory model issues. See my blog post (including comments) for more discussion of this.

Jon Skeet
  • 1,421,763
  • 867
  • 9,128
  • 9,194
  • @RuneFS: Well it changes it slightly. Basically there are always race conditions around this, and there's no perfect solution. – Jon Skeet Feb 22 '15 at 08:34
  • Thank you for such an astute answer Jon. I've just gotten used to `delegate { }` but used your code sample before, blissfully negligent of any thread safety. – ProfK Feb 22 '15 at 09:58
  • Yes yes they'll always be race conditions the original comment was more due to the fact that quite often the "Assign to local" is said to remove the race conditions. It does indeed remove a critical one since the application might blow up otherwise but it then creates another which seems to "always" be forgotten when explaining the "assign to local" approach – Rune FS Feb 24 '15 at 08:25