.NET4.0: Thread-Safe Manner of Updating A Dictionary and Its Values

Question

I have a static Dictionary that I want to safely update. Initially, the dictionary will be empty, but over the lifetime of app, it will have new values added to it. Also, the integer values will act as individual counters that can increment and decrement.

private static Dictionary<string, int> foo = new Dictionary<string, int>();

public static void Add(string bar)
{
    if (!foo.ContainsKey(bar))
        foo.Add(bar, 0);

    foo[bar] = foo[bar] + 1;
}


public static void Remove(string bar)
{
    if (foo.ContainsKey(bar))
    {
        if (foo[bar] > 0)
            foo[bar] = foo[bar] - 1;
    }
}

I've been reading up on the Interlocked class as a means to provide thread-safety, and it appears that this may be something that I can use:

public static void Add(string bar)
{
    if (!foo.ContainsKey(bar))
        foo.Add(bar, 0);

    Interlocked.Increment(ref foo[bar]);
}


public static void Remove(string bar)
{
    if (foo.ContainsKey(bar))
    {
        if (foo[bar] > 0)
            Interlocked.Decrement(ref foo[bar]);
    }
}

However, my gut instinct is that this won't solve issues when I'm actually adding new items to the Dictionary, so lock immediately comes to mind:

private static Dictionary<string, int> foo = new Dictionary<string, int>();
private static object myLock = new object();

public static void Add(string bar)
{
    lock(myLock)
    {
        if (!foo.ContainsKey(bar))
            foo.Add(bar, 0);

        Interlocked.Increment(ref foo[bar]);
    }
}


public static void Remove(string bar)
{
    lock(myLock)
    {
        if (foo.ContainsKey(bar))
        {
            if (foo[bar] > 0)
                Interlocked.Decrement(ref foo[bar]);
        }
    }
}

Is this approach ideal, or even correct? Can it be improved? Am I way off?

Answer 1

lock is good (and necessary; you were right to suspect that Interlocked would not suffice) but once you do that the Interlocked.Increment and Interlocked.Decrement are unnecessary. The issue with accessing a Dictionary<TKey, TValue> from multiple threads is that one thread could trigger a rebuild of the internal hashtable, then that thread gets swapped out mid-rebuild for another thread which now comes along and adds to the dictionary wreaking havoc on the internal structure of the dictionary.

Additionally, your implementation is good in that you lock on a private object rather than this. As chibacity pointed out, this lock object should be readonly though.

Be careful that you don't trick yourself into thinking that you've now made your dictionary bullet-proof in multi-threaded scenarios. For example, the following could happen:

Thread 1 looks up string "Hello, World!" in the dictionary, receives back the count 1.

Thread 1 is swapped out for Thread 2.

Thread 2 calls remove with key "Hello, World!" setting the count to 0.

Thread 2 is swapped out for Thread 1.

Thread 1 now thinks that the count is 1 but it is actually 0.

Finally, in .NET 4.0, you should consider using ConcurrentDictionary<TKey, TValue>. Note that this effectively eliminates the need to lock when calling instance-methods on the dictionary, but it does not eliminate the above scenario from occurring.

Answer 2

If you are using .Net 4.0 you could think about using collections from System.Collections.Concurrent namespace, for example ConcurrentDictionary<TKey,TValue> might work for you.

Answer 3

As you have guarded multi-threaded access to your dictionary in the Add and Remove methods with a lock, the interlocked statements are unnecessary as this code sits within the scope of those locks. All code within the lock blocks is now guaranteed to be single-threaded and so safe.

A small point, but you should mark your myLock object as readonly, as currently it is possible to change this object by reassigning it. You could therefore change the object whilst one thread had a lock on it, and a subsequent thread would then see a different lock object and so could ignore the previous threads lock. Marking the object as readonly makes it immutable.

Answer 4

In your case the add\remove operations on the dictionary have to be thread safe. This is because if you are enumerating a dictionary on one thread you do not want any other thread to modify it.

To create a thread safe dictionary you should create a wrapper over the dictionary which maintains data internally in a private IDictionary<> and use lock in methods like Add, Remove, Clear etc.

Refer to this SO post for how to do that - What's the best way of implementing a thread-safe Dictionary?

Alternatively, if you are using .Net 4 then you can use the ConcurrentDictionary which provides thread safety out of the box.

Answer 5

The Monitor.Enter(), Monitor.Exit() (lock(object)) is probably your close to best bet. But this code can be improved somewhat, although real benefit might be insignificant.

     private static Dictionary<string, int> foo = new Dictionary<string, int>();
     private static ReaderWriterLock rwLock= new ReaderWriterLock();
     static int reads = 0;
     static int writes = 0;


     private static bool Contains(string bar) 
     {
          try 
          { 
              rwLock.AquireReaderLock(TimeOut.Infinite);
              InterLocked.Increment(ref reads);
              return foo.ContainsKey(bar);


          }
          catch(Exception) 
          {


          }
          finally 
          {
                rwLock.ReleaseReaderLock();
          }
     }

     public static void Add(string bar)
     {
        try 
        {
           rwLock.AquireWriterLock(TimeOut.Infinite);

           if (!ContainsKey(bar)) 
           {
                 foo.Add(bar, 0);
           }
           foo[bar] = foo[bar] + 1;
           Interlocked.Increment(ref writes);
        }
        catch(Exception) {}
        finally 
        {
              rwLock.ReleaseWriterLock();
        }

}

you would do the same for remove.