Use of null check in event handler

Question

When checking if an event handler is null, is this done on a per-thread basis?

Ensuring someone is listening to the event is done like this:

EventSeven += new DivBySevenHandler(dbsl.ShowOnScreen);

If I add code following this pattern above where I check for null, then why would I need a null check (code taken from this site). What am I missing?

Also, what's the rule with events and GC?

Pulsifer

See: dailycoding.com/Posts/avoiding_event__null_check.aspx for an expanded explanation. – Optimum

Answer 1

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.

Medic

(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. – Qua

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. – Elector

@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. – Medic

@JonSkeet Query about your last point, which touched on GC. From the example, it's my understanding that the variable handler is scoped to the funciton OnSeven, and will die when that function completes. So the publisher shouldn't end up having a permanent reference to the subscriber(s). Am I missing something? – Trattoria

@ColmBhandal: No, the publisher does have a permanent reference to the subscriber - because handler is just copying the value from EventSeven. That's a field (or backed by a field) in the subscriber, and it has a reference to the subscriber, for as long as the subscriber is subscribed. To put it another way: if the publisher doesn't know anything about the subscriber, how do you expect that information to be present within the OnSeven method? It's got to come from somewhere... – Medic

@JonSkeet My confusion is around whether your comment at the end of the answer applies specifically to the fact that the publisher has created the variable handler, which I believe creates a temporary snapshot of the subscriber(s) at a given point in time, copied from EventSeven. Or are you just commenting that, in general, if you use events in C#, then the subscribers had better unsubscribe before dying? – Trattoria

@ColmBhandal: No, it's unrelated to the "how do I invoke an event". It's a general comment, in the same way that the question was asked in a general way: "Also, [...]" – Medic

Answer 2

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).

Qua

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... – Pulsifer

@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... – Qua

...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. – Qua

+ 1 for the generic extension method. Also why is it virtual in protected virtual void OnMyEvent() - is there a reason? – Spermogonium

@Spermogonium when sun classing, it is considered bad practice to subscribe to your own events. It is better to override when inheriting. Tht is all. – Qua

Seems I can't call the second extension method on more specific kinds of EventHandler<T> like PropertyChangedEventHandler. This makes it not so useful I think. – Zamboanga

PropertyChangedEventHandler predates generics, otherwise presumably it would have been EventHandler<PropertyChangedEventArgs> You need to add additional extension methods for these, for example PropertyChangedEventHandler, ListChangedEventHandler (binding lists), etc. – Sharlenesharline

Answer 3

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

Mohair

I'm trying to understand if the MyEvent?.Invoke is as safe as handler?.Invoke. Is null propagation check atomic? – Dripping

Actually, it seems yes. codeblog.jonskeet.uk/2015/01/30/… – Dripping

Answer 4

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.

Cq

That's not thread-safe. If the last handler unsubscribes after the "if" has been evaluated, you could end up with a NullReferenceException. – Medic

Man, how come that so many of us didn't see the thread-UN-safety in this. Thanks for the wakeup call! – Cq

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? – Pulsifer

Marc's code: EventHandler handler = MyEvent; if(handler != null) handler(this, EventArgs.Empty); Pretty much the same, apart from the absence of var and instead the actual type. – Pulsifer

@dotnetdev - yes, guess I'm lazy. – Cq

@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. – Bechance

@Reed: It's thread-safe now, yes. It wasn't when I wrote the comment - check the edit log. – Medic

Answer 5

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.

Ostensible

That's not thread-safe. If the last handler unsubscribes after the "if" has been evaluated, you could end up with a NullreferenceException. – Medic

True... this example is all over the place (and I use it too), never stood still at the thread safety of this! – Ostensible

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? – Epochmaking

"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. – Qua

@Rowland Shaw I copied the code from the link provided by the question – Ostensible

Answer 6

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 );
}

Skees