Is this thread-safe?

Question

The following code is thread-safe as far as I can tell, with the caveat that I am not looking for a singleton pattern (I don't care if multiple threads get different instances of MyObject as MyObject is immutable. After the potential contention among the concurrent threads that create multiple instances, subsequent threads will all get the same intance).

private static MyObject myObject;
...
public static MyObject GetMyObject()
{
    if (myObject == null)
    {
        myObject = new MyObject(...);
    }
    return myObject;
}

The following is clearly not thread-safe, since multiple threads could attempt to access the myObject field before it is completely initialized:

private static MyObject myObject;
...
public static MyObject GetMyObject()
{
    if (myObject == null)
    {
        myObject = new MyObject();
        myObject.Initialize(...);
    }
    return myObject;
}

Is the following thread-safe, or is the volatile keyword required on the myObject field? It looks safe as it stands, since the myObject field is not assigned until the object is fully initialized. But I'd be concerned that the JITter might be able to inline the LoadMyObject method and essentially reimplement it as the above code, which isn't thread safe.

private static MyObject myObject;
...
private static MyObject LoadMyObject()
{
    MyObject myObject = new MyObject();
    myObject.Initialize(...);
    return myObject;
}

public static MyObject GetMyObject()
{
    if (myObject == null)
    {
        myObject = LoadMyObject();
    }
    return myObject;
}

The background to the above is that I want to refactor some multithreaded code, and replace calls to a constructor by calls to a factory method. And I want to know if doing so can break thread-safety.

EDIT

Just to reiterate, I don't care if multiple instances of MyObject are created, only that there is no way to access an instance that is not completely initialized. As a concrete example, think of a readonly XmlDocument that is loaded with the contents of a configuration file. I want a static reference to the XmlDocument available in memory to avoid the overhead of loading from disk on every access. But I don't care if two threads happen to run concurrently and both load the document from disk.

EDIT2

If I understand it correctly, the following statement from the C# Language Specification seems to imply that the JITter won't reorder my code, so that my last sample should be thread-safe. Am I understanding correctly?

3.10 Execution order ... Data dependence is preserved within a thread of execution. That is, the value of each variable is computed as if all statements in the thread were executed in original program order

Answer 1

If I can channel clippy for a minute: "It looks like you're trying to implement the Singleton Pattern". Please see Jon Skeet's canonical article on the subject.

Answer 2

Why don't you move Initialize method into c'tor of MyObject and initialize it with one call?

Answer 3

It's still not thread safe even without compiler optimizations. Volatile won't help in that case - it's only designed to protect a variable from being modified "under the covers". Imagine two threads pass the null check at the exact same time - you would end up calling "LoadMyObject" twice, which might be OK in this case, or might not. This is a TOCTOU bug (Time Of Check / Time Of Use). You would essentially have to make the body of the entire GetMyObject method safe, including the check (i.e. by making that call synchronized).

Answer 4

Here is my take at it:

public class TestClass
{
    private static TestClass instance = LoadObject();

    private TestClass(){ }

    private static TestClass LoadObject()
    {
        var t = new TestClass();
        //do init

        return t;
    }

    public static  TestClass GetObject()
    {
        return instance;
    }
}

Answer 5

It's safe in the sense that you will get a correctly initialised object, but several threads may create objects simultaneously, and a thread might return an object that a different thread just created.

If the compiler inlines the method, it still won't be the same as the first code. As the reference to the object is kept in a local variable until it's initialised, it's still safe.

You can inline the method yourself, there is no need to have the code in a separate method to make it safe:

public static MyObject GetMyObject() {
   if (myObject == null) {
      MyObject newObject = new MyObject();
      newObject.Initialize(...);
      myObject = newObject;
   }
   return myObject;
}

If you want to prevent that several objects are created by separate threads, you need to use a lock.

Answer 6

To answer your second edit, if your initialize method takes a little while, there is the potential that another thread may as k if the object is not null, then pull its non-initialized state. I suggest you use a lock for the null check and initialization to guarantee that the object is only retrieved when fully initialized.

If you want it to be fully thread safe this can help:

private static MyObject myObject;
private object lockObj = new object();
...
public static MyObject GetMyObject()
{
    lock(lockObj)
    {
        if (myObject == null)
        {
            myObject = new MyObject(...);
        }
    }
    return myObject;
}

Answer 7

This seems to me like an optimisation too far. Rather than getting into the subtleties of what the compiler is doing, pick whichever one of these suits you best:

• One instance (singleton)
• One instance per thread (thread-static)
• One instance per caller (no special code at all)

It all depends what penalty you want to pay. For a singleton, you pay a performance penalty. For the other two options, you pay a memory penalty.

To make it thread-static, just do this:

[ThreadStatic]
private static MyObject myObject;
...
public static MyObject GetMyObject()
{
    if (myObject == null)
    {
        myObject = new MyObject(...);
    }
    return myObject;
}

Answer 8

Volatile can be a performance option in some cases to avoid the additional hit of using a lock, but it doesn't address threadsafety issues of race conditions, which your third example (the one at question) has.

To make sure an uninitialized instance is not retrieved by another thread you should not assign to the static variable until you have a completely initialized instance. Then as soon as they see a non-null, they know it's good. Edited: (correction) While I think it's more ideal to avoid redundant work (below), I guess allowing regeneration and replacement of the static reference (as you had it, or Guffa's version) is still "safe" for your use case because each reference to GetMyObject() will grab the single object in place at the time.

I would suggest something like this:

private static object s_Lock = new object();
private static volatile MyObject s_MyObject;
private static MyObject LoadMyObject()
{
    MyObject myObject = new MyObject();
    myObject.Initialize();
    return myObject;
}
public static MyObject GetMyObject()
{
    if (s_MyObject == null)
    {
        lock (s_Lock)
        {
            if (s_MyObject == null) // Check again now that we're in the lock.
                s_MyObject = LoadMyObject(); // Only one access does this work.
        }
    }
    return s_MyObject; // Once it's set, it's never cleared or assigned again.
}

This has the advantage of only doing the init work once, but also avoiding the lock contention overhead unless it's actually needed... while still being threadsafe. (You could use volatile as above if you want to be sure it propagates out, but this should also "work" without it; it would just be more likley to need the lock before seeing the new contents of s_MyObject, depending on the architecture. So I think volatile is helpful for this approach.)

I've kept your LoadMyObject and GetMyObject methods, but you could also combine the logic into the then clause of the inner if for a single factory method.

Answer 9

But I'd be concerned that the JITter might be able to inline the LoadMyObject method and essentially reimplement it as the above code, which isn't thread safe.

Even if the method is inlined, your local won't be optimized away, and the write to the field will still happen only once.

So your last example is thread safe.

As a rule, locals aren't optimized and replaced with globals when there is more than one access to the variable.