Why is (or isn't) setting fields in a constructor thread-safe?

Question

Let's say you have a simple class like this:

class MyClass
{
    private readonly int a;
    private int b;

    public MyClass(int a, int b) { this.a = a; this.b = b; }

    public int A { get { return a; } }
    public int B { get { return b; } }
}

I could use this class in a multi-threaded manner:

MyClass value = null;
Task.Run(() => {
    while (true) { value = new MyClass(1, 1); Thread.Sleep(10); }
});
while (true)
{
    MyClass result = value;
    if (result != null && (result.A != 1 || result.B != 1)) { 
        throw new Exception(); 
    }
    Thread.Sleep(10);
}

My question is: will I ever see this (or other similar multi-threaded code) throw an exception? I often see reference to the fact that non-volatile writes might not immediately be seen by other threads. Thus, it seems like this could fail because the write to the value field might happen before the writes to a and b. Is this possible, or is there something in the memory model that makes this (quite common) pattern safe? If so, what is it? Does readonly matter for this purpose? Would it matter if a and b were a type that can't be atomically written (e. g. a custom struct)?

Answer 1

Code as written will work starting from CLR2.0 as the CLR2.0 memory model guarantees that All stores have release semantics.

Release semantics: Ensures no load or store that comes before the fence will move after the fence. Instructions after it may still happen before the fence.(Taken from CPOW Page 512).

Which means that constructor initialization cannot be moved after the assignment of the class reference.

Joe duffy mentioned this in his article about the very same subject.

Rule 2: All stores have release semantics, i.e. no load or store may move after one.

Also Vance morrison's article here confirms the same(Section Technique 4: Lazy Initialization).

Like all techniques that remove read locks, the code in Figure 7 relies on strong write ordering. For example, this code would be incorrect in the ECMA memory model unless myValue was made volatile because the writes that initialize the LazyInitClass instance might be delayed until after the write to myValue, allowing the client of GetValue to read the uninitialized state. In the .NET Framework 2.0 model, the code works without volatile declarations.

Writes are guaranteed to happen in order starting from CLR 2.0. It is not specified in ECMA standard, it is just the microsoft implementation of the CLR gives this guarantee. If you run this code in either CLR 1.0 or any other implementation of CLR, your code is likely to break.

Story behind this change is:(From CPOW Page 516)

When the CLR 2.0 was ported to IA64, its initial development had happened on X86 processors, and so it was poorly equipped to deal with arbitrary store reordering (as permitted by IA64) . The same was true of most code written to target .NET by nonMicrosoft developers targeting Windows

The result was that a lot of code in the framework broke when run on IA64, particularly code having to do with the infamous double-checked locking pattern that suddenly didn't work properly. We'll examine this in the context of the pattern later in this chapter. But in summary, if stores can pass other stores, consider this: a thread might initialize a private object's fields and then publish a reference to it in a shared location; because stores can move around, another thread might be able to see the reference to the object, read it, and yet see the fields while they are still i n an uninitialized state. Not only did this impact existing code, it could violate type system properties such as initonly fields.

So the CLR architects made a decision to strengthen 2.0 by emitting all stores on IA64 as release fences. This gave all CLR programs stronger memory model behavior. This ensures that programmers needn' t have to worry about subtle race conditions that would only manifest in practice on an obscure, rarely used and expensive architecture.

Note Joe duffy says that they strengthen 2.0 by emitting all stores on IA64 as release fences which doesn't mean that other processors can reorder it. Other processors itself inherently provides the guarantee that store-store(store followed by store) will not be reordered. So CLR doesn't need to explicitly guarantee this.

Answer 2

The code described above is thread safe. The constructor is fully executed before it is assigned to the "value" variable. The local copy in the second loop will either be null or a fully constructed instance as assigning an instance reference is an atomic operation in memory.

If "value" was a structure then it would not be thread safe as the initialization of value wouldn't be atomic.

Answer 3

Thus, it seems like this could fail because the write to the value field might happen before the writes to a and b. Is this possible

Yes, it most certainly is possible.

You'll need to synchronize access to the data in some way to prevent such a reordering.

Answer 4

will I ever see this (or other similar multi-threaded code) throw an exception?

Yes, on ARM (and any other hardware with weak memory model) you will observe such behavior.

I often see reference to the fact that non-volatile writes might not immediately be seen by other threads. Thus, it seems like this could fail because the write to the value field might happen before the writes to a and b. Is this possible, or is there something in the memory model that makes this (quite common) pattern safe?

Volatile is not about instantaneousness of changes observation, it's about order and acquire/release semantics.
Moreover, ECMA-335 say that it can happen (and it will happen on ARM or any other hardware with weak memory model).

Does readonly matter for this purpose?

readonly has nothing to do with instructions reordering and volatile.

Would it matter if a and b were a type that can't be atomically written (e. g. a custom struct)?

Atomicity of fields doesn't matter in this scenario. To prevent that situation you should write reference to created object via Volatile.Write (or just make that reference volatile and compiler will do the job). Volatile.Write(ref value, new MyClass(1, 1)) will do the trick.

For more information on volatile semantics and memory model see ECMA-335, section I.12.6

Answer 5

As written, this code is threadsafe because value is not updated until constructor has finished executing. In other words, the object under construction is not observed by anyone else.

You can write code which helps you shoot yourself in the face by explicitly publishing this to the outside world like

class C { public C( ICObserver observer ) { observer.Observe(this); } }

When Observe() executes, all bets are off because it no longer holds true that object is not observed by outside world.

Answer 6

This was wrong, sorry...

I think you could throw an error if your test if executed before the first assignment of the variable in the other thread. This would be a race condition... it may be an intermittent problem.

You could also get an error if the while loop checks the values at the exact moment that a new class is instantiated and assigned to value, but before the a and b variables are set.

As for a better way to do this, it would depend on your goal. Is there a reason value keeps getting overwritten? I would think it more common that the new classes would be placed into a collection which need to be processed in order.

There are collections that would handle this. You would add classes to the collection in one thread, and check and extract them in the other.

See http://dotnetcodr.com/2014/01/14/thread-safe-collections-in-net-concurrentstack/ for an example.