CODEWITHSUNDEEP
c#.netconcurrencydictionarylocking
Ace
Ace

Reputation: 4443

.NET Dictionary: Potential concurrency problems?

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:

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?

Upvotes: 9

Views: 6549

Answers (11)

Sani Huttunen
Sani Huttunen

Reputation: 24375

Shouldn't 

if (!evilDict.contains(item.name.ToLower().Trim()))

be 

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

?

Upvotes: 1

lurscher
lurscher

Reputation: 26943

the refactoring does not have to be painful or difficult to execute. Just do the following refactorings:

1) create a wrapper container class for your dictionary that implements the same interface as dictionary

2) look for the declaration of your dictionary and apply a refactoring changing the declared type (to the one you just created)

3) try to build, at this time, if your wrapper implements all the interface members, you should not get any compilation errors

4) on the accessors for your dictionary, wrap everything in a lock or whatever synchronization strategy you feel like applying

Upvotes: 0

lockNetMonster
lockNetMonster

Reputation:

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..

Upvotes: 1

lockNetMonster
lockNetMonster

Reputation:

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.

Upvotes: 1

Peter McG
Peter McG

Reputation: 19035

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.

Upvotes: 1

mandel
mandel

Reputation: 2947

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.

Upvotes: 2

lockNetMonster
lockNetMonster

Reputation:

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!! ;)

Upvotes: 3

lockNetMonster
lockNetMonster

Reputation: 

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

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

Upvotes: 1

ng5000
ng5000

Reputation: 12560

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.

Upvotes: 2

Frederik Gheysels
Frederik Gheysels

Reputation: 56934

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 ( );
}

Upvotes: 1

Megacan
Megacan

Reputation: 2518

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.

Upvotes: 6

Related Questions