(invalid way to) avoid double checked locks in C#

Question

Is this a valid and optimized way to avoid double checked locks:

public class SomeBaseClass
{
     protected static object InitializeLock = new object();
     protected static bool IsInitialized = false;

     public void SomeFunction()
     {
         if (!IsInitialized)
         {
             System.Threading.Thread.MemoryBarrier();
             lock (InitializeLock)
             {
                 // do init stuff
                 IsInitialized = true;
             }
     }

     //Do stuff that have to happen when function is called
    }
}

With this being the double-checked alternative:

public class SomeBaseClass
{
     protected static object InitializeLock = new object();
     protected static bool IsInitialized = false;

     public void SomeFunction()
     {
         if (!IsInitialized)
         {
             lock (InitializeLock)
             {
                 if (!IsInitialized)
                 {                              
                     // do init stuff
                     IsInitialized = true;
                 }
             }
         }

     //Do stuff that have to happen when function is called
    }
}

yeah well I kinda asked this question to figure that out I think... The way I read the documentation was that memorybarrier made sure that no other thread could come in at the point where it was called, but I probably just misunderstood it completely... — Casper Leon Nielsen, Apr 28 '11 at 16:46
@spender it is, but first `if` is outside of it. So if field is not `volatile` it is not good. — Andrey, Apr 28 '11 at 16:52
I think I got the solution you're looking for, check my answer. — Kiril, Apr 28 '11 at 17:58

Andrey · Accepted Answer · 2011-04-28T16:51:33.207

6

No, because thread switch can happen right after two threads pass if (!IsInitialized)

There is a great article where this topic is explained in context of creating singleton: http://csharpindepth.com/Articles/General/Singleton.aspx (by Jon Skeet)

edited Apr 28 '11 at 16:51

answered Apr 28 '11 at 16:32

Andrey

59,039
12
119
163

Yeah I read that a few times already and was thinking I had come up with a whole new way of solving the issue, but of course I wasnt that clever at all ... ;) – Casper Leon Nielsen Apr 28 '11 at 16:54
@Casper Leon Nielsen it is not about being clever. I guess there is just nothing more to say on this topic :) – Andrey Apr 28 '11 at 16:58

score 5 · Answer 2 · edited May 23 '17 at 12:29

This is the second time this question has come up today. See:

C# manual lock/unlock

The short answer to your question is no, that is absolutely not valid. If the non-volatile read of "IsInitialized" is reordered with respect to the non-volatile read of whatever state is being initialized then the code path never has a memory barrier on it of any sort, and therefore the reads can be re-ordered, and therefore "IsInitialized" can be true while the out-of-date cached uninitialized state is still good.

What you have to do is either (1) don't do double-checked locking; it is dangerous, or (2) ensure that there is always at least one volatile read of IsInitialized to prevent reads of the initialized state being moved backwards in time.

score 4 · Answer 3 · answered Apr 28 '11 at 16:58

4

The MemoryBarrier call in your first example is completely superfluous since the subsequent lock call creates an implicit memory barrier anyway.

Even if you moved the memory barrier before the first IsInitialized check, the code is still unsafe: there's a window for the thread to be interrupted between the IsInitialized check and the lock statement. That's why you generally need a second IsInitialized check inside the lock block.

answered Apr 28 '11 at 16:58

LukeH

263,068
57
365
409

yours is also right and explains why better, but andrey was quicker on the trigger and he was correct too :) – Casper Leon Nielsen Apr 28 '11 at 17:02

Kiril · Answer 4 · 2011-04-28T18:00:32.193

You can help the check by making the IsInitialized flag volatile which will prevent other threads from caching it (a very minor improvement since you're locking), but you still need the flag after you're locking. In other words, you can't avoid the double-checked lock unless you use some tricky initialization.

However, you can do away with the locks if you re-design your class and if you go to an optimistic approach of changing the state... this should work like a charm:

public class Internals
{
    private readonly bool IsInitialized;

    public Internals(bool initialized)
    {
        IsInitialized = initialized;
    }
}


public class SomeBaseClass
{        
    protected static Internals internals = new Internals(false);

    public void SomeFunction()
    {
        do
        {
            Internals previous = internals;
        }while(!previous.IsInitialized && previous != Interlocked.CompareExchange(internals, new Internals(true), previous))
    }
}

(invalid way to) avoid double checked locks in C#

4 Answers4