62

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?

John Smith
  • 7,243
  • 6
  • 49
  • 61
erikkallen
  • 33,800
  • 13
  • 85
  • 120

8 Answers8

85

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.

Jon Skeet
  • 1,421,763
  • 867
  • 9,128
  • 9,194
  • 1
    @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?) – IgorK Mar 24 '10 at 10:49
  • 10
    @IgorK: Yes, I *believe* it works when the variable is marked as volatile. – Jon Skeet Mar 24 '10 at 11:12
  • @Jon: OK, thank you. At least for singletons we have better solutions indeed. – IgorK Mar 24 '10 at 11:26
  • 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: http://stackoverflow.com/questions/5036518/is-it-good-to-test-a-condition-then-lock-then-re-test-the-condition ) – Myster Feb 18 '11 at 01:40
  • 2
    @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. – Jon Skeet Feb 18 '11 at 06:22
  • @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 http://www.yoda.arachsys.com/csharp/singleton.html ? – Myster Feb 20 '11 at 22:41
  • 3
    @Myster: By "take out the lock" I meant "acquire the lock". I see that it was ambiguous :) Yes, remove the outer check. – Jon Skeet Feb 20 '11 at 22:48
  • 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 http://blogs.msdn.com/b/brada/archive/2004/05/12/130935.aspx with comment `// Insure all writes...` – Chris Marisic Feb 18 '15 at 01:36
  • @ChrisMarisic: Yes, that sort of thing. – Jon Skeet Feb 18 '15 at 06:46
  • I disagree with this. "Lock every time" is a terrible approach. Never do this. – Mr. TA Aug 31 '20 at 17:54
  • @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. – Jon Skeet Aug 31 '20 at 18:24
  • @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` wraps around that so perhaps it's better to use it instead of rolling your own, but still, it's DCL. – Mr. TA Aug 31 '20 at 20:10
  • @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`, 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. – Jon Skeet Sep 01 '20 at 05:27
  • @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` is better (if available and possible in the circumstances). Still DCL is perfectly valid when `Lazy` cannot be used. I actually have a helper method for it: `LockableIf(Func condition, object locker, Action thenAction)` – Mr. TA Sep 01 '20 at 16:04
  • @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". – Jon Skeet Sep 01 '20 at 16:29
  • 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. – Kyle McClellan Sep 13 '22 at 15:57
  • 1
    @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. – Jon Skeet Sep 13 '22 at 16:06
32

.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.

David Messner
  • 383
  • 3
  • 9
  • Also there is a AsyncLazy version as well which I recommend, see https://blogs.msdn.microsoft.com/pfxteam/2011/01/15/asynclazyt/ – alastairtree Jul 20 '18 at 13:55
  • 1
    Exception caching of `Lazy` ruins everything and makes result code dirty. Double-checked locking is a solution. – it3xl Oct 28 '21 at 03:48
  • 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". – Charles Feb 04 '23 at 00:55
29

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.

Ian Kemp
  • 28,293
  • 19
  • 112
  • 138
Cameron MacFarland
  • 70,676
  • 20
  • 104
  • 133
10

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.

Michael Borgwardt
  • 342,105
  • 78
  • 482
  • 720
  • 1
    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). – C. K. Young Dec 27 '08 at 12:14
  • 1
    See the "Under the new Java Memory Model" section in http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html – C. K. Young Dec 27 '08 at 12:16
  • For java Singleton and DCL patterns see this link http://blogs.sun.com/cwebster/entry/double_check_locking – Marco Luglio Aug 01 '09 at 22:27
4

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;
        }
    }
}
M.Parent
  • 173
  • 12
  • 3
    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). – erikkallen Apr 14 '15 at 13:07
  • 9
    @erikkallen 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 (http://www.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` (https://msdn.microsoft.com/en-us/library/dd997286(v=vs.110).aspx) – Tiny Jul 15 '15 at 08:11
  • 2
    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. – Austin Salgat Sep 16 '18 at 17:57
  • @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. – M.Parent Sep 17 '18 at 14:29
  • 1
    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. – Austin Salgat Sep 17 '18 at 16:08
  • Doesn't `Lazy` use similar code under the hood? Why does it work there but not for a custom implementation? – wilmol Dec 13 '22 at 08:41
3

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();

Dimafa
  • 31
  • 2
-1

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).

Erhhung
  • 967
  • 9
  • 14
  • 7
    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. – kkm inactive - support strike Apr 19 '16 at 17:27
  • 1
    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. – Triynko May 21 '21 at 17:19
-5

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;
}
Jono
  • 908
  • 9
  • 13
  • 2
    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. – Jono Jan 15 '13 at 15:36