views:

4246

answers:

16

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?

A: 

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.

Keith
+3  A: 

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.

John.

John Richardson
That's no better, you might as well remove the statement outside the lock.Also, if the only thing that is done here is to set initialized to true then the original "non-safe" ver. was good enough. At worst you set it twice, the only reason for the early return is performance (not correctness).
Wedge
I did say that the first check is for performance. Locks are very expensive so it's always worth doing. On the second point, I think it's reasonable to assume more complex code has been omitted. I somehow doubt MS would go to the expense of a lock unnecessarily.
John Richardson
A: 

@Keith
The "Perform initialization" comment is standing in for all the config-reading, class-instantiating, and settings-setting that Membership does to initialize s_Provider, so I understand why that's in a "lock" - so that it's only done once.

@littlegeek
I think you're looking at the MembershipProvider class. My example comes from System.Web.Security.Membership, which descends directly from Object.

Rory MacLeod
A: 

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 initialision. You would need to check the sInitialized flag inside the lock as well as outside, to prevent a race condition in which multiple threads read the sInitialized flag before any of them actually does the initialisation code. eg.

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

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

[Edit: John got this before me]

olliej
A: 

@John Richardson
You're right. The real Membership class has that second check. I left it out because what I'm really interested in is whether the first call to s_Initialized, outside the lock, is thread-safe.

Rory MacLeod
A: 

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

I would have guessed that their not atomic.

Peteter
+1  A: 

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.

John.

John Richardson
+1  A: 

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!

OJ
+1  A: 

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

Leon Bambrick
A: 

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.

JoshL
+18  A: 

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

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

John.

John Richardson
Though accessing a memory location no larger than the native word size is atomic, the sample code provided in the question is not thread safe because of read/write reordering. See my answer for more details.
Thomas Danecker
Good spot, Thomas. Everyone should read be sure to read your answer as well.
John Richardson
+1  A: 

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.

John.

John Richardson
+4  A: 

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.

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

Rory MacLeod
It's not correct.See my Answer for the reason why.
Thomas Danecker
I see your point, but the code in my example is accurate with respect to how the locking is done in the actual Membership class. Perhaps the ASP.NET team know enough about their compiler and their targeted processor to be confident that no reordering is going to occur. All the same...
Rory MacLeod
...I agree with Joe Duffy in the article you linked to when he says that we should always use "volatile" in situations like this just to be on the safe side.
Rory MacLeod
If "int" is atomic, why does Interlocked.Increment()/Decrement() accept int types?
Chris
because Interlocked.Increment does the load, increment and store in one atomic operation.
Thomas Danecker
+5  A: 

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

Leon Bambrick
public struct WTFBoolean {private UInt64 value; public static implicit operator bool (WTFBoolean wtfbool) {return wtfbool.value == 0xFFFFFFFFFFFFFFFF;}}
Triynko
+1  A: 

@Leon
I see your point - the way I've asked, and then commented on, the question allows it to be taken in a couple of different ways.

To be clear, I wanted to know if it was safe to have concurrent threads read and write to a boolean field without any explicit synchronization code, i.e., is accessing a boolean (or other primitive-typed) variable atomic.

I then used the Membership code to give a concrete example, but that introduced a bunch of distractions, like the double-check locking, the fact that s_Initialized is only ever set once, and that I commented out the initialization code itself.

My bad.

Rory MacLeod
+12  A: 

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 uninitalized data can be reordered (ie. prefetched by the cpu), so if s_Initalized is true, reading the data that should be initialized can result in reading old, uninitalized 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_Initalized 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

Thomas Danecker
Good point. Reading the article you linked to, whether the code in Membership is right or wrong seems to depend on the precise details of the compiler and the processor, but I agree that s_Initialized should be made volatile just to be sure.
Rory MacLeod
It *is* wrong according to .net's Memory Modell. On certain processors and/or with certain compilers, you may be lucky enought that it still works.Joe Duffy wrote another article: http://www.bluebytesoftware.com/blog/2008/07/17/LoadsCannotPassOtherLoadsIsAMyth.aspx
Thomas Danecker
The absence of write reordering is a characteristic of x86 and x64 but not necessarily Microsoft's .NET, i.e. I would not rely upon it if Microsoft add support for other architectures where the hardware does reorder writes.
Jon Harrop