C# usage of lock() statements and caching of data

Question

I have a number of static List's in my application, which are used to store data from my database and are used when looking up information:

public static IList<string> Names;

I also have some methods to refresh this data from the database:

public static void GetNames()
{
    SQLEngine sql = new SQLEngine(ConnectionString);
    lock (Names)
    {
        Names = sql.GetDataTable("SELECT * FROM Names").ToList<string>();
    }
}

I initially didnt have the lock() in place, however i noticed very occasionally, the requesting thread couldnt find the information in the list. Now, I am assuming that if the requesting thread tries to access the Names list, it cant until it has been fully updated.

Is this the correct methodology and usage of the lock() statement?

As a sidenote, i noticed on MSDN that one shouldnt use lock() on public variables. Could someone please elaborate in my particular scenario?

It is a recommended practise to use separate dedicated object for locking not the where itself — Haris Hasan, Feb 16 '12 at 07:30
http://stackoverflow.com/questions/230716/difference-between-locklocker-and-lockvariable-which-i-am-using — Haris Hasan, Feb 16 '12 at 07:41

jgauffin · Answer 1 · 2012-02-16T07:44:21.763

3

No, it's not correct since anyone can use the Names property directly.

public class SomeClass
{
    private List<string> _names;
    private object _namesLock = new object();

    public IEnumerable<string> Names
    {
        get
        {
            if (_names == null)
            {
                lock (_namesLock )
                {
                    if (_names == null)
                        _names = GetNames();
                }
            }

            return _names;
        }
    }

    public void UpdateNames()
    {
        lock (_namesLock)
            GetNames();
    }

    private void GetNames()
    {
        SQLEngine sql = new SQLEngine(ConnectionString);
        _names = sql.GetDataTable("SELECT * FROM Names").ToList<string>();
    }   
}

Try to avoid static methods. At least use a singleton.

The check, lock, check is faster than a lock, check since the write will only occur once.

Assigning a property on usage is called lazy loading.

The _namesLock is required since you can't lock on null.

edited Feb 16 '12 at 07:44

answered Feb 16 '12 at 07:30

jgauffin

99,844
45
235
372

Pretty solid - but suppose he wanted to update Names without it being null - just wanted to refresh the data? – Prescott Feb 16 '12 at 07:39
there's nothing wrong with static methods, when used appropriately. One problem with this approach is that it is castable back to a mutable list - I've seen people cause mayhem with this, mainly accidentally by data-binding the enumerable (which is also a list). The data-binding doesn't care (it only knows `object`), and happily mutates it. – Marc Gravell Feb 16 '12 at 07:46
Double locking needs a memory barrier like `volatile _names`. The code above could return `null` – Daniel Doubleday Mar 03 '22 at 18:11

score 3 · Accepted Answer · answered Feb 16 '12 at 07:37

3

lock is only useful if all places intended to be synchronized also apply the lock. So every time you access Names you would be required to lock. At the moment, that only stops 2 threads swapping Names at the same time, which frankly isn't a problem here anyway, as reference swaps are atomic anyway.

Another problem; presumably Names starts off null? You can't lock a null. Equally, you shouldn't lock on something that may change reference. If you want to synchronize, a common approach is something like:

// do not use for your scenario - see below
private static readonly object lockObj = new object();

then lock(lockObj) instead of your data.

With regards to not locking things that are visible externally; yes. That is because some other code could randomly choose to lock on it, which could cause unexpected blocking, and quite possibly deadlocks.

The other big risk is that some of your code obtains the names, and then does a sort/add/remove/clear/etc - anything that mutates the data. Personally, I would be using a read-only list here. In fact, with a read-only list, all you have is a reference swap; since that is atomic, you don't need any locking:

public static IList<string> Names { get; private set; }
public static void UpdateNames() {
    List<string> tmp = SomeSqlQuery();
    Names = tmp.AsReadOnly();
}

And finally: public fields are very very rarely a good idea. Hence the property above. This will be inlined by the JIT, so it is not a penalty.

answered Feb 16 '12 at 07:37

Marc Gravell

1,026,079
266
2,566
2,900

+1 Thanks. I probably should have showed more code, Names is actually instantiated by default. Hmm, some of your comments actually lead me to another question.... – Simon Feb 16 '12 at 07:48
+++1 (if i could) Cant beleive i missed that. The {get; private set; } is exactly what i need. Would i need to lock() as well if the dat is only updated in the static method? – Simon Feb 16 '12 at 08:03
@Simon that depends; if you are just swapping a **reference** (as per the example I show), then actually: no - since a reference swap is atomic. If you are going to mutate (edit the contents) a list that everyone has access to, then **everyone** needs to lock. The lock might be handy to make sure that you don't have 7 threads swapping it at the same time, but: if you do that, you *probably* actually want to use `Monitor.TryEnter` with a 0ms timeout, as it is *not* the case that you want 7 threads updating it in a row - you just want 1 thread to update it, and the rest to not bother. – Marc Gravell Feb 16 '12 at 08:16
Thanks Marc, I am simply swapping a reference (re-reading the data from the DB). The data is only ever refreshed in its entirety (no editing/deleting/inserting). Looks like i will be ok without. – Simon Feb 16 '12 at 08:19
@Simon - the important thing, then, is (when updating): create a **new** list, and insert the data to whatever you want; and **then** when it is "final", swap the reference (ideally making it read-only at the same time). Don't swap it until the contents are fully stable. – Marc Gravell Feb 16 '12 at 08:22
Thanks, yes my DataTable extension method is defined `where T : new()` so i should be fine. I will ensure to store in local variable then swap the reference (again this example was simplified) – Simon Feb 16 '12 at 08:30

score 0 · Answer 3 · answered Feb 16 '12 at 07:36

From the oode you have shown, the first time GetNames() is called the Names property is null. I don't known what a lock on a null object would do. I would add a variable to lock on.

 static object namesLock = new object();

Then in GetNames()

 lock (namesLock)
 {
      if (Names == null)
        Names = ...;
 }

We do the if test inside of the lock() to stop race conditions. I'm assuming that the caller of GetNames() also does the same test.

C# usage of lock() statements and caching of data

3 Answers3