Code Inspection: Possible incorrect implementation of Double-Check Locking pattern. Read access to checked field

Consider the following piece of code:

public class Foo
{
  private static volatile Foo _instance;
  private static readonly object Padlock = new object();

  public static Foo GetValue()
  {
    if (_instance != null) return _instance;
    lock (Padlock)
    {
      if (_instance != null) return _instance;
      _instance = new Foo();
      _instance.Init();
    }
    return _instance;
  }

  private void Init()
  {
    // object initialization
  }
}
    

If we assume that Init() is a method used to initialize the state of Foo, then the above code may not function as expected in a multi-threaded environment.

There may be a situation when one thread is executed _instance = new Foo(); but not yet executed _instance.Init();. If at this moment some other thread calls GetValue(), the method will see that _instance is not null and a non-initialized instance will be returned to the caller.

There are two ways to resolve this issue for the code above.

The first, and most obvious, is to relocate the contents of Init() to a private constructor.

The second is to perform the initialization in a non-checked variable and then assign it to the checked one, thus eliminating the problem. This way _instance will only become not null when it is already initialized. The code in the lock statement from the above example could be re-written as:

if (_instance != null) return _instance;
var temp = new Foo();
temp.Init();
_instance = temp;
    

This answer on StackOverflow explains other possible problems with this pattern, and why _instance should be declared volatile.

Last modified: 08 March 2021