5

I've just encountered a bug in the program I'm writing where an exception was thrown stating an "object reference must be set to an instance of an object". Upon investigation, I found that this exception was thrown when trying to fire an event BUT the event didn't have any delegate methods added to it.

I wanted to check that my understanding was correct that as a developer you shouldn't fire events without first checking that the event doesn't equal null? For example:

if (this.MyEventThatIWantToFire != null)
{
    this.MyEventThatIWantToFire();
}

Thanks in advance for the advice/thoughts.

James Bedford
  • 28,702
  • 8
  • 57
  • 64
  • 1
    Yes your understanding is correct. – btlog Feb 11 '11 at 14:54
  • Also, in a multithreaded app you would store the `this.MyEventThatIWantToFire ` in a temp variable before checking and eventually raising it so as to prevent events from being unhooked between your check and your raise. – deepee1 Feb 11 '11 at 14:56

6 Answers6

10

The pattern you've shown is broken in a multi-threaded environment. MyEventThatIWantToFire could become null after the test but before the invocation. Here's a safer approach:

EventHandler handler = MyEventThatIWantToFire;
if (handler != null)
{
    handler(...);
}

Note that unless you use some sort of memory barrier, there's no guarantee you'll see the latest set of subscribers, even ignoring the obvious race condition.

But yes, unless you know that it will be non-null, you need to perform a check or use a helper method to do the check for you.

One way of making sure there's always a subscriber is to add a no-op subscriber yourself, e.g.

public event EventHandler MyEventThatIWantToFire = delegate {};

Of course, events don't have to be implemented with simple delegate fields. For example, you could have an event backed by a List<EventHandler> instead, using an empty list to start with. That would be quite unusual though.

An event itself is never null, because the event itself is just a pair of methods (add/remove). See my article about events and delegates for more details.

Jon Skeet
  • 1,421,763
  • 867
  • 9,128
  • 9,194
4

You shouldn't do it that way - if someone where to remove a listener between the if and the call to the event, you'd get an exception. It is good practise to use it with a local variable:

protected void RaiseEvent()
{
    var handler = Event;

    if(handler != null)
        handler(this, EventArgs.Empty);
}

Of course, this has the disadvantage of maybe calling a listener that already was removed between the creation of the local variable and the call to the handler.

Femaref
  • 60,705
  • 7
  • 138
  • 176
3

Yes, you should check it for null but the way you're doing it leads to a race condition. It's possible the event may end up being null anyway if someone unsubscribes the last delegate in between your "if" and your event raise. The accepted way to do this is like:

var temp = this.MyEventThatIWantToFire;
if (temp != null) {
    this.temp(this, EventArgs.Empty);
}

or alternatively, ensure there's always at least one delegate in the invocation list by declaring your event like this:

public event EventHandler MyEventThatIWantToFire = delegate {};

Make sense?

x0n
  • 51,312
  • 7
  • 89
  • 111
1

Yes, your understanding is correct. It's up to you to check the delegate's value before calling it.

Most delegates - which events are - are "multi-cast" delegates. This means that a delegate can manage a list of one or more methods that it will call when activated.

When the delegate evaluates to a null value, there are no registered methods; therefore, there is nothing to call. If it evaluates to something other than a null value, there is at least one method registered. Calling the delegate is an instruction to call all methods it has registered. The methods are called in no guaranteed order.

Using the addition-assignment operator (+=) or addition operator (+) adds a method to the delegate's list of methods. Using the subtraction-assignment operator (-=) or subtraction operator (-) removes a method from the delegate's list of methods.

Also note that the called methods are executed from the caller's thread. This is important if you are using events to update your user interface controls. In this situation, use of your control's InvokeRequired property lets you handle cross-threading calls (using Invoke() or BeginInvoke()) gracefully.

Suncat2000
  • 1,105
  • 1
  • 14
  • 17
0

Yes in C# if an event has no delegates, it will be null, so you must check

Steve Hobbs
  • 3,296
  • 1
  • 22
  • 21
0

Yes, you must check whether event is not null. Mostly you encounter protected method that does the check and invokes event, or even constructs event args. Something like this:

public event EventHandler Foo;

protected void OnFoo() {
    if (Foo == null)
        return;

    Foo(this, new EventArgs());
}

This approach also alows your derived classes to invoke (or prevent invoking) the event.

Lukáš Novotný
  • 9,012
  • 3
  • 38
  • 46
  • 1
    this approach is not thread safe. The handler could change between the time that you test for null and when you call it, most likely during a race condition. It is better to store the event in a variable and then test for null. – Pierre-Alain Vigeant Feb 11 '11 at 14:59