.NET Dictionary: Potential concurrency problems?

Question

Hi all. I'm working on maintenance of a .NET project, and I'm having some troubles which I'll gladly share with you guys =)

The problem code:

if( evilDict.Count < 1 )
{
    foreach (Item item in GetAnotherDict())
        if (!evilDict.containsKey(item.name.ToLower().Trim()))
            evilDict.add(item.name.ToLower().Trim(), item.ID);
}

Despite the contains()-check, I'm getting an ArgumentException telling me that an item with the same key has already been added. We have only gotten this problem in production, never in testing, which makes me suspect a concurrency problem. What I'm wondering is:

• Do you think it's a concurrency problem?
• How do I fix it?
• Is my fix viable (see below)?
• This is my first stab at .NET, are Dictionaries usually a source of problems?

Here's my potential fix, replacing the dictionary.add() thing

protected static void DictAddHelper(Dictionary<String, int> dict, String key, int value)
{
    lock (dict)
    {
        key = key.ToLower().Trim();
        if (dict.ContainsKey(key) == false)
        {
            try
            {
                dict.Add(key, value);
            }
            catch (ArgumentException aex)
            {
                StringBuilder debugInfo = new StringBuilder();
                debugInfo.AppendLine("An argumentException has occured: " + aex.Message);
                debugInfo.AppendLine("key = " + key);
                debugInfo.AppendLine("value = " + value);
                debugInfo.AppendLine("---Dictionary contains---");

                foreach (String k in dict.Keys)
                    debugInfo.AppendLine(k + " = " + dict[k]);

                log.Error(debugInfo, aex);
            }
        }
    }
}

EDIT:

Suggestions that don't require me to make a thread-safe implementation of the Dict class are better, since it would be a pretty big refactoring that won't be a very welcome suggestion =)

EDIT2:

I tried

lock (((IDictionary)dict).SyncRoot)

But I get

Error   28 Using the generic type 'System.Collections.Generic.IDictionary<TKey,TValue>' requires '2' type arguments

Then I try this:

lock (((IDictionary<String, int>)dict).SyncRoot)

Error:

Error   28 'System.Collections.Generic.IDictionary<string,int>' does not contain a definition for 'SyncRoot'

FINAL EDIT (I guess):

Thanks for all the answers!

Now, all I want to know is this. Will my method (DictAddHelper) work at all, and if not, why?

Answer 1

Shouldn't
if (!evilDict.contains(item.name.ToLower().Trim()))

be
if (!evilDict.ContainsKey(item.name.ToLower().Trim()))

Answer 2

If you suspect you have concurrency problems accessing the dictionary then your fix won't be of any use. It will solve the specific problem you are experiencing, however if you have concurrent access to the dictionary you are going to have more problems in the future.

Consider locking the access to the dictionary when you are modifying it.

Answer 3

Difficult to say whether it is indeed a concurrency problem. If the dictionary is being accessed by multiple threads, then it can indeed be a concurrency problem, but I think that you should add some tracing code to find out who the culprit is.

If the dictionary is being accessed by multiple threads, then you should indeed make sure that the Contains (or ContainsKey) and Add methods are called in one atomic transaction. In order to do this, you should indeed call these 2 methods within a lock.

lock( dict.SyncRoot )
{
   if( dict.Contains( .. ) == false )
      dict.Add ( );
}

Answer 4

The first code (assuming that the Dictionary type is System.Collections.Generic.Dictionary) won't compile. There is no public contains (nor Contains method).

That said, there is a possibility that you could have a concurrency issue. As you've implied another thread could modify the dictionary after the ContainsKey check and before the insertion. To correct this the lock statement is the way to go.

One point - I'd prefer to see the dictionary wrapped in a thread safe class, something like (caveats: not complete, and not intended as reference code, and can be improved):

public class ThreadSafeDictionary
{
    private Dictionary<string, int> dict = new Dictionary<string, int>();
    private object padlock = new object();

    public void AddItem( string key, int value )
    {
        lock ( padlock )
        {
            if( !dict.ContainsKey( key ) )
                dict.Add( key, value );
        }
    }
}

How to implement thread safe dictionaries has been covered in full here.

Answer 5

    lock (((IDictionary)dict).SyncRoot)
    {
        if(!dict.Contains( .. ))
        dict.Add ( );
    }

this works for me (see update ng5000 example below) :)

Answer 6

using ng5000's example, modified to the lock strategy suggested would give us:

public static class ThreadSafeDictionary
{
    private static readonly Dictionary<string, int> dict = new Dictionary<string, int>();

    public static void AddItem(string key, int value)
    {
        lock (((IDictionary)dict).SyncRoot)
        {
            if (!dict.ContainsKey(key))
                dict.Add(key, value);
        }
    }
}

enjoy . . .

jimi

EDIT: note that class has to be static to make sense of this example!! ;)

Answer 7

As Megacan said you might want to focus on solving any possible concurrency problems you might have in your solution.

I recommend using the SynchronizedKeyedCollection although that might be far to many re factoring for you since the members for the calss are not the same a the ones of a dictionary.

Answer 8

There is a thread-safe dictionary class built into the .NET Framework that already offers a good start for solving your problem which could indeed be concurrency related.

It is an abstract class called SynchronizedKeyedCollection(K, T) that you could derive from and add a method that contains a call to Contains then Add inside a lock that locks on base.SyncRoot.

Answer 9

Ace - strange that it didn't work on your code, it certainly does as per the example (i know this doesn't help you, just a curiosity really!!).

Hope you work it out, i'm sure it'll be a fairly simple reason for failing. you might even want to try the example in place of your code to see if it's some other issue.

Answer 10

Ace,

Create a new WindowsFormsApplication (.Net 2.0) and paste the code into the Form1.cs code. (you may have to change the name of the namespace from WindowsFormsApplication1 to whatever your system defaults to). Also, add a command button to the form. Anyway, here it all is:

using System;
using System.Collections;
using System.Collections.Generic;
using System.Windows.Forms;

namespace WindowsFormsApplication1
{
    public partial class Form1 : Form
    {
        private readonly Dictionary<string, int> localDict1 = new Dictionary<string, int>();
        private readonly Dictionary<string, string> localDict2 = new Dictionary<string, string>();

        public Form1()
        {
            InitializeComponent();
        }

        private void button1_Click(object sender, EventArgs e)
        {
            // use two different dictionaries just to show it working with
            // different data types (i.e. we could use class objects too)
            if (localDict1 != null)
            {
                ThreadSafeDictionaryStatic.AddItem(localDict1, "New Item :1", 1);
                ThreadSafeDictionaryStatic.AddItem(localDict1, "New Item :2", 2);
            }
            if (localDict2 != null)
                ThreadSafeDictionaryStatic.AddItem(localDict2, "New Item :1", "this is not a number");
        }
    }

    public static class ThreadSafeDictionaryStatic
    {

        public static void AddItem<T>(IDictionary<string, T> dict, string key, T value)
        {
            if (dict == null) return;
            lock (((IDictionary)dict).SyncRoot)
            {
                if (!dict.ContainsKey(key))
                    dict.Add(key, value);
                else
                {
                    // awful and we'd never ever show a message box in real life - however!!
                    var result = dict[key];
                    MessageBox.Show(String.Format("Key '{0}' is already present with a value of '{1}'", key, result));
                }
            }
        }

        public static T GetItem<T>(IDictionary<string, T> dict, string key)
        {
            if (dict == null) return default(T);
            if (dict.ContainsKey(key))
                return dict[key];
            else
                return default(T);
        }

        public static bool Remove<T>(IDictionary<string, T> dict, string key)
        {
            if (dict == null) return false;
            lock (((IDictionary)dict).SyncRoot)
            {
                if (dict.ContainsKey(key))
                    return dict.Remove(key);
            }
            return false;
        }
    }
}

Let me know how this goes...

EDIT: re-did the class to match your method signature, also used generics as you'd want the method to accept ANY kind of object. you can easily change it back by removing the <T> refs etc..