Is this example thread safe?

Question

Suppose I have singleton class that acts as a data cache. Multiple threads will read from the cache, and a single thread will periodically refresh it. It looks something like this:

public sealed class DataStore
{
    public static DataStore Instance { get { return _instance; } }
    public Dictionary<Foo, Bar> FooBar { get; private set; }

    static DataStore() { }
    private DataStore() { }

    public void Refresh() {
        FooBar = GetFooBarFromDB();
    }

    private static readonly DataStore _instance = new DataStore();
}

My question is essentially, is it safe to Refresh() while other threads may be accessing FooBar? Do I need to be using locks, or are my getting and setting operations atomic? Do I need to explicitly declare volatile fields to back up my properties?

P.S., If someone can think of a more descriptive title for this question, I would gladly welcome it.

Edit: Fixed my example to correct obviously non-atomic code.

Answer 1

Yes, in cases like that you need explicit synchronization, because another thread could get FooBar and start reading it before you have finished writing.

If you do this, however,

public void Refresh() {
    var tmp = new Dictionary<Foo, Bar>();
    // Fill out FooBar from DB
    FooBar = tmp;
}

then you wouldn't need to add explicit synchronization, because the switch from one reference over to the other reference is atomic.

Of course there is an implicit assumption here that there is no writing outside the Refresh method.

EDIT: You also should switch from an auto-implemented property to a manually implemented property with a backing variable declared with volatile modifier.

Answer 2

Your example is not thread-safe. Dictionary is not a thread-safe class and any thread could be reading while Refresh is being executed. You can either put a lock around or use one of the thread-safe classes like ConcurrentDictionary.

Answer 3

Well, we agree that your current code is not thread-safe. So, you must use synchronization features, because FooBar is your critical section.

If you let it be public, you are expecting that people outside the DataStore class will act accordingly. However, this is a poor design decision.

So, I would suggest you to wrap everything into your current class, with something like this: What's the best way of implementing a thread-safe Dictionary?

Answer 4

Because you are exposing the dictionary publicly then you run into more issues with the code you have written around access to the methods on the dictionary itself. As @Icarus has pointed out you should use ConcurrentDictionary but I would argue that any form of locking around the instance will not help you.

You can easily get one thread adding to the collection while another is iterating over it.

EDIT What I am saying.. never expose a static Dictionary or any other collection type. Always use the concurrent version