Cookie Monster Cookie Monster - 1 month ago 8
C# Question

ConcurrectDictionary or Custom ThreadSafeDictionary

Recently I've been trying out the Concurrent Collections and I must say that I'm really impressed with them as they seem to save you from lots of work when it comes to multi-threading environments.

Until now I've been using this custom ThreadSafeDictionary and basically I'd like to ask what you guys think about its use when compared to ConcurrentDictionary. Is it worth continue using this one? Shall I drop it and start using ConcurrentDictionary?

It would be great if someone who's familiar to the Concurrent Collections could give me some opinions on this.

using System;
using System.Linq;
using System.Collections.Generic;
using System.Runtime.InteropServices;

namespace System.Collections.Generic
{
public class ThreadSafeDictionary<T1, T2>
{
private Dictionary<T1, T2> SafeDictionaryBase;
public T2[] Values;

public ThreadSafeDictionary(int capacity)
{
Values = new T2[0];
SafeDictionaryBase = new Dictionary<T1, T2>(capacity * 2);
}
public int Count
{
get
{
return SafeDictionaryBase.Count;
}
}
public Dictionary<T1, T2> Base
{
get
{
return SafeDictionaryBase;
}
}
public void Add(T1 key, T2 value)
{
lock (SafeDictionaryBase)
SafeDictionaryBase.Add(key, value);
safeUpdate();
}
public void Remove(T1 key)
{
lock (SafeDictionaryBase)
SafeDictionaryBase.Remove(key);
safeUpdate();
}

public T2 this[T1 key]
{
get
{
if (ContainsKey(key))
return SafeDictionaryBase[key];
else return default(T2);
}
}

public bool TryGetValue(T1 key, out T2 value)
{
return SafeDictionaryBase.TryGetValue(key, out value);
}

public bool ContainsKey(T1 key)
{
return SafeDictionaryBase.ContainsKey(key);
}

public bool ContainsValue(T2 value)
{
return SafeDictionaryBase.ContainsValue(value);
}

private void safeUpdate()
{
if (Values == null)
Values = SafeDictionaryBase.Values.ToArray();
else
{
if (Threading.Monitor.TryEnter(Values, 1))
Values = SafeDictionaryBase.Values.ToArray();
}
}
}
}

Answer

I am not the best expert in thread safety but I have used the ConcurrentDictionary a few times and I never had any trouble with it. In my opinion you should use the ConcurrentDictionary because it is really well supported and not too difficult to implement. Even though your ThreadSafeDictionary is more safe than a normal Dictionary I think it lacks some features that make it less safe.

For instance:

public T2 this[T1 key]
{
    get
    {
        if (ContainsKey(key)) // You check if your dictionary contains the key
            // In the meantime, another thread might have deleted this entry
            return SafeDictionaryBase[key]; // Here you try to return the entry but the key might be missing
        else return default(T2);
    }
}

In this example you can see that your implementation is not really thread safe, and that is just one example.

You can try this simple program to compare your implementation compared to the ConcurrentDictionary:

namespace TestThreadSafeDictionary
{
    class Program
    {
        static void Main(string[] args)
        {
            try
            {
                TestConcurrentDictionary();
                Console.WriteLine("ConcurrentDictionary finished without problem.");
            }
            catch
            {
                Console.WriteLine("ConcurrentDictionary finished with at least one problem.");
            }

            try
            {
                TestThreadSafeDictionary();
                Console.WriteLine("ThreadSafeDictionary finished without problem.");
            }
            catch
            {
                Console.WriteLine("ThreadSafeDictionary finished with at least one problem.");
            }

            Console.ReadKey();
        }

        private static void TestConcurrentDictionary()
        {
            var random = new Random();

            var dic = new ConcurrentDictionary<object, object>();

            var addThread = new Task(() =>
            {
                for (int i = 0; i < 1000; ++i)
                    dic.TryAdd(random.Next(100), random.Next());
            });
            addThread.Start();

            var getThread = new Task(() =>
            {
                object v;
                for (int i = 0; i < 1000; ++i)
                    dic.TryGetValue(random.Next(100), out v);
            });
            getThread.Start();

            var removeThread = new Task(() =>
            {
                object v;
                for (int i = 0; i < 1000; ++i)
                    dic.TryRemove(random.Next(100), out v);
            });
            removeThread.Start();

            Task.WaitAll(addThread, getThread, removeThread);
        }

        private static void TestThreadSafeDictionary()
        {
            var random = new Random();

            var dic = new ThreadSafeDictionary<object, object>(1000);

            var addThread = new Task(() =>
            {
                for (int i = 0; i < 1000; ++i)
                    dic.Add(random.Next(100), random.Next());
            });
            addThread.Start();

            var getThread = new Task(() =>
            {
                object v;
                for (int i = 0; i < 1000; ++i)
                    dic.TryGetValue(random.Next(100), out v);
            });
            getThread.Start();

            var removeThread = new Task(() =>
            {
                for (int i = 0; i < 1000; ++i)
                    dic.Remove(random.Next(100));
            });
            removeThread.Start();

            Task.WaitAll(addThread, getThread, removeThread);

            Console.WriteLine("Finished without problem.");
        }
    }
}

So far the ConcurrentDictionary didn't had any error when I tested it while your implementation is causing some exceptions.