Is the null coalescing operator (??) in C# thread-safe?

Question

Is there a race condition in the following code that could result in a NullReferenceException?

-- or --

Is it possible for the Callback variable to be set to null after the null coalescing operator checks for a null value but before the function is invoked?

class MyClass {
    public Action Callback { get; set; }
    public void DoCallback() {
        (Callback ?? new Action(() => { }))();
    }
}

EDIT

This is a question that arose out of curiosity. I don't normally code this way.

I'm not worried about the Callback variable becoming stale. I'm worried about an Exception being thrown from DoCallback.

EDIT #2

Here is my class:

class MyClass {
    Action Callback { get; set; }
    public void DoCallbackCoalesce() {
        (Callback ?? new Action(() => { }))();
    }
    public void DoCallbackIfElse() {
        if (null != Callback) Callback();
        else new Action(() => { })();
    }
}

The method DoCallbackIfElse has a race condition that may throw a NullReferenceException. Does the DoCallbackCoalesce method have the same condition?

And here is the IL output:

MyClass.DoCallbackCoalesce:
IL_0000:  ldarg.0     
IL_0001:  call        UserQuery+MyClass.get_Callback
IL_0006:  dup         
IL_0007:  brtrue.s    IL_0027
IL_0009:  pop         
IL_000A:  ldsfld      UserQuery+MyClass.CS$<>9__CachedAnonymousMethodDelegate1
IL_000F:  brtrue.s    IL_0022
IL_0011:  ldnull      
IL_0012:  ldftn       UserQuery+MyClass.<DoCallbackCoalesce>b__0
IL_0018:  newobj      System.Action..ctor
IL_001D:  stsfld      UserQuery+MyClass.CS$<>9__CachedAnonymousMethodDelegate1
IL_0022:  ldsfld      UserQuery+MyClass.CS$<>9__CachedAnonymousMethodDelegate1
IL_0027:  callvirt    System.Action.Invoke
IL_002C:  ret         

MyClass.DoCallbackIfElse:
IL_0000:  ldarg.0     
IL_0001:  call        UserQuery+MyClass.get_Callback
IL_0006:  brfalse.s   IL_0014
IL_0008:  ldarg.0     
IL_0009:  call        UserQuery+MyClass.get_Callback
IL_000E:  callvirt    System.Action.Invoke
IL_0013:  ret         
IL_0014:  ldsfld      UserQuery+MyClass.CS$<>9__CachedAnonymousMethodDelegate3
IL_0019:  brtrue.s    IL_002C
IL_001B:  ldnull      
IL_001C:  ldftn       UserQuery+MyClass.<DoCallbackIfElse>b__2
IL_0022:  newobj      System.Action..ctor
IL_0027:  stsfld      UserQuery+MyClass.CS$<>9__CachedAnonymousMethodDelegate3
IL_002C:  ldsfld      UserQuery+MyClass.CS$<>9__CachedAnonymousMethodDelegate3
IL_0031:  callvirt    System.Action.Invoke
IL_0036:  ret    

It looks to me like call UserQuery+MyClass.get_Callback is only getting called once when using the ?? operator, but twice when using if...else. Am I doing something wrong?

Answer 1

public void DoCallback() {
    (Callback ?? new Action(() => { }))();
}

is guaranteed to be equivalent to:

public void DoCallback() {
    Action local = Callback;
    if (local == null)
       local = new Action(() => { });
    local();
}

Whether this may cause a NullReferenceException depends on the memory model. The Microsoft .NET framework memory model is documented to never introduce additional reads, so the value tested against null is the same value that will be invoked, and your code is safe. However, the ECMA-335 CLI memory model is less strict and allows the runtime to eliminate the local variable and access the Callback field twice (I'm assuming it's a field or a property that accesses a simple field).

You should mark the Callback field volatile to ensure the proper memory barrier is used - this makes the code safe even in the weak ECMA-335 model.

If it's not performance critical code, just use a lock (reading Callback into a local variable inside the lock is sufficient, you don't need to hold the lock while invoking the delegate) - anything else requires detailed knowledge about memory models to know whether it is safe, and the exact details might change in future .NET versions (unlike Java, Microsoft hasn't fully specified the .NET memory model).