locking only when modifying vs entire method

Question

When should locks be used? Only when modifying data or when accessing it as well?

public class Test {
    static Dictionary<string, object> someList = new Dictionary<string, object>();

    static object syncLock = new object();

    public static object GetValue(string name) {
        if (someList.ContainsKey(name)) {
            return someList[name];
        } else {
            lock(syncLock) {
                object someValue = GetValueFromSomeWhere(name);
                someList.Add(name, someValue);
            }
        }
    }
}

Should there be a lock around the the entire block or is it ok to just add it to the actual modification? My understanding is that there still could be some race condition where one call might not have found it and started to add it while another call right after might have also run into the same situation - but I'm not sure. Locking is still so confusing. I haven't run into any issues with the above similar code but I could just be lucky so far. Any help above would be appriciated as well as any good resources for how/when to lock objects.

Answer 1

You have to lock when reading too, or you can get unreliable data, or even an exception if a concurrent modification physically changes the target data structure.

In the case above, you need to make sure that multiple threads don't try to add the value at the same time, so you need at least a read lock while checking whether it is already present. Otherwise multiple threads could decide to add, find the value is not present (since this check is not locked), and then all try to add in turn (after getting the lock)

You could use a ReaderWriterLockSlim if you have many reads and only a few writes. In the code above you would acquire the read lock to do the check and upgrade to a write lock once you decide you need to add it. In most cases, only a read lock (which allows your reader threads to still run in parallel) would be needed.

There is a summary of the available .Net 4 locking primitives here. Definitely you should understand this before you get too deep into multithreaded code. Picking the correct locking mechanism can make a huge performance difference.

You are correct that you have been lucky so far - that's a frequent feature of concurrency bugs. They are often hard to reproduce without targeted load testing, meaning correct design (and exhaustive testing, of course) is vital to avoid embarrassing and confusing production bugs.