Should this class use data locking for multi threading?

Question

I have a class that contains some data and there are many threads use it:

class MyClass
{
    static Dictionary<Key, Value> MyData;

    static IEnumerable<Data> Data
    {
        get
        {
            return MyData.Values;
        }
    }

    static void Reset()
    {
         MyData = GetMyData();
    }
}

Sometime (say once in a day) the Reset method is called. I don't want to add locking because of performance, but not sure if everything will work without it.

The question is: should I use any type of locking in this class?

Answer 1

If it is going to be called by multiple threads then: yes.

A Monitor (the type that a lock statement uses) has very low overhead if there is little contention. Create a private Object instance to use with the lock and ensure all access paths are protected.

Another possibility (on .NET 4) is a ConcurrentDictionary

Edit Additional: I note that the Data property returns an IEnumerable over the internal content of your Dictionary. This means that an iteration over the dictionary's values could be ongoing when other modifying methods are called. Even with internal locking you would have concurrency problems. There are two approaches:

1. Move the locking into the callers of this type. If it is an internal or helper type (rather than exposed as part of an API) this could be a workable approach, but it makes the burden of ensuring correct locking more difficult.

2. Make a copy of the values, and return that:

   static IEnumerable<Data> Data {
     get {
         return MyData.Values.ToArray();
     }
   }
   

Answer 2

I disagree with Richard. You seem to use the Dictionary<,> in an immutable fashion, never changing the content. It is returned completely populated from the GetMyData() method and simply replaces a reference on MyData, while only exposing the immutable Values property. Even if someone is enumerating over the dictionary while someone else calls Reset(), the person will continue enumerating over the old dictionary while any new reader will get the new dictionary. So your code is completely thread-safe, thanks to the fact that that you use data structures as if they were immutable. You might want to wrap it in a real immutable dictionary (e.g. from here) If, on the other hand, there are different usages in other parts of the code that we can't see, then it would be a different matter.

EDIT: I just noticed that both MyData and Data are private (then why even bother with an extra property?), so you're also might be using MyData to modify the dictionary. In that case, as long as all the dictionary read/writes happen on the same thread, but the Reset() method is called from a different thread, then you're okay. If, on the other hand, you're modifying the collection on one thread while reading from it on another thread, then you'll have to perform manual synchronization as Richard mentioned. The point is that the Reset() method is never the issue, since it replaces the reference of the dictionary with a new instance without affecting the old instance.

Answer 3

Yes. You should make the code safe for multithreaded operations. The reason is because it is standard practice to make all static members thread-safe whether you expect the code to be run in multithreaded environment or not. Your current code has a staleness problem. That is one thread may call Reset, but other threads calling Data may never see the change. This can be fixed easily by marking MyData as volatile.

Update:

The issue of staleness that I am talking about is related to how the C# and JIT compilers optimize code. For example, consider two threads T_A and T_B. T_A calls MyClass.Data and thus reads the MyData reference while T_B changes the MyData reference by calling Reset. The compiler is free to reorder the reads and writes of MyData by lifting them outside of loops, caching them in a CPU register, etc. What this means is that T_A might miss a change in MyData if it caches the reference in a CPU register or if T_B does not commit its write to main memory immediately.

This is not just some theoretical problem that rarely occurs in practice. In fact, it is quite easy to demonstrate with the following program. Make sure you compile the code in the Release configuration and run it without the debugger (both are mandatory to reproduce the problem). A cursory glance suggests that this program should terminate in about 1 second, but alas it does not because m_Stop is being cached by the worker thread and never sees that the main thread changed its value to true. The simple fix is mark m_Stop as volatile. You can see my explanation here for information about the concept of memory barriers and the reordering of instructions.

public class Program
{
    private static bool m_Stop = false;

    public static void Main(string[] args)
    {
        var thread = new Thread(
            () =>
            {
                int i = 0;
                Console.WriteLine("begin");
                while (!m_Stop)
                {
                    i++;
                }
                Console.WriteLine("end");
            });
        thread.Start();
        Thread.Sleep(1000);
        m_Stop = true;
        Console.WriteLine("exit");
    }
}