Using lock on the key of a Dictionary<string, object>

Question

I have a Dictionary<string, someobject>.

EDIT: It was pointed out to me, that my example was bad. My whole intention was not to update the references in a loop but to update different values based on differnt threads need to update/get the data. I changed the loop to a method.

I need to update items in my dictionary - one key at a time and i was wondering if there are any problems in using the lock on the .key value of my Dictionary object?

private static Dictionary<string, MatrixElement> matrixElements = new Dictionary<string, MatrixElement>();

//Pseudo-code

public static void UpdateValue(string key)
{
    KeyValuePair<string, MatrixElement> keyValuePair = matrixElements[key];
    lock (keyValuePair.Key)
    {
        keyValuePair.Value  = SomeMeanMethod();
    }
}

Would that hold up in court or fail? I just want each value in the dictionary to be locked independantly so locking (and updating) one value does not lock the others. Also i'm aware the locking will be holding for a long time - but the data will be invalid untill updated fully.

Answer 1

Locking on an object that is accessible outside of the code locking it is a big risk. If any other code (anywhere) ever locks that object you could be in for some deadlocks that are hard to debug. Also note that you lock the object, not the reference, so if I gave you a dictionary, I may still hold references to the keys and lock on them - causing us to lock on the same object.

If you completely encapsulate the dictionary, and generate the keys yourself (they aren't ever passed in, then you may be safe.

However, try to stick to one rule - limit the visibility of the objects you lock on to the locking code itself whenever possible.

That's why you see this:

public class Something
{
  private readonly object lockObj = new object();

  public SomethingReentrant()
  {
    lock(lockObj)    // Line A
    {
      // ...
     }
   }
}

rather than seeing line A above replaced by

  lock(this)

That way, a separate object is locked on, and the visibility is limited.

Edit Jon Skeet correctly observed that lockObj above should be readonly.

Answer 2

No, this would not work.

The reason is string interning. This means that:

string a = "Something";
string b = "Something";

are both the same object! Therefore, you should never lock on strings because if some other part of the program (e.g. another instance of this same object) also wants to lock on the same string, you could accidentally create lock contention where there is no need for it; possibly even a deadlock.

Feel free to do this with non-strings, though. For best clarity, I make it a personal habit to always create a separate lock object:

class Something
{
    bool threadSafeBool = true;
    object threadSafeBoolLock = new object(); // Always lock this to use threadSafeBool
}

I recommend you do the same. Create a Dictionary with the lock objects for every matrix cell. Then, lock these objects when needed.

PS. Changing the collection you are iterating over is not considered very nice. It will even throw an exception with most collection types. Try to refactor this - e.g. iterate over a list of keys, if it will always be constant, not the pairs.