.NET Dictionary: potential concurrency issues?

Question

I am working on supporting a .NET project, and I am having problems that I will gladly share with you guys =)

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 fact that contains () is check, I get an ArgumentException telling me that an element with the same key has already been added. We only got this problem in production, never testing, which makes me suspect a concurrency problem. I am wondering:

• Do you think this is a concurrency problem?
• How to fix it?
• Is my solution viable (see below)?
• This is my first hit in .NET, are dictionaries usually the source of problems?

Here is my potential fix replacing 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 do not require me to make a thread-safe implementation of the Dict class better, as it would be quite a lot of refactoring, which would not 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) 

Mistake:

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

FINAL EDIT (think):

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

If you suspect that you have problems accessing the w390 dictionary, then your fix will not be used. This will solve the specific problem you are facing, however, if you have parallel access to the dictionary, you will have more problems in the future.

Consider blocking access to the dictionary when changing it.

Answer 2

using the ng5000 example modified to the proposed blocking strategy:

 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 the class must be static in order to understand this example !;)

Answer 3

The first code (assuming the dictionary type is System.Collections.Generic.Dictionary) will not compile. No public content (or Contains method).

However, there is a chance that you may have a concurrency problem. Since you meant that another thread could change the dictionary after checking the ContainsKey and before pasting. To fix this, the lock statement is the way to go.

One point - I would rather see a 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 fully described here .

Answer 4

As Megacan said, you can focus on resolving any possible concurrency problems that may arise in your solution.

I recommend using SynchronizedKeyedCollection, although this may be far from many re-factoring for you, since the members for calss are different from the dictionary.

Answer 5

Not worth it

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

be

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

?

Answer 6

It's hard to say if this is really a concurrency problem. If multiple threads access the dictionary, then it really can be a concurrency problem, but I think you should add some trace code to find out who is the culprit.

If multiple threads are accessing the dictionary, then you need to make sure that the Contains (or ContainsKey) and Add methods are called in the same atomic transaction. To do this, you really have to call these 2 methods inside the lock.

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

Answer 7

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

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

Answer 8

The .NET Framework has a thread-safe dictionary class that already offers a good start to solve your problem, which can really be related to concurrency.

This is an abstract class called SynchronizedKeyedCollection (K, T) , from which you can extract and add a method that contains a call. Contains then Add inside the lock, which is locked based on .SyncRoot.

Answer 9

Ace - it is strange that it did not work on your code, it certainly does according to an example (I know this will not help you, just curiosity!).

I hope you can handle it, I'm sure it will be a fairly simple reason for failure. you can even try the example instead of your code to see if it has any other problem.

Answer 10

Ace

Create a new WindowsFormsApplication (.Net 2.0) and paste the code into the Form1.cs code. (you may need to change the namespace name from WindowsFormsApplication1 to your default standards). Also add a command to the form. In any case, everything is here:

 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 (ie 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 happens ...

EDIT: repeated the class to match your method signature, also used generics, since you want the method to take ANY kind of object. you can easily change it by deleting <T> refs etc.

Answer 11

refactoring should not be painful or difficult to perform. Just do the following refactoring:

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

2) find the declaration of your dictionary and apply refactoring by changing the declared type (to the one you just created)

3) try creating at this time, if your shell implements all interface members, you should not get compilation errors

4) on the accessories for your dictionary, wrap everything in a lock or any synchronization strategy that you want to apply