CODEWITHSUNDEEP
c#.netmultithreadingthread-safety
Eric
Eric

Reputation: 6016

Is this example thread safe?

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.

Upvotes: 3

Views: 1713

Answers (4)

Andre Calil
Andre Calil

Reputation: 7692

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?

Upvotes: 1

aqwert
aqwert

Reputation: 10789

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

Upvotes: 1

Sergey Kalinichenko
Sergey Kalinichenko

Reputation: 727047

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.

Upvotes: 8

Icarus
Icarus

Reputation: 63970

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.

Upvotes: 2

Related Questions