Is it safe to update the Dictionary from multiple threads with the same key/value
I have a static System.Collections.Generic.Dictionary<K, V>
that's used as a cache. On a cache miss I do some expensive work to get the value for the specified key and save it in the cache:
private static Dictionary<K, V> _cache = ...
public static V GetValue<K>(K key)
{
V value;
if (!_cache.TryGetValue(key, out value))
{
value = GetExpensiveValue(key);
_cache[开发者_运维知识库key] = value; // Thread-safety issue?
}
return value;
}
This is a common pattern. The question is: is this safe or could it fail when multiple threads try to call GetValue()
at the same time?
Assuming that GetExpensiveValue
always returns the same value for a given key, I don't really care which thread wins the race condition. Yes, it means that the expensive operation is potentially done one more time than needed, but that's a trade-off I may be happy with if reads are far more frequent than writes and this allows me to eliminate locking. But would something actually go wrong inside the Dictionary? That is, would it throw an exception or corrupt its data in some cases? I tested this out with a simple console app and found no issues, but of course that doesn't mean it will always work.
That doesn't look thread-safe to me; the inside of the dictionary could already be in the process of being re-written. At a minimum it probably needs a reader/writer lock:
private readonly static ReaderWriterLockSlim @lock = new ReaderWriterLockSlim();
public static V GetValue(K key)
{
V value;
@lock.EnterReadLock();
bool hasValue;
try
{
hasValue = _cache.TryGetValue(key, out value);
}
finally
{
@lock.ExitReadLock();
}
if (!hasValue)
{
value = GetExpensiveValue(key);
@lock.EnterWriteLock();
try
{
_cache[key] = value;
}
finally
{
@lock.ExitWriteLock();
}
}
return value;
}
and if you only want GetExpensiveValue(key)
to execute once, move it inside the write-lock and add a double-check:
@lock.EnterWriteLock();
try
{
if(!_cache.TryGetValue(key, out value)) { // double-check
value = GetExpensiveValue(key);
_cache[key] = value;
}
}
finally
{
@lock.ExitWriteLock();
}
If you don't care about race conditions then you should be fine. If you care about the expensive operation, just lock
after TryGetvalue()
and then call TryGetValue()
again
private static Dictionary<K, V> _cache = ...
private static object _lockObject = new object();
public static V GetValue<K>(K key)
{
V value;
if (!_cache.TryGetValue(key, out value))
{
lock(_lockObject)
{
if (!_cache.TryGetValue(key, out value))
{
value = GetExpensiveValue(key);
_cache[key] = value; // Thread-safety issue?
}
}
}
return value;
}
Edit: After consideration of Marc's comment, I think the above is not the best option. Perhaps consider using the ConcurrentDictionary instead.
精彩评论