views:

1635

answers:

10

I have seen a few mentions of this idiom (including on SO):

// Deliberately empty subscriber
public event EventHandler AskQuestion = delegate {};

The upside is clear - it avoids the need to check for null before raising the event.

However, I am keen to understand if there are any downsides. For example, is it something that is in widespread use and is transparent enough that it won't cause a maintenance headache? Is there any appreciable performance hit of the empty event subscriber call?

+9  A: 

The only downside is a very slight performance penalty as you are calling extra empty delegate. Other than that there is no maintenance penalty or other drawback.

Maurice
A: 

If you are doing it a /lot/, you might want to have a single, static/shared empty delegate that you re-use, simply to reduce the volume of delegate instances. Note that the compiler caches this delegate per event anyway (in a static field), so it is only one delegate instance per event definition, so it isn't a huge saving - but maybe worthwhile.

The per-instance field in each class will still take the same space, of course.

i.e.

internal static class Foo
{
    internal static readonly EventHandler EmptyEvent = delegate { };
}
public class Bar
{
    public event EventHandler SomeEvent = Foo.EmptyEvent;
}

Other than that, it seems fine.

Marc Gravell
+12  A: 

For systems that make heavy use of events and are performance-critical, you will definitely want to at least consider not doing this. The cost for raising an event with an empty delegate is roughly twice that for raising it with a null check first.

Here are some figures running benchmarks on my machine:

For 50000000 iterations . . .
No null check (empty delegate attached): 530ms
With null check (no delegates attached): 249ms
With null check (with delegate attached): 452ms

And here is the code I used to get these figures:

using System;
using System.Diagnostics;

namespace ConsoleApplication1
{
    class Program
    {
     public event EventHandler<EventArgs> EventWithDelegate = delegate { };
     public event EventHandler<EventArgs> EventWithoutDelegate;

     static void Main(string[] args)
     {
      //warm up
      new Program().DoTimings(false);
      //do it for real
      new Program().DoTimings(true);

      Console.WriteLine("Done");
      Console.ReadKey();
     }

     private void DoTimings(bool output)
     {
      const int iterations = 50000000;

      if (output)
      {
       Console.WriteLine("For {0} iterations . . .", iterations);
      }

      //with anonymous delegate attached to avoid null checks
      var stopWatch = Stopwatch.StartNew();

      for (var i = 0; i < iterations; ++i)
      {
       RaiseWithAnonDelegate();
      }

      stopWatch.Stop();

      if (output)
      {
       Console.WriteLine("No null check (empty delegate attached): {0}ms", stopWatch.ElapsedMilliseconds);
      }


      //without any delegates attached (null check required)
      stopWatch = Stopwatch.StartNew();

      for (var i = 0; i < iterations; ++i)
      {
       RaiseWithoutAnonDelegate();
      }

      stopWatch.Stop();

      if (output)
      {
       Console.WriteLine("With null check (no delegates attached): {0}ms", stopWatch.ElapsedMilliseconds);
      }


      //attach delegate
      EventWithoutDelegate += delegate { };


      //with delegate attached (null check still performed)
      stopWatch = Stopwatch.StartNew();

      for (var i = 0; i < iterations; ++i)
      {
       RaiseWithoutAnonDelegate();
      }

      stopWatch.Stop();

      if (output)
      {
       Console.WriteLine("With null check (with delegate attached): {0}ms", stopWatch.ElapsedMilliseconds);
      }
     }

     private void RaiseWithAnonDelegate()
     {
      EventWithDelegate(this, EventArgs.Empty);
     }

     private void RaiseWithoutAnonDelegate()
     {
      var handler = EventWithoutDelegate;

      if (handler != null)
      {
       handler(this, EventArgs.Empty);
      }
     }
    }
}
Kent Boogaart
You're kidding, right? The invocation adds 5 nanoseconds and you're warning against doing it? I can't think of a more unreasonable general optimization than that.
Brad Wilson
Interesting. According to your findings it is faster to check for null and call a delegate than just to call it without the check. Doesn't sound right to me. But anyway this is such a small difference that I don't think it is noticeable in all but the most extreme cases.
Maurice
Brad, I specifically said for performance-critical systems that make heavy use of events. How is that general?
Kent Boogaart
A: 

Isn't one of the good things about using events is that even if nothing handles the event, you can still raise it? If there is no event handler, you should still be able to raise the event, without checking if anything handles it. At least, that's the way it works in VB.Net. You don't have to handle events at all, and if there is nothing to handle an event, you shouldn't experience any bad effects. Perhaps C# handles events differently from VB.Net, but I thought they were very much the same in this respect.

Kibbee
VB.Net does some of the plumbing automatically that C# programs have to do manually. In C#, invoking an event with no handlers causes a null reference exception.
Jeffrey L Whitledge
+2  A: 

It is my understanding that the empty delegate is thread safe, whereas the null check is not.

bennage
+2  A: 

I would say it's a bit of a dangerous construct, because it tempts you to do something like :

MyEvent(this, EventArgs.Empty);

If the client throws an exception, the server goes with it.

So then, maybe you do:

try
{
  MyEvent(this, EventArgs.Empty);
}
catch
{
}

But, if you have multiple subscribers and one subscriber throws an exception, what happens to the other subscribers?

To that end, I've been using some static helper methods that do the null check and swallows any exception from the subscriber side (this is from idesign).

// Usage
EventHelper.Fire(MyEvent, this, EventArgs.Empty);


public static void Fire(EventHandler del, object sender, EventArgs e)
{
    UnsafeFire(del, sender, e);
}
private static void UnsafeFire(Delegate del, params object[] args)
{
    if (del == null)
    {
        return;
    }
    Delegate[] delegates = del.GetInvocationList();

    foreach (Delegate sink in delegates)
    {
        try
        {
            sink.DynamicInvoke(args);
        }
        catch
        { }
    }
}
Scott P
Not to nitpick, but dont you thing <code> if (del == null) { return; } Delegate[] delegates = del.GetInvocationList();</code>is a race condition candidate?
Cherian
Not quite. Since delegates are value types, del is actually a private copy of the delegate chain which is accessible only to the UnsafeFire method body. (Caveat: This fails if UnsafeFire gets inlined, so you'd need to use the [MethodImpl(MethodImplOptions.NoInlining)] attribute to inure against it.)
Daniel Fortunov
1) Delegates are reference types 2) They are immutable, so it's not a race condition 3) I don't think inlining does change the behavior of this code. I'd expect that the parameter del becomes a new local variable when inlined.
CodeInChaos
+6  A: 

Instead of inducing performance overhead, why not use an extension method to alleviate both problems:

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

Once defined, you never have to do another null event check again:

// Works, even for null events.
MyButtonClick.Raise(this, EventArgs.Empty);
Judah Himango
THAT is a fantastic idea!
Charlie Flowers
In fact, this belongs in the language or the framework!
Charlie Flowers
See here for a generic version: http://stackoverflow.com/questions/192980/boiler-plate-code-replacement-is-there-anything-bad-about-this-code
Benjol
A: 

An extension method is not that useful IMHO, the thing is that not all events are EventHandlers.

True, but they ought to be: http://www.codeproject.com/KB/cs/event_fundamentals.aspx#5.1TheRoleofSystem.EventArgs12
Benjol
A: 

Instead of "empty delegate" approach one can define a simple extension method to encapsulate the conventional method of checking event handler against null. It is described here and here.

vkelman
A: 

One thing is missed out as an answer for this question so far: It is dangerous to avoid the check for the null value.

public class X
{
    public delegate void MyDelegate();
    public MyDelegate MyFunnyCallback = delegate() { }

    public void DoSomething()
    {
        MyFunnyCallback();
    }
}


X x = new X();

x.MyFunnyCallback = delegate() { Console.WriteLine("Howdie"); }

x.DoSomething(); // works fine

// .. re-init x
x.MyFunnyCallback = null;

// .. continue
x.DoSomething(); // crashes with an exception

The thing is: You never know who will use your code in which way. You never know, if in some years during a bug fix of your code the event/handler is set to null.

Always, write the if check.

Hope that helps ;)

ps: Thanks for the performance calculation.

pps: Edited it from a event case to and callback example. Thanks for the feedback ... I "coded" the example w/o Visual Studio and adjusted the example I had in mind to an event. Sorry for the confusion.

ppps: Do not know if it still fits to the thread ... but I think it is an important principle. Please also check another thread of stackflow

Thomas
x.MyFunnyEvent = null; <- That doesn't even compile(outside of the class). The point of an event is only supporting += and -=. And you can't even do x.MyFunnyEvent-=x.MyFunnyEvent outside of the class since the event getter is quasi `protected`. You can only break the event from the class itself(or from a derived class).
CodeInChaos
you are right ... true for events ... had a case with a simple handler. Sorry. I will try to edit.
Thomas