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

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

  re开发者_运维技巧turn 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)

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

This isn't safe, because a second thread can potentially read the value from SomeDictionary while the dictionary is in an inconsistent state.

Consider the following scenario:

1. Thread A attempts to get id 3. It doesn't exist, so it acquires the lock and calls Add, but is interrupted partway through the method.
2. Thread B attempts to get id 3. The call to Add has gotten far enough that the method returns (or attempts to return) true.

Now a variety of bad things could happen. It's possible that Thread B sees the first TryGetValue (outside the lock) return true, but the value that's returned is nonsensical because the real value hasn't actually been stored yet. The other possibility is that the Dictionary implementation realizes that it's in an inconsistent state and throws InvalidOperationException. Or it might not throw, it might just continue with a corrupted internal state. Either way, bad mojo.

Just remove the first TryGetValue and you'll be fine.

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

Do not use ReaderWriterLock or ReaderWriterLockSlim unless you are doing less than 20% writes AND the workload within the lock is significant enough that parallel reads will matter. As an example, the following demonstrates that a simple lock() statement will out-perform the use of either reader/writer locks when the read/write operation is simple.

internal class MutexOrRWLock
{
    private const int LIMIT = 1000000;
    private const int WRITE = 100;//write once every n reads

    private static void Main()
    {
        if (Environment.ProcessorCount < 8)
            throw new ApplicationException("You must have at least 8 cores.");
        Process.GetCurrentProcess().ProcessorAffinity = new IntPtr(255); // pin the process to first 8 CPUs

        Console.WriteLine("ReaderWriterLock");
        new RWLockTest().Test(3);
        Console.WriteLine("ReaderWriterLockSlim");
        new RWSlimTest().Test(3);
        Console.WriteLine("Mutex");
        new MutexTest().Test(3);
    }

    private class RWLockTest : MutexTest
    {
        private readonly ReaderWriterLock _lock1 = new ReaderWriterLock();

        protected override void BeginRead() { _lock1.AcquireReaderLock(-1); }
        protected override void EndRead() { _lock1.ReleaseReaderLock(); }

        protected override void BeginWrite() { _lock1.AcquireWriterLock(-1); }
        protected override void EndWrite() { _lock1.ReleaseWriterLock(); }
    }

    private class RWSlimTest : MutexTest
    {
        private readonly ReaderWriterLockSlim _lock1 = new ReaderWriterLockSlim();

        protected override void BeginRead() { _lock1.EnterReadLock(); }
        protected override void EndRead() { _lock1.ExitReadLock(); }

        protected override void BeginWrite() { _lock1.EnterWriteLock(); }
        protected override void EndWrite() { _lock1.ExitWriteLock(); }
    }

    private class MutexTest
    {
        private readonly ManualResetEvent start = new ManualResetEvent(false);
        private readonly Dictionary<int, int> _data = new Dictionary<int, int>();

        public void Test(int count)
        {
            for (int i = 0; i < count; i++)
            {
                _data.Clear();
                for (int val = 0; val < LIMIT; val += 3)
                    _data[val] = val;

                    start.Reset();
                Thread[] threads = new Thread[8];
                for (int ti = 0; ti < 8; ti++)
                    (threads[ti] = new Thread(Work)).Start();

                Thread.Sleep(1000);
                Stopwatch sw = new Stopwatch();
                sw.Start();
                start.Set();
                foreach (Thread t in threads)
                    t.Join();
                sw.Stop();
                Console.WriteLine("Completed: {0}", sw.ElapsedMilliseconds);
            }
        }

        protected virtual void BeginRead() { Monitor.Enter(this); }
        protected virtual void EndRead() { Monitor.Exit(this); }

        protected virtual void BeginWrite() { Monitor.Enter(this); }
        protected virtual void EndWrite() { Monitor.Exit(this); }

        private void Work()
        {
            int val;
            Random r = new Random();
            start.WaitOne();
            for (int i = 0; i < LIMIT; i++)
            {
                if (i % WRITE == 0)
                {
                    BeginWrite();
                    _data[r.Next(LIMIT)] = i;
                    EndWrite();
                }
                else
                {
                    BeginRead();
                    _data.TryGetValue(i, out val);
                    EndRead();
                }
            }
        }
    }
}

The preceeding program outputs the following results on my PC:

ReaderWriterLock
Completed: 2412
Completed: 2385
Completed: 2422

ReaderWriterLockSlim
Completed: 1374
Completed: 1397
Completed: 1491

Mutex
Completed: 763
Completed: 750
Completed: 758