65
votes

I've been raised to believe that if multiple threads can access a variable, then all reads from and writes to that variable must be protected by synchronization code, such as a "lock" statement, because the processor might switch to another thread halfway through a write.

However, I was looking through System.Web.Security.Membership using Reflector and found code like this:

public static class Membership
{
    private static bool s_Initialized = false;
    private static object s_lock = new object();
    private static MembershipProvider s_Provider;

    public static MembershipProvider Provider
    {
        get
        {
            Initialize();
            return s_Provider;
        }
    }

    private static void Initialize()
    {
        if (s_Initialized)
            return;

        lock(s_lock)
        {
            if (s_Initialized)
                return;

            // Perform initialization...
            s_Initialized = true;
        }
    }
}

Why is the s_Initialized field read outside of the lock? Couldn't another thread be trying to write to it at the same time? Are reads and writes of variables atomic?

15
For those of you reading this post as it currently exists, it is a little confusing. The code above has been adjusted to display a correct way to double check with a lock (inside & outside). However, the question text refers to the initial code sample which did not include the second s_initialized check. It's a bit confusing without looking at the edit history.GEEF
@GEEF Sorry for the confusion, but the question DOES refer to the current code, including the second check inside the lock. This is not a question about how to do double-check locking, it's about atomic memory access, and whether a boolean could be written to at the same time as it's being read.Rory MacLeod

15 Answers

37
votes

For the definitive answer go to the spec. :)

Partition I, Section 12.6.6 of the CLI spec states: "A conforming CLI shall guarantee that read and write access to properly aligned memory locations no larger than the native word size is atomic when all the write accesses to a location are the same size."

So that confirms that s_Initialized will never be unstable, and that read and writes to primitve types smaller than 32 bits are atomic.

In particular, double and long (Int64 and UInt64) are not guaranteed to be atomic on a 32-bit platform. You can use the methods on the Interlocked class to protect these.

Additionally, while reads and writes are atomic, there is a race condition with addition, subtraction, and incrementing and decrementing primitive types, since they must be read, operated on, and rewritten. The interlocked class allows you to protect these using the CompareExchange and Increment methods.

Interlocking creates a memory barrier to prevent the processor from reordering reads and writes. The lock creates the only required barrier in this example.

34
votes

This is a (bad) form of the double check locking pattern which is not thread safe in C#!

There is one big problem in this code:

s_Initialized is not volatile. That means that writes in the initialization code can move after s_Initialized is set to true and other threads can see uninitialized code even if s_Initialized is true for them. This doesn't apply to Microsoft's implementation of the Framework because every write is a volatile write.

But also in Microsoft's implementation, reads of the uninitialized data can be reordered (i.e. prefetched by the cpu), so if s_Initialized is true, reading the data that should be initialized can result in reading old, uninitialized data because of cache-hits (ie. the reads are reordered).

For example:

Thread 1 reads s_Provider (which is null)  
Thread 2 initializes the data  
Thread 2 sets s\_Initialized to true  
Thread 1 reads s\_Initialized (which is true now)  
Thread 1 uses the previously read Provider and gets a NullReferenceException

Moving the read of s_Provider before the read of s_Initialized is perfectly legal because there is no volatile read anywhere.

If s_Initialized would be volatile, the read of s_Provider would not be allowed to move before the read of s_Initialized and also the initialization of the Provider is not allowed to move after s_Initialized is set to true and everything is ok now.

Joe Duffy also wrote an Article about this problem: Broken variants on double-checked locking

12
votes

Hang about -- the question that is in the title is definitely not the real question that Rory is asking.

The titular question has the simple answer of "No" -- but this is no help at all, when you see the real question -- which i don't think anyone has given a simple answer to.

The real question Rory asks is presented much later and is more pertinent to the example he gives.

Why is the s_Initialized field read outside of the lock?

The answer to this is also simple, though completely unrelated to the atomicity of variable access.

The s_Initialized field is read outside of the lock because locks are expensive.

Since the s_Initialized field is essentially "write once" it will never return a false positive.

It's economical to read it outside the lock.

This is a low cost activity with a high chance of having a benefit.

That's why it's read outside of the lock -- to avoid paying the cost of using a lock unless it's indicated.

If locks were cheap the code would be simpler, and omit that first check.

(edit: nice response from rory follows. Yeh, boolean reads are very much atomic. If someone built a processor with non-atomic boolean reads, they'd be featured on the DailyWTF.)

7
votes

The correct answer seems to be, "Yes, mostly."

  1. John's answer referencing the CLI spec indicates that accesses to variables not larger than 32 bits on a 32-bit processor are atomic.
  2. Further confirmation from the C# spec, section 5.5, Atomicity of variable references:

Reads and writes of the following data types are atomic: bool, char, byte, sbyte, short, ushort, uint, int, float, and reference types. In addition, reads and writes of enum types with an underlying type in the previous list are also atomic. Reads and writes of other types, including long, ulong, double, and decimal, as well as user-defined types, are not guaranteed to be atomic.

  1. The code in my example was paraphrased from the Membership class, as written by the ASP.NET team themselves, so it was always safe to assume that the way it accesses the s_Initialized field is correct. Now we know why.

Edit: As Thomas Danecker points out, even though the access of the field is atomic, s_Initialized should really be marked volatile to make sure that the locking isn't broken by the processor reordering the reads and writes.

2
votes

The Initialize function is faulty. It should look more like this:

private static void Initialize()
{
    if(s_initialized)
        return;

    lock(s_lock)
    {
        if(s_Initialized)
            return;
        s_Initialized = true;
    }
}

Without the second check inside the lock it's possible the initialisation code will be executed twice. So the first check is for performance to save you taking a lock unnecessarily, and the second check is for the case where a thread is executing the initialisation code but hasn't yet set the s_Initialized flag and so a second thread would pass the first check and be waiting at the lock.

1
votes

Reads and writes of variables are not atomic. You need to use Synchronisation APIs to emulate atomic reads/writes.

For an awesome reference on this and many more issues to do with concurrency, make sure you grab a copy of Joe Duffy's latest spectacle. It's a ripper!

1
votes

"Is accessing a variable in C# an atomic operation?"

Nope. And it's not a C# thing, nor is it even a .net thing, it's a processor thing.

OJ is spot on that Joe Duffy is the guy to go to for this kind of info. ANd "interlocked" is a great search term to use if you're wanting to know more.

"Torn reads" can occur on any value whose fields add up to more than the size of a pointer.

1
votes

An If (itisso) { check on a boolean is atomic, but even if it was not there is no need to lock the first check.

If any thread has completed the Initialization then it will be true. It does not matter if several threads are checking at once. They will all get the same answer, and, there will be no conflict.

The second check inside the lock is necessary because another thread may have grabbed the lock first and completed the initialization process already.

1
votes

You could also decorate s_Initialized with the volatile keyword and forego the use of lock entirely.

That is not correct. You will still encounter the problem of a second thread passing the check before the first thread has had a chance to to set the flag which will result in multiple executions of the initialisation code.

1
votes

I think you're asking if s_Initialized could be in an unstable state when read outside the lock. The short answer is no. A simple assignment/read will boil down to a single assembly instruction which is atomic on every processor I can think of.

I'm not sure what the case is for assignment to 64 bit variables, it depends on the processor, I would assume that it is not atomic but it probably is on modern 32 bit processors and certainly on all 64 bit processors. Assignment of complex value types will not be atomic.

0
votes

I thought they were - I'm not sure of the point of the lock in your example unless you're also doing something to s_Provider at the same time - then the lock would ensure that these calls happened together.

Does that //Perform initialization comment cover creating s_Provider? For instance

private static void Initialize()
{
    if (s_Initialized)
        return;

    lock(s_lock)
    {
        s_Provider = new MembershipProvider ( ... )
        s_Initialized = true;
    }
}

Otherwise that static property-get's just going to return null anyway.

0
votes

Perhaps Interlocked gives a clue. And otherwise this one i pretty good.

I would have guessed that their not atomic.

0
votes

To make your code always work on weakly ordered architectures, you must put a MemoryBarrier before you write s_Initialized.

s_Provider = new MemershipProvider;

// MUST PUT BARRIER HERE to make sure the memory writes from the assignment
// and the constructor have been wriitten to memory
// BEFORE the write to s_Initialized!
Thread.MemoryBarrier();

// Now that we've guaranteed that the writes above
// will be globally first, set the flag
s_Initialized = true;

The memory writes that happen in the MembershipProvider constructor and the write to s_Provider are not guaranteed to happen before you write to s_Initialized on a weakly ordered processor.

A lot of thought in this thread is about whether something is atomic or not. That is not the issue. The issue is the order that your thread's writes are visible to other threads. On weakly ordered architectures, writes to memory do not occur in order and THAT is the real issue, not whether a variable fits within the data bus.

EDIT: Actually, I'm mixing platforms in my statements. In C# the CLR spec requires that writes are globally visible, in-order (by using expensive store instructions for every store if necessary). Therefore, you don't need to actually have that memory barrier there. However, if it were C or C++ where no such guarantee of global visibility order exists, and your target platform may have weakly ordered memory, and it is multithreaded, then you would need to ensure that the constructors writes are globally visible before you update s_Initialized, which is tested outside the lock.

0
votes

What you're asking is whether accessing a field in a method multiple times atomic -- to which the answer is no.

In the example above, the initialise routine is faulty as it may result in multiple initialization. You would need to check the s_Initialized flag inside the lock as well as outside, to prevent a race condition in which multiple threads read the s_Initialized flag before any of them actually does the initialisation code. E.g.,

private static void Initialize()
{
    if (s_Initialized)
        return;

    lock(s_lock)
    {
        if (s_Initialized)
            return;
        s_Provider = new MembershipProvider ( ... )
        s_Initialized = true;
    }
}
-1
votes

Ack, nevermind... as pointed out, this is indeed incorrect. It doesn't prevent a second thread from entering the "initialize" code section. Bah.

You could also decorate s_Initialized with the volatile keyword and forego the use of lock entirely.