.Net caching service - thread safety

I have a simple cache service that i am using inside my WPF prism application, i am concerned about the thread safety of it - we will be accessing this via multiple threads and using the current code it has started deadlocking, it works fine without the locks but obviously this is not acceptable.

How can i go about imroving this, i have read about using ReaderWriterLockSlim or even a thread safe dictionary - but i am not sure if this is the best approach.

```
public class CacheService : ICacheService
{
internal class CacheItem
{
internal string Key { get; set; }
internal DateTime Expires { get; set; }
internal object Data { get; set; }
internal CacheItem(string key, TimeSpan lifeTime)
{
this.Key = key;
this.Expires = lifeTime == TimeSpan.MaxValue ? DateTime.MaxValue : DateTime.Now.Add(lifeTime);
}

internal bool IsExpired
{
get
{
return this.Expires internalCache = new Dictionary();
private object syncRoot = new object();
public void ClearCache()
{
foreach (var item in this.internalCache.ToList())
{
ClearCache(item.Key);
}

}

public void ClearCache(string key)
{
lock (syncRoot)
{
CacheItem item;
if (this.internalCache.TryGetValue(key, out item))
{
this.internalCache.Remove(key);
var disp = item.Data as IDisposable;
if (disp != null)
{
disp.Dispose();
}
}
}
}

public T Cache(string key, Func loadFunc, TimeSpan cacheFor) where T : class
{
lock (syncRoot)
{
CacheItem item;
if (this.internalCache.TryGetValue(key, out item))
{
if (item.IsExpired)
{
ClearCache(key);
item

I don't see any reason why your code should deadlock. Deadlocks require at least two locks (or similar constructs) used at the same time, but there is only one lock in your code. The only potential for deadlocok I can see is if some loadFunc also used locking.

And your code is not thread-safe: iterating Dictionary while its being modified may not work correctly, you should use locking there too (ReaderWriterLockSlim might make sense here):

public void ClearCache()
{
    List items;
    lock (syncRoot)
    {
        items = this.internalCache.ToList();
    }

    foreach (var item in items)
    {
        ClearCache(item.Key);
    }
}

But I think a better solution here would be to use ConcurrentDictionary, because its methods fit closely with what you need (especially GetOrAdd()):

private readonly ConcurrentDictionary internalCache =
    new ConcurrentDictionary();

public void ClearCache()
{
    foreach (var item in this.internalCache.ToList())
    {
        ClearCache(item.Key);
    }
}

public void ClearCache(string key)
{
    CacheItem item;
    if (this.internalCache.TryRemove(key, out item))
    {
        var disp = item.Data as IDisposable;
        if (disp != null)
        {
            disp.Dispose();
        }
    }
}

public T Cache(string key, Func loadFunc, TimeSpan cacheFor) where T : class
{
    CacheItem item;
    if (this.internalCache.TryGetValue(key, out item))
    {
        if (item.IsExpired)
        {
            ClearCache(key);
            item = null;
        }
    }
    if (item == null)
        item = this.internalCache.GetOrAdd(
            key, _ => new CacheItem(key, cacheFor) { Data = loadFunc() });
    return (T)item.Data;
}

StackExchange Code Review Q#15374, answer score: 3