Locking on a non-thread-safe object, is it acceptable practice?

Question

I got some grief about this in a comment I posted the other day, so I wanted to post the question in an attempt for people to tell me that I'm crazy, which I'll accept, or tell me that I may be right, which I'll also gladly accept. I may also accept anything in between.

Let's say you have a non-thread-safe object type such as Dictionary<int, string>. For the sake of argument, I know you can also use ConcurrentDictionary<int, string> which is thread safe, but I want to talk about the general practice around non-thread-safe objects in a multi-threaded environment.

Consider the following example:

private static readonly Dictionary<int, string> SomeDictionary = new Dictionary<int, string>();
private static readonly object LockObj = new object();

public static string GetById(int id)
{
  string result;

  /** Lock Bypass **/
  if (SomeDictionary.TryGetValue(id, out result)
  {
    return result;
  }

  lock (LockObj)
  {
    if (SomeDictionary.TryGetValue(id, out result)
    {
      return result;
    }

    SomeDictionary.Add(id, result = GetSomeString());
  }

  return result;
}

The locking pattern is called Double-Checked Locking, since the lock is actively bypassed if the dictionary is already initialized with that id. The "Add" method of the dictionary is called within the lock because we only want to call the method once, because it will throw an exception if you try to add an item with the same key.

It was my understanding that this locking pattern essentially synchronizes the way that Dictionary is handled, which allows it to be thread safe. But, I got some negative comments about how that doesn't actually make it thread safe.

So, my question is, is this locking pattern acceptable for non-thread-safe objects in a multi-threaded environment? If not, what would be a better pattern to use? (assuming there's not an identical C# type that is thread-safe)

Answer 1

No, this is not safe. The TryGetValue method simply isn't thread-safe, so you shouldn't use it when the object is shared between multiple threads without locking. The double-checked locking pattern involves just testing a reference - which while it isn't guaranteed to give an up to date result, won't cause any other problems. Compare that with TryGetValue which could do anything (e.g. throw an exception, corrupt the internal data structure) if called at the same time as, say, Add.

Personally I'd just use a lock, but you could potentially use ReaderWriterLockSlim. (In most cases a simply lock will be more efficient - but it depends on how long the reading and writing operations take, and what the contentions are like.)