3
\$\begingroup\$

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 < DateTime.Now;
            }
        }
    }

    private Dictionary<string, CacheItem> internalCache = new Dictionary<string, CacheItem>();
    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<T>(string key, Func<T> loadFunc, TimeSpan cacheFor) where T : class
    {
        lock (syncRoot)
        {
            CacheItem item;
            if (this.internalCache.TryGetValue(key, out item))
            {
                if (item.IsExpired)
                {
                    ClearCache(key);
                    item = null;
                }
            }
            if (item == null)
            {
                item = new CacheItem(key, cacheFor);
                item.Data = loadFunc();
                this.internalCache.Add(key, item);
            }
            return item.Data as T;
        }
    }

}

Example Usage

var results = ServiceLocator.Current.GetInstance<ICacheService>().Cache<object>(key, () =>
            {
                return GetSomeData();
            },TimeSpan.FromSeconds(60));

We actually use this from within a PostSharp aspect :

[Serializable]
[MulticastAttributeUsage(MulticastTargets.Method, PersistMetaData = true)]
public class MethodCacheAttribute : MethodInterceptionAspect
{

    public const int ShortCacheSeconds = 120; // two mins
    public const int MediumCacheSeconds = 1200; // 20 minutes
    public const int LongCacheSeconds = 18000; //5 hours
    public const int DefaultCacheSeconds = MediumCacheSeconds;


    private ICacheService CacheService { get; set; }
    private string methodName;

    [IntroduceMember(Visibility = PostSharp.Reflection.Visibility.Family, OverrideAction = MemberOverrideAction.Ignore, IsVirtual = true)]
    public double CacheForSeconds { get; set; }


    public MethodCacheAttribute() : this(900) { } // 15 mins default;
    public MethodCacheAttribute(int seconds)
    {
        this.CacheForSeconds = seconds;
    }



    public override void CompileTimeInitialize(MethodBase method, AspectInfo aspectInfo)
    {
        var genericArgs = String.Join(".", method.GetGenericArguments().Select(t => t.Name));
        this.methodName = String.Format("{0}_{1}", method.Name, genericArgs);
    }

    public override void OnInvoke(MethodInterceptionArgs args)
    {
        var key = this.BuildCacheKey(args.Arguments);

        var item = ServiceLocator.Current.GetInstance<ICacheService>().Cache<object>(key, () =>
            {
                return args.Invoke(args.Arguments);
            },CacheForSeconds ==0 ? TimeSpan.MaxValue :  TimeSpan.FromSeconds(CacheForSeconds));
            args.ReturnValue = item;
    }

    private string BuildCacheKey(Arguments arguments)
    {
        var sb = new StringBuilder();
        sb.Append(this.methodName);

        foreach (var argument in arguments.ToArray())
        {
            sb.Append(argument == null ? "_" : argument.GetHashCode().ToString());
        }

        return sb.ToString();
    }
}

Then it is just a case of popping the attribute on a method - and its is cached automatically.

Example :

   [MethodCache(0)]
   public IEnumerable<IUserPermission> GetUserPermissions(string userName)
   {}
\$\endgroup\$
3
\$\begingroup\$

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<CacheItem> 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<string, CacheItem> internalCache =
    new ConcurrentDictionary<string, CacheItem>();

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<T>(string key, Func<T> 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;
}
\$\endgroup\$
  • \$\begingroup\$ I think the deadlock occurs because the loadFunc may call into another cached method, which tries to aquire the lock. \$\endgroup\$ – Richard Friend Sep 6 '12 at 9:29
  • \$\begingroup\$ I think that shouldn't cause a deadlock if it's the same cache. locks are reentrant, so if a thread already owns it and tries to take it again, it will succeed and not block. \$\endgroup\$ – svick Sep 6 '12 at 9:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.