Double-checked locking in .NET

Question

I came across this article discussing why the double-check locking paradigm is broken in Java. Is the paradigm valid for .NET (in particular, C#), if variables are declared volatile?

Spermatophyte

Answer 1

Implementing the Singleton Pattern in C# talks about this problem in the third version.

It says:

Making the instance variable volatile can make it work, as would explicit memory barrier calls, although in the latter case even experts can't agree exactly which barriers are required. I tend to try to avoid situations where experts don't agree what's right and what's wrong!

The author seems to imply that double locking is less likely to work than other strategies and thus should not be used.

Elberfeld

Answer 2

Double-checking locking now works in Java as well as C# (the Java memory model changed and this is one of the effects). However, you have to get it exactly right. If you mess things up even slightly, you may well end up losing the thread safety.

As other answers have stated, if you're implementing the singleton pattern there are much better ways to do it. Personally, if I'm in a situation where I have to choose between implementing double-checked locking myself and "lock every time" code I'd go for locking every time until I'd got real evidence that it was causing a bottleneck. When it comes to threading, a simple and obviously-correct pattern is worth a lot.

Extravagant

@Jon: Article from the question states that volatile helps for JDK5+. What about .NET? Was volatile enough to implement a proper double-check locking on .NET 1.1? (for example, what about the example "Third version - attempted thread-safety using double-check locking" in your article: is it technically 'fixed' when volatile put on instance static field?) – Sulphurate

@IgorK: Yes, I believe it works when the variable is marked as volatile. – Extravagant

@Jon: OK, thank you. At least for singletons we have better solutions indeed. – Sulphurate

should I use this pattern where I'm dealing with disk access rather than singleton? In my case the 'check' is IsFileOnDiskUpToDate() and the 'new Singleton' is actually DownloadContentAndWriteOrUpdateFileOnDisk() and in this case do I need 'an explicit memory barrier' ? (I was directed here from my closed question: #5037018 ) – Appendicular

@Myster: I would personally just take out the lock always - particularly if you're doing other slow things, which make the cost of the lock insignificant. – Extravagant

@Jon the 'lock' is preventing multiple threads writing the file simultaneously. did you mean to say take out the 'outer check'? as per example 2 in this article yoda.arachsys.com/csharp/singleton.html ? – Appendicular

@Myster: By "take out the lock" I meant "acquire the lock". I see that it was ambiguous :) Yes, remove the outer check. – Extravagant

When you talk about using an explicit memory barrier with double checked locking is the code you're referring to the same as the 2nd illustration included in blogs.msdn.com/b/brada/archive/2004/05/12/130935.aspx with comment // Insure all writes... – Hypnotize

@ChrisMarisic: Yes, that sort of thing. – Extravagant

I disagree with this. "Lock every time" is a terrible approach. Never do this. – Sizeable

@Mr.TA: Please quote the full sentence: "Personally, if I'm in a situation where I have to choose between double-checked locking and "lock every time" code I'd go for locking every time until I'd got real evidence that it was causing a bottleneck." There are approaches that don't require locking, and I'd generally use those - I wouldn't go for DCL though. – Extravagant

@JonSkeet I was responding to that. Obviously single-threaded applications aside, in multi-threaded environments, anything other than simple singleton (which is an anti-pattern anyway) which is accomplished nicely by relying on .NET type initializer, calls for DCL. For example, you want to cache some setting value which you retrieve from an external source. Code like that doesn't belong in a type initializer. Lazy<T> wraps around that so perhaps it's better to use it instead of rolling your own, but still, it's DCL. – Sizeable

@Mr.TA: But it's not implementing the DCL yourself which is what I'm strongly suggesting people avoid. I'm perfectly happy to use Lazy<T>, but I don't think I'd ever want to write code implementing DCL myself. I'll edit the answer to make it clear I'm talking about implementing rather than using a tried-and-trusted implementation, but I hoped that was already pretty clear. – Extravagant

@JonSkeet I was objecting to "lock every time". Rolling your own DCL is nothing special; if, lock, if, done, any programmer should figure this out. Yes using Lazy<T> is better (if available and possible in the circumstances). Still DCL is perfectly valid when Lazy<T> cannot be used. I actually have a helper method for it: LockableIf(Func<bool> condition, object locker, Action thenAction) – Sizeable

@Mr.TA: I think we'll have to agree to disagree. As I said in the answer, if you don't get everything absolutely right it's easy to have something that looks fine - until it doesn't actually work. The memory model is fiendishly hard to reason about (and not nearly as well specified as I'd like) - I'd rather have "correct and microscopically slower" than "fast and I guess it might be okay but I'm not actually sure". – Extravagant

Are memory barriers still a concern when the locking is async (e.g. SemaphoreSlim.WaitAsync())? I assume double-checked locking makes a lot more sense here, where acquiring the lock is more costly. – Energid

@KyleMcClellan: I don't think there's actually a good specification for memory barriers and async/await, unfortunately :( I suspect there's always a full memory barrier. – Extravagant

Answer 3

.NET 4.0 has a new type: Lazy<T> that takes away any concern about getting the pattern wrong. It's part of the new Task Parallel Library.

See the MSDN Parallel Computing Dev Center: http://msdn.microsoft.com/en-us/concurrency/default.aspx

BTW, there's a backport (I believe it is unsupported) for .NET 3.5 SP1 available here.

Roosevelt

Also there is a AsyncLazy<T> version as well which I recommend, see blogs.msdn.microsoft.com/pfxteam/2011/01/15/asynclazyt – Bisson

Exception caching of Lazy<T> ruins everything and makes result code dirty. Double-checked locking is a solution. – Swanky

Lazy always uses locks, so there's nothing to gain here. If we always want to use locks then its much cleaner to just "always use a lock". – Collazo

Answer 4

Note than in Java (and most likely in .Net as well), double-checked locking for singleton initialization is completely unnecessary as well as broken. Since classes are not initialized until they're first used, the desired lazy initialization is already achieved by this;

private static Singleton instance = new Singleton();

Unless your Singleton class contains stuff like constants that may be accessed before a Singleton instance is first used, this is all you need to do.

Galley

DCL does work since Java 5 (see Jon Skeet's comment, though he didn't talk about exactly what you must do to make it work). You need: 1. Java 5. 2. DCL reference declared volatile (or atomic in some way, e.g., using AtomicReference). – Balder

See the "Under the new Java Memory Model" section in cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html – Balder

For java Singleton and DCL patterns see this link blogs.sun.com/cwebster/entry/double_check_locking – Granville

Answer 5

I don't get why all people says that the double-check locking is bad pattern, but don't adapt the code to make it work correctly. In my opinion, this below code should work just fine.

If someone could tell me if this code suffer from the problem mentionned in Cameron's article, please do.

public sealed class Singleton {
    static Singleton instance = null;
    static readonly object padlock = new object();

    Singleton() {
    }

    public static Singleton Instance {
        get {
            if (instance != null) {
                return instance;
            }

            lock (padlock) {
                if (instance != null) {
                    return instance;
                }

                tempInstance = new Singleton();

                // initialize the object with data

                instance = tempInstance;
            }
            return instance;
        }
    }
}

Ironsmith

The reason people say it's bad is because it is incredibly easy to get wrong. I think your code needs a volatile on the instance member. (whether code works is, at least in this case, not a matter of opinion but a matter of fact. I think the fact is that it does not work, but I might be wrong on this). – Spermatophyte

@Spermatophyte There is no need for volatile in .NET. Don't forget that .NET has stronger memory model than Java, that is what confuses people a lot. The lock statement in C# (Monitor.Enter(), Monitor.Exit()) implicitely creates full memory fence (albahari.com/threading/part4.aspx). The only excuse to use volatile in C# by some is because then the double-checked locking looks the same in Java and in C# so it doesn't confuse people. My opinion is to use Lazy<T> (msdn.microsoft.com/en-us/library/dd997286(v=vs.110).aspx) – Daphie

Correct me if I'm mistaken but the issue is that you cannot guarantee the ordering of instructions inside the lock. So the instance = tempInstance assignment could be done before the instance is finished initializing, making a second call that sees a non-null value outside the lock (that's not finished initializing) unsafe. – Capps

@Salgat, IMO the compiler optimization is smart enough to create the object before assigning it so the problem you describe is very unlikely to happen, but to be more bulletproof, maybe adding a Thread.MemoryBarrier() before the assignment would do the trick. – Ironsmith

The concern is not about instruction ordering in the compiled code, but the x86 CPU reordering instructions in the instruction pipeline; but you are right, a memory barrier would prevent that from occurring before initialization is complete. – Capps

Doesn't Lazy use similar code under the hood? Why does it work there but not for a custom implementation? – Hitt

Answer 6

I've gotten double-checked locking to work by using a boolean (i.e. using a primitive to avoid the lazy initialisation):

The singleton using boolean does not work. The order of operations as seen between different threads is not guaranteed unless you go through a memory barrier. In other words, as seen from a second thread, created = true may be executed before instance= new Singleton();

Desolate

Answer 7

The issue is always the same:

A) Each line of code can consist of several different operations. For example, the statement var helper = new Helper(); involves at least 3 operations:

1. Allocate memory for the Helper class
2. Call the constructor
3. Assign reference to the variable helper.

B) You can never be sure that two operations, which are mathematically independent in a single-threaded environment, will not be reordered by the compiler. So, from

1. Allocate memory for the Helper class
2. Call the constructor
3. Assign reference to the variable helper.

it could become:

1. Allocate memory for the Helper class
2. Assign reference to the variable helper.
3. Call the constructor

The compiler checks the dependencies among them and makes optimizations on its own

C) Variables can be cached in temporary variables as long as it would result in the same outcome in a single-threaded environment.

Example:

this._helper = new Helper();
this._done = true;

could become:

var h = new Helper();
this._done = true;
this._helper = h;

I believe (like so many before me ;-) to have now found a solution for the "Broken Double Check Lock Problem":

class Foo
{
    private Helper? _helper = null;
    private int _helpersHashCode;

    public Helper GetHelper()
    {
        if (_helpersHashCode == 0 || _helper == null)
        {
            lock(this) 
            {
                if (_helpersHashCode==0)
                {
                    _helper = new Helper();
                    var hashCode = _helper.GetHashCode();
                    _helpersHashCode = hashCode == 0 ? 1 : hashCode;
                }
            }
        }
        return _helper!;
    }
}

The crucial point here is that operations are used which cannot be reordered even in a single-threaded environment, without breaking the code. And the compiler understands this and thus does not optimize the code improperly.

In detail:

The assignment to the variable _helpersHashCode occurs after the constructor call of Helper, because this assignment depends on a member call of the fully initialized object. This ensures that under no circumstances can a not fully initialized Helper object be used by another thread.

The lock ensures that only one thread can write the members of Foo.

Although it is possible that from the perspective of other threads, _helpersHashCode and _helper are set late and in random order, this would only lead to that thread having to enter the Memory Barrier (lock). After leaving the Memory Barrier, the members of Foo are definitely set.

Meritocracy

Answer 8

I don't quite understand why there are a bunch of implementation patterns on double-checked locking (apparently to work around compiler idiosyncrasies in various languages). The Wikipedia article on this topic shows the naive method and the possible ways to solve the problem, but none are as simple as this (in C#):

public class Foo
{
  static Foo _singleton = null;
  static object _singletonLock = new object();

  public static Foo Singleton
  {
    get
    {
      if ( _singleton == null )
        lock ( _singletonLock )
          if ( _singleton == null )
          {
            Foo foo = new Foo();

            // Do possibly lengthy initialization,
            // but make sure the initialization
            // chain doesn't invoke Foo.Singleton.
            foo.Initialize();

            // _singleton remains null until
            // object construction is done.
            _singleton = foo;
          }
      return _singleton;
    }
  }

In Java, you'd use synchronized() instead of lock(), but it's basically the same idea. If there's the possible inconsistency of when the singleton field gets assigned, then why not just use a locally scoped variable first and then assign the singleton field at the last possible moment before exiting the critical section? Am I missing something?

There's the argument by @michael-borgwardt that in C# and Java the static field only gets initialized once on first use, but that behavior is language specific. And I've used this pattern frequently for lazy initialization of a collection property (e.g. user.Sessions).

Vomer

The error in the analysis is this: “ _singleton remains null until object construction is done.” The compiler is free to rearrange this whole line above foo.Initialize(), or even optimize out the foo variable altogether. Even if a compiler is dumb, the CPU is smart, and may execute some memory stores done inside foo.Initialize() after the assignment _singleton = foo. The article linked to from the original post also explains other reasons this code does not do what you may think it does. – Veii

What if there is no 'initialize' call, just the constructor? Seems kind of crazy that construction of the object would not be 'complete' before the instance assignment occurs. In that case, I don't see any possible problem with the pattern. If anything is 'seeing' and instance reference before construction is complete, that would be a problem with the runtime itself. – Ricardaricardama

Answer 9

I've gotten double-checked locking to work by using a boolean (i.e. using a primitive to avoid the lazy initialisation):

private static Singleton instance;
private static boolean created;
public static Singleton getInstance() {
    if (!created) {
        synchronized (Singleton.class) {
            if (!created) {
                instance = new Singleton();
                created = true;
            }
        }
    }
    return instance;
}

Archenteron

The problem with double checked locking is that the instance could be non-null but still not initialized. Hence a thread outside of the synchronized block could pass the initial (unsynchronized) null check even though the object has not yet been initialized. The code above gets around this by checking the boolean instead. When created=true is hit we know the instance has been initialized so there is no way a thread outside the synchronized block can get an uninitialized instance. – Archenteron