Take the 2-minute tour ×

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)

share|improve this question
The short answer is you can't do this safely, see duplicate for details. –  csharptest.net Jul 19 '11 at 19:15

3 Answers 3

up vote 8 down vote accepted

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

share|improve this answer
I think I see what you're saying. Ignoring Dictionary<TKey, TValue> for a second, if there was simply a bool named "IsInitialized", then that would be a proper way to do the double-checked locking pattern? And also, regarding Dictionary, if you used 'ContainsKey' instead of 'TryGetValue', would that be a better method for doing the first check? Or can you just not trust any method off of dictionary for thread safety? –  Mike Richards Jul 19 '11 at 19:27
@Mike: Yes, checking a boolean variable would be okay - although personally I'm not a fan of the DCL pattern in the first place. No, using Dictionary.ContainsKey wouldn't be any better. The class is simply documented not to be thread-safe. –  Jon Skeet Jul 19 '11 at 19:45
@Jon Skeet, "... although personally I'm not a fan of the DCL pattern in the first place." +1 for that alone. –  csharptest.net Jul 19 '11 at 19:51

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.

share|improve this answer

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

        new RWLockTest().Test(3);
        new RWSlimTest().Test(3);
        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 val = 0; val < LIMIT; val += 3)
                    _data[val] = val;

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

                Stopwatch sw = new Stopwatch();
                foreach (Thread t in threads)
                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();
                if (i % WRITE == 0)
                    _data[r.Next(LIMIT)] = i;
                    _data.TryGetValue(i, out val);

The preceeding program outputs the following results on my PC:

Completed: 2412
Completed: 2385
Completed: 2422

Completed: 1374
Completed: 1397
Completed: 1491

Completed: 763
Completed: 750
Completed: 758
share|improve this answer
+1 for the performance breakdown. –  hoodaticus Jul 19 '11 at 20:07

Your Answer


