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.

(1) Sorry, but why do you have that static ctor? (2) You don't need that Instance. Simply make _instance public. (3) IMHO, no, this is not thread safe. — Andre Calil
@Andre Calil: Instance is read-only. Making the _instance public will change it to writeable — zerkms
@zerkms You could have something like { get; private set; }, right? — Andre Calil
"Thread safe" is a vague term. Do you mean to ask "will this crash / throw an exception"? (Maybe, if two threads try to modify the contents of the same FooBar value.) Or do you mean "will this do what I indend it to do?" (Impossible to tell from this little information.) Thread safety isn't a matter of "using thread safe classes" or "putting locks around all shared data" (which would work but might be terrible for performance), it's a matter of having the correct (desired) behaviour in the face of concurrency. — millimoose
Personally I just wouldn't expose an entire Dictionary to other classes / threads, and instead let them work over either point-in-time snapshots; or if you don't need consistent snapshots, the enumerator of a ConcurrentDictionary. (Of course you then need to make sure clients can't or won't mutate the objects stored in the dictionary.) — millimoose

Sergey Kalinichenko Sergey Kalinichenko · Accepted Answer · 2012-07-31T00:54:18

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.

Is this example thread safe?

4 Answers