Double checked locking in C#

Question

The idea is to get a value ("data") from

a variable
if doesn't exist, then from a file
if doesn't exist, then from a server

public static string LoadData(int id)
{
    if (_isDataLoaded[id - 1])
    {
      return _data[id - 1];
    }

    if (File.Exists(_fileName))
    {
        bool dataExists;
        string cachedData;
        if (GetCachedConstant(id, out cachedData)) //read from a file
        {
            lock (_locker)
            {
                _isDataLoaded[id - 1] = true;
                _data[id - 1] = cachedData;
            }

            return _data[id - 1];
        }
    }

    string remoteData = GetFromServer(id);
    lock (_locker)
    {
        CacheToFile(id, remoteData); //write to a file
        _data[id - 1] = remoteData;
        _isDataLoaded[id - 1] = true;
    }

    return _data[id - 1];
}

This code is used by many threads. Although it seems to be thread safe, actually it isn't. I tested it, thus, that gives me that idea and makes me sure. There should be double check of existing _data before writing. For example, it should be like double check used in pattern Singleton usually.

Someone please let me understand how do I implement it here?

EDIT:

string[] _data;
bool[] _isDataLoaded.

EDIT2:

The code above might be used with .NET < 4.0, so it's not allowed to use Lasy there. The only question I have for now, is should I use volatile in case I would use double-checked locking?

volatile string[] _data;
volatile bool[] _isDataLoaded.

What about using the `Lazy` class instead of implementing it yourself? — CodesInChaos, Nov 22 '12 at 15:57
@Alexandre `_data` is obviously not a string. It's some kind of collection that contains strings. But which collection? — CodesInChaos, Nov 22 '12 at 16:07
Could you be optimizing prematurely? There could be other areas for optimization rather than lock contention. — rro, Nov 22 '12 at 16:40
@Alexandre Have you identified that it is locking contention among your threads that is causing bottleneck in your application? If you have not, you many want to profile you application first and see if you can optimize other areas, or identify that it is really locking contention that's causing bottlenecks. — rro, Nov 24 '12 at 01:15

score 3 · Accepted Answer · edited May 23 '17 at 11:53

I can see two obvious sources of threading problems

What type is _data and _isDataLoaded? If these types are not thread safe then neither is your code arrays are only thread safe if no two threads access the same element, which could happen in the above code. Try using a ConcurrentDictionary instead.
Depending on the exact implementation of GetCachedConstant and CacheToFile there is a potential race condition whereby thread A could begin writing cached data to a file causing thread B to go down the File.Exists route when in fact the file only contains partially written data

I'd guess the culprit is the first of these two - there might be other problems but unless these types are thread safe no amount of double checked locking will save you unless you also synchronize access to those objects.

@Alexandre Arrays are only thread safe if you can ensure that no two threads access the same element, which can happen in this case. See my edit. — Justin, Nov 22 '12 at 16:16

CodesInChaos · Answer 2 · 2012-11-23T11:12:39.333

2

I'd replace _data with a Lazy<string>[], where the fetching from file&network happens in the delegate you pass in.

So in the static constructor do something like:

_data=new Lazy<string>[maxId+1];
for(int i=0;i<_data.Length;i++)
{
  _data[i]=new Lazy<string>(()=>fetchData(i), LazyThreadSafetyMode.ExecutionAndPublication);
}

and then to get the value simply _data[i].Value.

If you really want double checked locking, it basically works like this:

if (!_isDataLoaded[id - 1])
{
    lock(locker)
    {
        if(!_isDataLoaded[id - 1])
        {
            ...
            _data[id - 1] = ...;
            _isDataLoaded[id - 1] = true;
        }
    }
}
return _data[id - 1];

The problem with this code is that it's difficult to figure out if it's actually guaranteed to work. This depends on the memory model of the platform you're running on. AFAIK the .net 2.0 memory model guarantees that this works, but the ECMA CLR model and the java model don't. Memory model issues are very subtle and easy to get wrong. So I strongly recommend not using this pattern.

edited Nov 23 '12 at 11:12

answered Nov 22 '12 at 16:12

CodesInChaos

106,488
23
218
262

I don't want to use it, I want to understand how to work with double check. Lazy is an abstraction. – Alexandre Nov 22 '12 at 16:14
double locked is very subtle, and might break on some platforms. For example if you're using the EMCA memory model. I strongly recommend avoiding it, unless it's really necessary. – CodesInChaos Nov 22 '12 at 16:16
What if I would use Java, Ruby, C or something else? I think there is no Lazy there. – Alexandre Nov 23 '12 at 03:16
@Alexandre For each language/platform the answer is different, since each uses a different memory model. For example I'm not sure you can implement double checked locking in java, since their memory model is rather weak. As I said, double checked locking is subtle, and you need to know the memory model very well to be sure that it actually works reliably. – CodesInChaos Nov 23 '12 at 11:04
Thanks. But in case I would use double-checked locking, should I define data[] and dataLoaded as volatile? – Alexandre Nov 23 '12 at 11:46
An top on my question, what is fetchData()? It has to get the data from either a memory, a file or a server. So it will have to use lock anyway. Where is a benefit? – Alexandre Nov 24 '12 at 18:23
`fetchData` is the function which gets the information if it isn't loaded yet. In your case loading a file or downloading it. No matter which locking pattern you choose, it's a good idea to decouple the lazy/lock logic from the loading logic. – CodesInChaos Nov 24 '12 at 18:35

score 1 · Answer 3 · answered Nov 22 '12 at 16:02

1

Why don't you lock at the beginning of the method. That assures you that your data (cache) will always be in a valid/consistent state.

answered Nov 22 '12 at 16:02

rro

619
5
22

Only when you're retrieving from file or from the server. In both cases, all threads would have to wait anyway. – rro Nov 22 '12 at 16:06

score 1 · Answer 4 · answered Nov 22 '12 at 16:05

1

how about this, keep a single lock to load/persist data from/to the cache

public static string LoadData(int id)
{
    if (_isDataLoaded[id - 1])
      return _data[id - 1];

    lock (_locker)
    {
        if (_isDataLoaded[id - 1])
          return _data[id - 1];

        if (File.Exists(_fileName))
        {
            bool dataExists;
            string cachedData;
            if (GetCachedConstant(id, out cachedData)) //read from a file
            {
                _data[id - 1] = cachedData;
                _isDataLoaded[id - 1] = true;
                return _data[id - 1];
            }
        }

        string remoteData = GetFromServer(id);
        CacheToFile(id, remoteData); //write to a file
        _data[id - 1] = remoteData;
        _isDataLoaded[id - 1] = true;
        return _data[id - 1];
    }
}

answered Nov 22 '12 at 16:05

Christian Westman

2,985
1
26
28

Why did you remove lock before CacheToFile? – Alexandre Nov 22 '12 at 17:14
because we already have a surrounding lock making sure that only one thread is allowed inside of that block at any given time. – Christian Westman Nov 22 '12 at 17:18
Well, then why didn't you use lock at the beginning of the method? I assume it has to be there to ensure thread safe. – Alexandre Nov 24 '12 at 18:26
the read operations are thread safe, you only need the lock if you are going to alter the contents of the collections. – Christian Westman Nov 24 '12 at 18:31

score 1 · Answer 5 · answered Nov 22 '12 at 16:11

1

    lock (_locker)
    {
        _isDataLoaded[id - 1] = true;
        _data[id - 1] = cachedData;
    }

Here _isDataLoaded is set before the _data, therefore there's a race with someone seeing _isDataLoaded and reading from _data before _data is initialized.

But reversing it does not solve the problem. There's no guarantee that another thread will see the assignments in the same order, because the reader does not use any lock nor memory barriers.

Check Wikipedia about proper way to do it with C#. http://en.wikipedia.org/wiki/Double-checked_locking

answered Nov 22 '12 at 16:11

chill

16,470
2
40
44

I don't understand. There might be only one thread. Where is a problem? – Alexandre Nov 22 '12 at 17:11
First problem: one thread sets `_isDataLoaded[id-1]`. **Before** it sets `_data[id-1]` it gets preempted and another thread enters the method, sees `_isDataLoaded[id-1]` set and proceeds to access `_data[id-1]`, which is not yet initilized. Second problem: one thread sets `_data[id-1]` and `_isDataLoaded[id-1]` in that order, but another thread, on another processor, see the new value of `_isDataLoaded[id-1]`, but the old value of `_data[id-1]`. – chill Nov 22 '12 at 17:52
All right. Then I could say `lock(...)` at the very beginning of LoadData() and there will be no issue, won't it (almost like @Christian Westerman said)? – Alexandre Nov 23 '12 at 11:33
Yes, but then you won't get the benefits of a (correctly implemented) double-checked locking. – chill Nov 23 '12 at 13:02

score 0 · Answer 6 · answered Nov 22 '12 at 15:54

The idea behind double-check locking is in its name. You check your condition before lock (like in your code) but also inside the lock block again, to make sure that another thread didn't change the state (i.e. result of the condition) while the current thread was between the (successful) check and the lock statement.

Double checked locking in C#

6 Answers6