1

I have a dictionary declared as follows

IDictionary<string, object> _objectIds = new Dictionary<string, object>();

I was experiencing some problems with it and it discovered that the instance returned false as a result of ContainsKey method and from the watch window I was sure that the item was there. So I created helper method

private bool IdsContainsKey(string key)
{
  lock (syncObject)
  {
     lock (_objectIds)
     {
       if (_objectIds.ContainsKey(key))
         return true; // A
       if (_objectIds.ContainsKey(key))
         return true; // B
       return _objectIds.ContainsKey(key); // C
     }
  }
}

During my debugging session I run into situation when the method exited in place B and sometimes I made to C returning true. Can anybody help me? Thanks.

Karel Frajták
  • 4,389
  • 1
  • 23
  • 34
  • 1
    well, that will depend entirely what you pass in! – Mitch Wheat Apr 25 '10 at 09:47
  • well, it doesn't, key as parameter is not mutable and if you follow the code and my question, it is strange that first condition is not met, while the second is and sometimes it is not met too. And result is true. – Karel Frajták Apr 25 '10 at 19:30

3 Answers3

5

You need to make sure you put a lock around every place you use _objectIds in order to be sure you are synchronizing properly.

Oded
  • 489,969
  • 99
  • 883
  • 1,009
  • well, lock on syncObject is placed everywhere, in fact I pass a wrapper dictionary, that is recording each access or method call to inner dictionary and there are records for only one thread only – Karel Frajták Apr 25 '10 at 12:17
  • 1
    Why is a lock on `_objectIds` required if access to that object is already protected by a lock on `syncObject`? I don't see the value in having a double-lock pattern here. – Scott Dorman Apr 25 '10 at 13:34
  • I admit that this lock is redundant but I didn't know what to do, so I tried everything including this... – Karel Frajták Apr 25 '10 at 19:27
0

Is there a reason for locking twice? I think 


private bool IdsContainsKey(string key)
{
  lock (syncObject)
  {
    ...
  }
}

should do it. Althought I read somewhere that it's never a good idea to lock the instance with itself.

DHN
  • 4,807
  • 3
  • 31
  • 45
0

You don't need the lock(_objectIds) block. Also, why are you using a wrapper dictionary? Since you are using a generic Dictionary<TKey, TValue>, you should derive your own custom dictionary and implement your tracking there. Finally, your code would be better written as:

private bool IdsContainsKey(string key) 
{ 
   lock (syncObject) 
   {
      return _objectIds.ContainsKey(key); 
   } 
}

The result of the method is going to be entirely dependent upon the value you pass in as key. I suspect the problems you are encountering are due to incorrect locking in other places, an issue with your wrapper dictionary, or you aren't passing in the key you think you are.

Scott Dorman
  • 42,236
  • 12
  • 79
  • 110
  • Wrapper dictionary is used for diagnostic purpose only. I can assure you that every method of the class that contains _objectIds dictionary is "secured" with lock section and _objectIds is declared as private. – Karel Frajták Apr 25 '10 at 20:07