Use of null check in event handler

47

It's really not clear what you mean I'm afraid, but if there's the possibility of the delegate being null, you need to check that separately on each thread. Typically you'd do:

public void OnSeven()
{
    DivBySevenHandler handler = EventSeven;
    if (handler != null)
    {
        handler(...);
    }
}

This ensures that even if EventSeven changes during the course of OnSeven() you won't get a NullReferenceException.

But you're right that you don't need the null check if you've definitely got a subscribed handler. This can easily be done in C# 2 with a "no-op" handler:

public event DivBySevenHandler EventSeven = delegate {};

On the other hand, you might want some sort of locking just to make sure that you've got the "latest" set of handlers, if you might get subscriptions from various threads. I have an example in my threading tutorial which can help - although usually I'd recommend trying to avoid requiring it.

In terms of garbage collection, the event publisher ends up with a reference to the event subscriber (i.e. the target of the handler). This is only a problem if the publisher is meant to live longer than the subscriber.

---

• (for the OP/list) re the last point, this is especially true of static events (since the static field never becomes eligible for collection). Static events are best avoided for exactly this reason. - Marc Gravell♦
• I wonder if there would have been any problem having an EventDelegate category of delegates which would have the same semantics as a void delegate /except/ that the invocation of a null EventDelegate would be explicitly defined as a nop. - supercat
• What about EventSeven?.Invoke(....). is it thread safe? - Hossein Narimani Rad
• @HosseinNarimaniRad: It's as safe as the example in the answer, yes. The memory model for C# isn't as clear as it might be, but basically, I would regard it as safe. - Jon Skeet

51

The problem is that if nobody subscribes the the event, it is null. And you can't invoke against a null. Three approaches leap to mind:

• check for null (see below)
• add a "do nothing" handler: public event EventHandler MyEvent = delegate {};
• use an extension method (see below)

When checking for null, to be thread-safe, you must in theory capture the delegate reference first (in case it changes between the check and the invoke):

protected virtual void OnMyEvent() {
    EventHandler handler = MyEvent;
    if(handler != null) handler(this, EventArgs.Empty);
}

Extension methods have the unusual property that they are callable on null instances...

    public static void SafeInvoke(this EventHandler handler, object sender)
    {
        if (handler != null) handler(sender, EventArgs.Empty);
    }
    public static void SafeInvoke<T>(this EventHandler<T> handler,
        object sender, T args) where T : EventArgs
    {
        if (handler != null) handler(sender, args);
    }

then you can call:

MyEvent.SafeInvoke(this);

and it is both null-safe (via the check) and thread-safe (by reading the reference once only).

---

• like the thread safety via the implict copy on the stack - ShuggyCoUk
• Nice explanation. When you say "by reading....only", that is thread safe because once the reading is done, the target (object which is read - reference), can change freely. But how do you "read the reference once"? Perhaps I just didn't quite follow something... - dotnetdev
• @dotnetdev - because we are passing it as a method argument; it reads the current value onto the stack. Inside SafeInvoke, it only sees the copy now on the stack (the original can be updated 200 times, we'll never see it). It helps here that delegates are immutable, so changes such as... - Marc Gravell♦
• ...unsubscribing create a new delegate, and change the field to point at the new instance. Contrast to the "if(MyEvent!=null) MyEvent(...)" - there are clearly 2 reads there. - Marc Gravell♦
• @VoodooChild when sun classing, it is considered bad practice to subscribe to your own events. It is better to override when inheriting. Tht is all. - Marc Gravell♦

23

I want to append some short information about the C# 6.0-Syntax:

It is now possible to replace this:

var handler = EventSeven;

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

with this:

handler?.Invoke(this, EventArgs.Empty);

---

Combining it with expression-bodied members, you can shorten the following code:

protected virtual void OnMyEvent()
{
    EventHandler handler = MyEvent;
    handler?.Invoke(this, EventArgs.Empty);
}

down to a one-liner:

protected virtual void OnMyEvent() => MyEvent?.Invoke(this, EventArgs.Empty);

---

See MSDN for more information about the null-conditional operator
See this blog about expression-bodied members

---

• I'm trying to understand if the MyEvent?.Invoke is as safe as handler?.Invoke. Is null propagation check atomic? - Teejay
• Actually, it seems yes. codeblog.jonskeet.uk/2015/01/30/… - Teejay

2

It is always good practice to check an event handler before firing it. I do this even if I initially "guarantee" myself that it is always set. If I later change this I don't have to check all my event firing. So for each event I always have an accompanying OnXXX method like this:

private void OnEventSeven()
{
    var handler = EventSeven;
    if (handler != null)
    {
        handler(this, EventArgs.Empty);
    }
}

This is especially important if the event handler is public to your class since external callers can add and remove event handlers at will.

---

• That's not thread-safe. If the last handler unsubscribes after the "if" has been evaluated, you could end up with a NullReferenceException. - Jon Skeet
• Man, how come that so many of us didn't see the thread-UN-safety in this. Thanks for the wakeup call! - Peter Lillevold
• Seems like the same (anti) pattern is being used by many of us. But in this case, Peter is doing as Marc did (capturing the reference, but using an implicit type). What's the difference between what Marc wrote and Peter? - dotnetdev
• @Jon Skeet - Just reiterating Marc Gravell above so others aren't convinced that this isn't thread-safe. Delegates are immutable, so if handlers are unsubscribed from the source event, it will create a new object. The copy that was made of the event still references the original delegate. - Reed Rector
• @Reed: It's thread-safe now, yes. It wasn't when I wrote the comment - check the edit log. - Jon Skeet

-1

If you mean this:

public static void OnEventSeven(DivBySevenEventArgs e)
    {
        if(EventSeven!=null)
            EventSeven(new object(),e);
    }    

piece of code, then the answer is:

If nobody subscribes to the "EventSeven" event handler then you'll get a null-reference exception on "EventSeven(new object(),e);"

And the rule:

The subscriber is responsible for adding a handler (+=) and removing it (-=) when he doesn't want to receive the events any longer. Garbage collection goes by the default rules, if an object is no longer referenced it can be cleaned.

---

• That's not thread-safe. If the last handler unsubscribes after the "if" has been evaluated, you could end up with a NullreferenceException. - Jon Skeet
• True... this example is all over the place (and I use it too), never stood still at the thread safety of this! - thijs
• As an aside, isn't it convention for the sender to be the object initiating the event (i.e. this), rather than a random newly created object? - Rowland Shaw
• @Rowland - agreed - Marc Gravell♦
• "if an object is no longer referenced" ... by an object that is not itself eligible for collcetion; if a references b, and b references a, they are still both eligible for collection if nothing else can see them. - Marc Gravell♦

-1

Using PostSharp it is possible to adjust the compiled assembly in a post-compilation step. This allows you to apply 'aspects' to the code, resolving cross-cutting concerns.

Although the null checks or empty delegate initialization might be a very minor issue, I wrote an aspect which resolves it by adding an empty delegate to all events in an assembly.

It's usage is quite easy:

[assembly: InitializeEventHandlers( AttributeTargetTypes = "Main.*" )]
namespace Main
{
   ...
}

I discussed the aspect in detail on my blog. In case you have PostSharp, here is the aspect:

/// <summary>
///   Aspect which when applied on an assembly or class, initializes all the event handlers (<see cref="MulticastDelegate" />) members
///   in the class(es) with empty delegates to prevent <see cref="NullReferenceException" />'s.
/// </summary>
/// <author>Steven Jeuris</author>
[AttributeUsage( AttributeTargets.Assembly | AttributeTargets.Class | AttributeTargets.Event )]
[MulticastAttributeUsage( MulticastTargets.Event, AllowMultiple = false )]
[AspectTypeDependency( AspectDependencyAction.Commute, typeof( InitializeEventHandlersAttribute ) )]
[Serializable]
public class InitializeEventHandlersAttribute : EventLevelAspect
{
    [NonSerialized]
    Action<object> _addEmptyEventHandler;


    [OnMethodEntryAdvice, MethodPointcut( "SelectConstructors" )]
    public void OnConstructorEntry( MethodExecutionArgs args )
    {
        _addEmptyEventHandler( args.Instance );
    }

    // ReSharper disable UnusedMember.Local
    IEnumerable<ConstructorInfo> SelectConstructors( EventInfo target )
    {
        return target.DeclaringType.GetConstructors( BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic );
    }
    // ReSharper restore UnusedMember.Local

    public override void RuntimeInitialize( EventInfo eventInfo )
    {
        base.RuntimeInitialize( eventInfo );

        // Construct a suitable empty event handler.
        MethodInfo delegateInfo = DelegateHelper.MethodInfoFromDelegateType( eventInfo.EventHandlerType );
        ParameterExpression[] parameters = delegateInfo.GetParameters().Select( p => Expression.Parameter( p.ParameterType ) ).ToArray();
        Delegate emptyDelegate
            = Expression.Lambda( eventInfo.EventHandlerType, Expression.Empty(), "EmptyDelegate", true, parameters ).Compile();

        // Create a delegate which adds the empty handler to an instance.
        _addEmptyEventHandler = instance => eventInfo.AddEventHandler( instance, emptyDelegate );
    }
}

... and the helper method it uses:

/// <summary>
///   The name of the Invoke method of a Delegate.
/// </summary>
const string InvokeMethod = "Invoke";


/// <summary>
///   Get method info for a specified delegate type.
/// </summary>
/// <param name = "delegateType">The delegate type to get info for.</param>
/// <returns>The method info for the given delegate type.</returns>
public static MethodInfo MethodInfoFromDelegateType( Type delegateType )
{
    Contract.Requires( delegateType.IsSubclassOf( typeof( MulticastDelegate ) ), "Given type should be a delegate." );

    return delegateType.GetMethod( InvokeMethod );
}