2

Reference

I'm currently dealing with some thread sensitive code.

In my code I have a list of objects that is manipulated by two different threads. One thread can add objects to this list, while the other may set it to null.

In the above reference it specifically mentions that for delegates:

myDelegate?.Invoke()

is equivalent to:

var handler = myDelegate;
if (handler != null)
{
    handler(…);
}

My question is, is this behavior the same for say, a List<>? Eg:

Is:

var myList = new List<object>();    
myList?.Add(new object());

guaranteed to be equivalent to:

var myList = new List<object>();

var tempList = myList;
if (tempList != null)
{
    tempList.Add(new object());
}

?


EDIT:

Note that there is a difference between (how a delegate works):

var myList = new List<int>();
var tempList = myList;
if (tempList != null)
{
    myList = null; // another thread sets myList to null here
    tempList.Add(1); // doesn't crash
}

And

var myList = new List<int>();
if (myList != null)
{
    myList = null; // another thread sets myList to null here
    myList.Add(1); // crashes
}
Salah Akbari
  • 39,330
  • 10
  • 79
  • 109
Loocid
  • 6,112
  • 1
  • 24
  • 42
  • Why don't you just write the code and test if your hypothesis is true? – Enigmativity Jul 10 '19 at 03:46
  • I wasn't sure how to test my hypothesis, before TheGeneral's answer I didn't know how to view the complied code. – Loocid Jul 10 '19 at 03:51
  • As for the thread-safety part, you may want to read [this](https://stackoverflow.com/questions/786383/c-sharp-events-and-thread-safety) because there is a race condition you may want to know about as it pertains to delegates. – Wyck Jul 10 '19 at 03:51
  • Edited my question with a specific example of why I need them to be the same. – Loocid Jul 10 '19 at 04:03
  • 2
    @Enigmativity: The question is "is it *possible* that a compiler, jitter, or CPU introduce an extra read in this scenario?" You cannot write any code that tests that proposition! You can only write code that tests "did this particular compiler, jitter and CPU not introduce an extra read on the day I ran the code?" and that's not the question that needs an answer. – Eric Lippert Jul 11 '19 at 22:09
  • That said, I am confused by the question. You have a local variable, initialized to a new object. It will never be null, so why would you ask if the null check has any particular behaviour? **Ask the question about a sample that more closely resembles the real code**. – Eric Lippert Jul 11 '19 at 22:15
  • Moreover, the supposition is that the problem is the read on the variable, when there is a much bigger problem staring us in the face: the `Add` is not safe! Again, show us the real code and we can critique it. It sounds like you are worried about the small problems when there are really big problems to be addressed; address them first. – Eric Lippert Jul 11 '19 at 22:45
  • 2
    @EricLippert - Thank you, Eric. Of all the people here, you are the one person that I like to, or even look forward to, being corrected by. :-) – Enigmativity Jul 12 '19 at 01:00

4 Answers4

8

This is a subtle problem that requires careful analysis.

First off, the code posed in the question is pointless, because it does a null check on a local variable that is guaranteed to not be null. Presumably the real code reads from a non-local variable that may or may not be null, and may be altered on multiple threads.

This is a super dangerous position to be in and I strongly discourage you from pursuing this architectural decision. Find another way to share memory across workers.

To address your question:

The first version of the question is: does the ?. operator have the same semantics as your version where you introduce a temporary?

Yes, it does. But we're not done.

The second question, that you did not ask, is: is it possible that the C# compiler, jitter, or CPU causes the version with the temporary to introduce an extra read? That is, are we guaranteed that

var tempList = someListThatCouldBeNull;
if (tempList != null)
    tempList.Add(new object());

is never executed as though you wrote

var tempList = someListThatCouldBeNull;
if (tempList != null) 
    someListThatCouldBeNull.Add(new object());

The question of "introduced reads" is complicated in C#, but the short version is: generally speaking you can assume that reads will not be introduced in this manner.

Are we good? Of course not. The code is completely not threadsafe because Add might be called on multiple threads, which is undefined behaviour!

Suppose we fix that, somehow. Are things good now?

No. We still should not have confidence in this code.

Why not?

The original poster has shown no mechanism which guarantees that an up-to-date value of someListThatCouldBeNull is being read. Is it accessed under a lock? Is it volatile? Are memory barriers introduced? The C# specification is very clear on the fact that reads may be moved arbitrarily backwards in time if there are no special effects such as locks or volatiles involved. You might be reading a cached value.

Similarly, we have not seen the code which does the writes; those writes can be moved arbitrarily far into the future. Any combination of a read moved into the past or a write moved into the future can lead to a "stale" value being read.

Now suppose we solve that problem. Does that solve the whole problem? Certainly not. We do not know how many threads there are involved, or if any of those threads are also reading related variables, and if there are any assumed ordering constraints on those reads. C# does not require that there be a globally consistent view of the order of all reads and writes! Two threads may disagree on the order in which reads and writes to volatile variables happened. That is, if the memory model permits two possible observed orderings, it is legal for one thread to observe one, and the other thread to observe the other. If your program logic implicitly depends on there being a single observed ordering of reads and writes, your program is wrong.

Now perhaps you see why I strongly advise against sharing memory in this manner. It is a minefield of subtle bugs.

So what should you do?

  • If you can: stop using threads. Find a different way to handle your asynchrony.
  • If you cannot do that, use threads as workers that solve a problem and then go back to the pool. Having two threads both hammering on the same memory at the same time is hard to get right. Having one thread go off and compute something and return the value when it is done is a lot easier to get right, and you can...
  • ... use the task parallel library or another tool designed to manage inter-thread communication properly.
  • If you cannot do that, try to mutate as few variables as possible. Do not be setting a variable to null. If you're filling in a list, initialize the list with a threadsafe list type once, and then only read from that variable. Let the list object handle the threading concerns for you.
Eric Lippert
  • 647,829
  • 179
  • 1,238
  • 2,067
2

The answer is yes.

var myList = new List<object>();    
myList?.Add(new object());

Compiles to the following (as seen here)

List<object> list = new List<object>();
if (list != null)
{
    list.Add(new object());
}
TheGeneral
  • 79,002
  • 9
  • 103
  • 141
  • Actually, now that I'm looking at it, it's not equivalent to the delegate. Note the delegate uses a temporary variable, where as the List doesn't. – Loocid Jul 10 '19 at 03:48
2

In this answer, Eric Lippert confirms that a temporary variable is used in all cases, which will prevent the "?." operator from causing a NullReferenceException or accessing two different objects. There are, however, many other factors which can make this code not thread safe, see Eric's answer.

UPD: to address the claim that a temporary variable is not created: there is no need to introduce a temp variable for a local variable. However, if you try to access something that may conceivably be modified, a variable gets created. Using the same SharpLab with slightly modified code we get:

using System;
using System.Collections.Generic;

public class C {
    public List<Object> mList;

    public void M() {
        this.mList?.Add(new object());
    }
}

becomes

public class C
{
    public List<object> mList;

    public void M()
    {
        List<object> list = mList;
        if (list != null)
        {
            list.Add(new object());
        }
    }
}
IMil
  • 1,391
  • 13
  • 17
  • That doesn't seem to be the case with the compiled code in the other answers though, I don't know who to believe! Eric's answer is 3 years old so things may have changed :/ – Loocid Jul 10 '19 at 04:19
  • @Loocid The compiled examples don't show what you think they show. They don't introduce a temporary variable because you are accessing a local variable, and no-one outside of your code has a way to modify it. – IMil Jul 10 '19 at 04:27
  • Bingo, you're 100% right. That does make sense. As soon as I moved myList out of the scope in those examples it created temporary list. – Loocid Jul 10 '19 at 04:31
  • 3
    **I confirm nothing of the sort**. I confirm that a temporary is used, yes, but the entire point of that answer is *the resulting operation may still not be safe*. The safety of the operation depends not just on no extra reads being introduced, but *also* on the correct handling of the race condition where side effects of the event handler being removed races with the event handler executing. – Eric Lippert Jul 11 '19 at 22:04
  • @EricLippert sorry for oversimplifying your words. I didn't imply that using the Elvis operator magically made the code thread-safe; there obviously may be lots of pitfalls. But the OP was only concerned with the possibility of an object (not even a handler) becoming null, so to speak, between "?" and ".", that's what I meant by "*for your purposes*". Let me clarify this in my answer. – IMil Jul 11 '19 at 22:48
0

Yes, they are same. You can also see the underlying IL below, generated by Ildasm:

public void M()
{
    var myList = new List<object>();
    myList?.Add(new object());
}

This will be:

.method public hidebysig instance void  M() cil managed
{
  // Code size       25 (0x19)
  .maxstack  2
  .locals init (class [System.Collections]System.Collections.Generic.List`1<object> V_0)
  IL_0000:  nop
  IL_0001:  newobj     instance void class [System.Collections]System.Collections.Generic.List`1<object>::.ctor()
  IL_0006:  stloc.0
  IL_0007:  ldloc.0
  IL_0008:  brtrue.s   IL_000c
  IL_000a:  br.s       IL_0018
  IL_000c:  ldloc.0
  IL_000d:  newobj     instance void [System.Runtime]System.Object::.ctor()
  IL_0012:  call       instance void class [System.Collections]System.Collections.Generic.List`1<object>::Add(!0)
  IL_0017:  nop
  IL_0018:  ret
} // end of method C::M

And:

public void M2()
{
    List<object> list = new List<object>();
    if (list != null)
    {
        list.Add(new object());
    }
}

This will be:

.method public hidebysig instance void  M2() cil managed
{
  // Code size       30 (0x1e)
  .maxstack  2
  .locals init (class [System.Collections]System.Collections.Generic.List`1<object> V_0,
           bool V_1)
  IL_0000:  nop
  IL_0001:  newobj     instance void class [System.Collections]System.Collections.Generic.List`1<object>::.ctor()
  IL_0006:  stloc.0
  IL_0007:  ldloc.0
  IL_0008:  ldnull
  IL_0009:  cgt.un
  IL_000b:  stloc.1
  IL_000c:  ldloc.1
  IL_000d:  brfalse.s  IL_001d
  IL_000f:  nop
  IL_0010:  ldloc.0
  IL_0011:  newobj     instance void [System.Runtime]System.Object::.ctor()
  IL_0016:  callvirt   instance void class [System.Collections]System.Collections.Generic.List`1<object>::Add(!0)
  IL_001b:  nop
  IL_001c:  nop
  IL_001d:  ret
} // end of method C::M2
Salah Akbari
  • 39,330
  • 10
  • 79
  • 109
  • Same as I said to @TheGeneral. This is NOT equivalent to the delegate. The delegate is assigned to a temporary variable before being called. The List is not. – Loocid Jul 10 '19 at 03:52