Is the volatile keyword required for this double checked lock to work?

Question

Possible Duplicate:
The need for volatile modifier in double checked locking in .NET

Given the following code snippet, which can be executed by more than one thread at once, would the volatile keyword be required to ensure that the 'real value' of connected is always read from the memory and not a cache?

(Assume that Disconnect() will only be called once (i.e. if it doesn't work properly the first time and the value of connected is read as false, it's not going to be attempted again)).

public class MyClass
{
    private readonly object syncRoot = new object();

    private bool connected; 

    public void Disconnect()
    {
        if (connected)
        {
            lock (syncRoot)
            {
                if (connected)
                {
                    // log off here
                    // ...
                    connected = false;
                }
            }
        }
    }

    public void Connect()
    {
        lock (syncRoot)
        {
            // blah
            // blah
            connected = true;
        }
    }
}

My feeling is that if double checked locking is used then it does need to be marked volatile since if it reads the incorrect value the first time, then it's going to think it's actually disconnected and won't go into the lock statement. I also think that in this case double checked locking isn't appropriate / isn't going to provide any performance gain and just a normal lock would do the job.

I'd like someone to confirm or deny these thoughts.

I've linked an article that I found very helpful to better understand synchronization issues. The short answer is that the 'lock' statement implicitly creates a memory barrier on both read and write, which is actually a stronger enforcement than volatile. http://www.albahari.com/threading/part4.aspx — Dan Bryant, Jul 12 '11 at 20:22
@Henk - thanks - it's similar, but in that question it can be a null reference or can be a reference but it's not going to be set back to null again. I'll edit this question to add in the Connect method. — GarethD, Jul 12 '11 at 20:53
I voted to reopen the question. This is *not* exactly the same as the double-checked locking pattern. The fact that this code has two different execution paths (via different methods calls) *and* that unmanaged resources which could be leaked are in play makes this *way* more complicated to analyze. I am not convinced that `volatile` makes this code safe. — Brian Gideon, Jul 13 '11 at 14:12
@Brian: The volatile is not even needed. The `lock` and the initial value (false) are sufficient. The 2 code-paths make it only a slight variation. — H H, Jul 13 '11 at 22:36

score 1 · Answer 1 · edited May 23 '17 at 12:10

1

It is my understanding of volatile that yes you would want it. I believe volatile will enforce that the value is read each time and would preclude the compiler caching the value or performing some other optimization that would cause the "correct" value not to be read from memory.

See Eric Lippert's answer where he mentions the use of volatile reads.

edited May 23 '17 at 12:10

Community

1
1

answered Jul 12 '11 at 20:09

pstrjds

16,840
6
52
61

No, You [don't want to use volatile](http://www.bluebytesoftware.com/blog/PermaLink,guid,a0484627-c752-45f6-a2ac-414130cb3d2f.aspx) – H H Jul 12 '11 at 20:14
@Henk could you explain why you don't want to use volatile (note: the link you provided is broken), I know it is hard to get it correct, but I was under the impression in this case you did want to use it. – pstrjds Jul 12 '11 at 20:25
`volatile` is (unofficially) deprecated. `lock` provides all the barriers needed here (See @Dan's comment). The link is OK but that site is now down. Also see http://stackoverflow.com/questions/6164466/a-reproducable-example-of-volatile-usage/6164770#6164770 – H H Jul 12 '11 at 20:30
@Henk I did not see any comments from @Dan there, but the write up from @Hans is a good read. So basically the lock is guaranteeing that the reads and writes don't get reordered. But what if there is double checked locking in both Connect and Disconnect, seems like a compiler causing a cache of the bool to happen could cause a deadlock OR cause a connection to either remain open or not be opened in the future. – pstrjds Jul 12 '11 at 20:43
The comment is under the question. Your deadlock/leak scenario would require (at least) dcl in Connect() and an erroneous start value for connected. – H H Jul 12 '11 at 20:50

H H · Answer 2 · 2011-07-12T20:18:11.073

1

Provided the Connect() method also uses a lock (syncRoot) the code should work without volatile.

But you're not going to see any benefit from dcl here, why go through the trouble/risks?

edited Jul 12 '11 at 20:18

answered Jul 12 '11 at 20:12

H H

263,252
30
330
514

Thanks. Can you elaborate please - "the code should work" - why exactly should the code work and why isn't volatile required? NB that the first check of the connected variable is not inside a lock(). – GarethD Jul 12 '11 at 20:29
@Gareth: But it is changed inside a `lock` in Connect(), right? That's enough. – H H Jul 12 '11 at 20:33
Yes the Connect method also sets it inside a lock, but my feeling is that's not going to help if something else reads it without a lock? – GarethD Jul 12 '11 at 20:38
@Gareth: follow the various links, but basically the lock in Connect() will ensure that Disconnect() will not see a stale value. – H H Jul 12 '11 at 20:48
I can't see where that's stated. Let's assume thread A is calling `Connect()`, it is using the `syncRoot` object for synchronisation. Thread B calls `Disconnect()`, the first read of `connected` is not inside a lock. Assume it reads the value (incorrectly) as false, the method will simply return. My understanding of Monitor.Enter/Exit is the threads must cooperate. Maybe it would be different if `MyClass` itself was used to sync. Could you perhaps edit your answer and quote the specific section of the article(s) where this is articulated? – GarethD Jul 12 '11 at 21:05
@Gareth : see the [Albahari article](http://www.albahari.com/threading/part4.aspx), especially the green box: Do We Really Need Locks and Barriers? Read the last sentence. – H H Jul 12 '11 at 21:11
i see - great thanks, I tested that and got the results he describes. – GarethD Jul 12 '11 at 21:40
@HenkHolterman let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/1387/discussion-between-gareth-down-and-henk-holterman) – GarethD Jul 12 '11 at 21:40

Dominic Zukiewicz · Answer 3 · 2011-07-12T20:15:04.173

-2

You are correct, the volatile keyword reads from memory and not the CPU cache. But it does not mean it is thread safe. Change the syncLock variable to a static modifier to ensure this is thread-safe across multiple instances. This is the recommended approach.

edited Jul 12 '11 at 20:15

answered Jul 12 '11 at 20:06

Dominic Zukiewicz

8,258
8
43
61

1

It's guaranteed there's only one instance, and that wasn't the question anyway. – GarethD Jul 12 '11 at 20:08
Can you add a question mark where the question was ... ? – Dominic Zukiewicz Jul 12 '11 at 20:10
Also, the value being compared is only a single instance, if the value was static then you would want the static for the lock. – pstrjds Jul 12 '11 at 20:10
Also, I'm assuming the Disconnect() method is the multi-threaded method. If you can absolutely guarantee that this class will never ever ever have more than 1 instance, you are fine. Otherwise just add static to your sync root and you are forever safe, because as it stands, it is not thread safe. – Dominic Zukiewicz Jul 12 '11 at 20:12
1

`synRoot` should be exactly as static as `connected`. – H H Jul 12 '11 at 20:15
1

@Dominic - The first sentence is terminated with a question mark. I don't care about the fact that the syncRoot variable is not static - that is not the question. – GarethD Jul 12 '11 at 20:21
2

Where precisely in section 10.5.3 of the C# specification does it state that "volatile" guarantees "reads from memory and not the CPU cache"? I see no evidence whatsoever that this claim is correct. – Eric Lippert Jul 12 '11 at 22:10
@Eric, reading from memory would not be a smart thing to do :) (lest imposed by spec) – bestsss Jul 12 '11 at 22:30
@Dominic, unless by "CPU cache" you mean a register, the statement may not be true. – bestsss Jul 12 '11 at 22:32

Is the volatile keyword required for this double checked lock to work?

3 Answers3