Checking delegates for null

Question

I was reading the Essential C# 3.0 book and am wondering if this is a good way to check delegates for null?:

class Thermostat
{
    public delegate void TemperatureChangeHandler ( float newTemperature );

    public TemperatureChangeHandler OnTemperatureChange { get; set; }

    float currentTemperature;

    public float CurrentTemperature
    {
        get { return this.currentTemperature; }
        set
        {
            if ( currentTemperature != value )
            {
                currentTemperature = value;

                TemperatureChangeHandler handler = OnTemperatureChange;

                if ( handler != null )
                {
                    handler ( value );
                }
            }
        }
    }
}

Does the solution changes if the type is immutable? I figured maybe with immutability you wouldn't run into this threading problem.

Archiphoneme

Thanks, I didn't think it thoroughly but just thought that might help for this threading case. – Archiphoneme

Answer 1

Original (somewhat inaccurate) Response:

There has been much discussion on this.

In short: you can't guarantee that the handler will be valid even by doing this copy/check for null/ execute step.

The problem is, if OnTemperatureChange is unregistered between the time you copy it, and the time you execute the copy, then it's probably true that you don't want the listener to be executed anyway.

You may as well just do:

if (OnTemperatureChange != null )
{
    OnTemperatureChange ( value );
}

And handle a null reference exception.

I sometimes add a default handler that does nothing, just to prevent the null reference exception, but that adds performance impact quite seriously, especially in the case where there is no other handler registered.

Update 2014-07-10:

I defer to Eric Lippert.

My original response did allude to using default handlers, but I didn't recommend using a temp variable, which I now agree as good practice also, per the article.

Gomes

you can guarantee if you manually write the add/remove code for the event and handle the synchronization, otherwise multithreaded code should handle the possible exceptions – Luthuli

So basically if I understand correctly the simple solution works in single threaded code all the time, but it's not guaranteed in multi-threaded code, and there's not simple solution? – Schoenfeld

Yerp, multi threaded code is really tricky to get right in this regard see: https://mcmap.net/q/16675/-c-events-and-thread-safety – Luthuli

Event handlers are supposed to gracefully handle being called even after they've been unregistered. The object the event handler is in will also not be garbage collected as long as the delegate holds a reference to it. In short after creating an immutable copy of an event delegate and checking it for null, it is completely safe to execute the copy. Any errors that arise out of this are the handlers' fault and they must be corrected. – Hindustan

@jnm2 I think Eric Lipperts answer to the same question is a little clearer? blogs.msdn.com/b/ericlippert/archive/2009/04/29/… – Gomes

Is there a different link to the blog post? It seems to be a dead link these days. – Gonococcus

@S.Buda I changed it – Oregano

Answer 2

Use a question mark for a conditional access:

OnTemperatureChange?.Invoke();

The null-conditional operators are short-circuiting. That is, if one operation in a chain of conditional member or element access operations returns null, the rest of the chain doesn't execute.

Hebdomadal

Only available in C# 6 or higher. – Longe

C# 6 was released 2016, you can simply upgrade. – Hebdomadal

I'm aware, it was just a warning. Not all environments will support it. (Unity, for instance, won't) – Longe

@MatheusRocha Unity has C# 7 for over a year now. The ? operator is supported for non Unity objects- so in the case of delegates, that should be perfectly supported. – Gonococcus

Answer 3

There is a reason the code you've given is recommended over C. Ross's version. However, John is also right that there is still another problem if an event is unregistered in the meanwhile. The blog I linked recommends that the handler ensure they can be called even after being unregistered.

Tourism

Answer 4

First, you aren't actually publishing an event - so at the moment, your code is "at risk" of people messing it up completely. It should be:

public event TemperatureChangeHandler CurrentTemperatureChanged;

The name "CurrentTemperatureChanged" is important for data-binding (there is a convention that the runtime uses - given a property Foo, it will look for FooChanged). However, IMO this should just be regular EventHandler. Data-binding will look for EventHandler, but more importantly: you aren't actually giving any information in the event that the subscriber can't already get just by looking at obj.CurrentTemperature.

I'll give the rest of the answer in terms of TemperatureChangeHandler, but I would encourage you (again) to switch to EventHandler:

public event EventHandler CurrentTemperatureChanged;

The approach:

TemperatureChangeHandler handler = CurrentTemperatureChanged;
if(handler != null) handler(value);

is reasonable, but (as per other replies) there is a slim risk of callers that think they disconnected getting the event. Unlikely in reality.

Another approach is an extension method:

public static class TemperatureChangeExt {
    public static void SafeInvoke(this TemperatureChangeHandler handler,
             float newTemperature) {
        if (handler != null) handler(newTemperature);
    }
}

Then in your class you can just use:

        if (currentTemperature != value) {
            currentTemperature = value;
            CurrentTemperatureChanged.SafeInvoke(value);
        }

Intrigante

Answer 5

If the Thermostat class doesn't need to be thread safe then yes the above code is fine - as long as there is only one thread accessing that instance of Thermostat there is no way for OnTemperatureChange to become unregistered between the test for null and the call to the event.

If you need to make Thermostat thread safe then you might want to take a look at the following article (new to me, looks like a good read):

http://www.yoda.arachsys.com/csharp/events.html

For the record, the recommendation is that you develop your classes not to be thread safe unless thread safety is explicitly needed as it can significantly increase the complexity of your code.

Benefit

Developing for thread safety does bot in it self increase complexity. E.g. Immutables is often a good idea in any case and they are always thread safe. – Kenwood

Answer 6

I just see a bit of refactoring that could be done but otherwise it looks good...

class Thermostat
{
    public delegate void TemperatureChangeHandler ( float newTemperature );

    public TemperatureChangeHandler OnTemperatureChange { get; set; }

    float currentTemperature;

    public float CurrentTemperature
    {
        get { return this.currentTemperature; }
        set
        {
                if (currentTemperature != value)
                {
                        currentTemperature = value;

                        if (this.OnTemperatureChange != null )
                        {
                                this.OnTemperatureChange.Invoke( value );
                        }
                }
        }
    }
}

Audreyaudri

Interesting... I've not seen that approach before. Why would you do .Invoke() ? – Gomes

Thanks. This doesn't have the same problems others mention? – Archiphoneme

wouldn't Invoke() work? It is a delegate isn't it? Not trying to be a smart a.. I may have missed something. – Audreyaudri

Without the temp variable, this can give you a null-pointer exception. The problem is this.OnTemperatureChange can become null after you check. See blogs.msdn.com/ericlippert/archive/2009/04/29/… – Tourism

@J.13.L 6 you can call the delegated method by use of method syntax, so just go: this.OnTemperatureChange(value); – Kenwood

Yep, you're in a catch-22. If you copy it to a temp variable and do the check for null, you can get caught with the fact that the listener that unregisters themselves is no longer in a state capable of receiving the event. A real conundrum that I don't see a good answer to. – Nyctaginaceous

The Above post from Eric describes just how to get out of the catch-22. The callee should it self be aware of it's own state and wether a call from the event handler is acceptable – Kenwood