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

Update

If we exclude the problem of getting a stale value as your edit clarifies then the null-coalescing option would always work reliably (even if the exact behavior cannot be determined). The alternate version (if not null then call it) however would not, and is risking a NullReferenceException.

The null-coalescing operator results in Callback being evaluated just once. Delegates are immutable:

Combining operations, such as Combine and Remove, do not alter existing delegates. Instead, such an operation returns a new delegate that contains the results of the operation, an unchanged delegate, or null. A combining operation returns null when the result of the operation is a delegate that does not reference at least one method. A combining operation returns an unchanged delegate when the requested operation has no effect.

Additionally, delegates are reference types so simple reading or writing one is guaranteed to be atomic (C# language specification, para 5.5):

Reads and writes of the following data types are atomic: bool, char, byte, sbyte, short, ushort, uint, int, float, and reference types.

This confirms that there is no way that the null-coalescing operator would read an invalid value, and because the value will be read only once there's no possibility of an error.

On the other hand, the conditional version reads the delegate once and then invokes the result of a second, independent read. If the first read returns a non-null value but the delegate is (atomically, but that doesn't help) overwritten with null before the second read happens, the compiler ends up calling Invoke on a null reference, hence an exception will be thrown.

All of this is reflected in the IL for the two methods.

Original Answer

In the absence of explicit documentation to the contrary then yes, there is a race condition here as there would also be in the simpler case

public int x = 1;

int y = x == 1 ? 1 : 0;

The principle is the same: first the condition is evaluated, and then the result of the expression is produced (and later used). If something happens that makes the condition change, it's too late.

Answer 2

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

Answer 3

I see no race condition in this code. There are a few potential issues:

• Callback += someMethod; is not atomic. Simple assignment is.
• DoCallback can call a stale value, but it will be consistent.
• The stale value problem can only be avoided by keeping a lock for the whole duration of the callback. But that is a very dangerous pattern, that invites dead-locks.

A clearer way of writing DoCallback would be:

public void DoCallback()
{
   var callback = Callback;//Copying to local variable is necessary
   if(callback != null)
     callback();
}

It's also a bit faster that your original code, since it doesn't create and invoke a no-op delegate, if Callback is null.

---

And you might want to replace the property by an event, to gain atomic += and -=:

 public event Action Callback;

When calling += on a property, what happens is Callback = Callback + someMethod. This is not atomic, since Callback might be changed between the read and the write.

When calling += on a field like event, what happens is a call to the Subscribe method of the event. Event subscription is guaranteed to be atomic for field like events. In practice it uses some Interlocked technique to do this.

---

The use of the null coalescence operator ?? doesn't really matter here, and it's not inherently thread safe either. What matters is that you read Callback only once. There are other similar patterns involving ?? that are not thread safe in any way.

Answer 4

We're you assuming it was safe because it is one line? That's usually not the case. You really should use the lock statement before accessing any shared memory.