For .NET 3.5 how would you make a thread-safe get or add cache?

I have starting working with some .NET 3.5 code and found the following extension method being used for a cache::

public static TValue GetOrAdd<TKey, TValue>(this Dictionary<TKey, TValue> @this, TKey key,Func<TKey,TValue> factory,bool useLocking)
{

    TValue value;
    if(!@this.TryGetValue(key,out value))
    {
        if (useLocking)
        {
            lock ((@this as ICollection).SyncRoot)
            {
                if (!@this.TryGetValue(key, out value))
                {
                    @this[key] = value = factory(key);
                }
            }
        }
        else
        {
            @this[key] = value = factory(key);
        }
    }
    return value;
}

The cache in question is keyed by string keys, and with useLocking = true. It is always being accessed by this method (there is no stray TryGetValue). There is also no issue in using the SyncRoot property because the dictionary is private and no where else is it being used. The double locking is dangerous because a dictionary does not support reading while it's being written to. While technically there has been no issue reported yet as the product hasn't shipped, I feel that this approach is going to lead to race c开发者_开发问答onditions.

1. Switch the Dictionary<,> to a Hashtable. We'll lose type-safety but we'll be able to support the concurrency model we're after, (1 writer, multiple readers).

2. Remove the outer TryGetValue. That way every read, requires acquiring a lock. This is potentially bad for performance, but acquiring an uncontested lock should be fairly cheap.

Both are pretty crappy. Does anyone have a better suggestion? If this was .NET 4 code I'd just switch it to a ConcurrentDictionary, but I don't have that option.

---

Yes, your suspicions are correct; there is a race condition and all methods, even TryGetValue need to be inside the lock in the implementation you presented.

As far as performance, you can look forward to the day that you can upgrade to .NET4 which includes a screaming-fast ConcurrentDictionary out of the box. Until then, you can review James Michael Hare's analysis done here:

• C#: The Curious ConcurrentDictionary

Those results suggest to me that the best implementation for .NET3.5 is Dictionary plus ReadWriteLockSlim and for good measure, here's a complete implementation:

• Thread Safe Dictionary in .NET with ReaderWriterLockSlim

Update:

I read the tables wrong and it looks like Dictionary + lock is little faster than the only other serious contender Dictionary + ReadWriteLockSlim.

---

My recommendation would be to download Reactive Extensions for .NET (Rx), which includes backports of the collections in the System.Collections.Concurrent namespace (this includes ConcurrentDictionary<TKey, TValue>) for .NET 3.5.

---

Remove the TryGetValue. I bet you won't see the concurrency problem; CLR monitors are pretty quick, and are "unfair" so that you don't see convoy or priority inversion problems.

If you do see a concurrency problem, then the next best thing is a ReaderWriterLockSlim. Unfortunately, you're going to want to make a new class for that instead of using an extension method, because you're going to need a place to store the lock.

If you go that route, be sure to stay away from upgrading from reader to writer locks.

---

I think you need to lock the complete statement (as you stated).

However, there's a neat solution on the web to be prepared for a future .NET 4 upgrade: Have a custom class instead of a dictionary, keep the dictionary as private member variable and wrap all access to the dictionary in a thread-safe lock statement. Detailed description can be found here: http://www.grumpydev.com/2010/02/25/thread-safe-dictionarytkeytvalue/