Wrapper for caching methods in ASP.NET

Question

I have written a caching wrapper method for some of my services. The actual wrapper method is written as follows:

public T GetFromCache<T>(string key, Func<T> defaultValuePredicate, object cacheLock, TimeSpan duration)
{
    lock (cacheLock)
    {
        var result = _cache[key] is T ? (T)_cache[key] : default(T);
        if (Equals(default(T), result))
        {
            result = defaultValuePredicate();
            if (!Equals(default(T), result))
            {
                _cache.Add(key, result, null,
                            DateTime.Now.Add(duration),
                            System.Web.Caching.Cache.NoSlidingExpiration,
                            System.Web.Caching.CacheItemPriority.Default,
                            (cacheKey, value, reason) =>
                            Debug.WriteLine(string.Format("{0} Dropped from cache becasue {1}", cacheKey, reason)));
            }
        }

        return result;
    }
}

A typical use case is this:

public class SomeService
{
    private static readonly object _lock = new object();

    public string GetSomeValue()
    {
         return GetFromCache("SomeService_GetSomeValue", () => 
                            {
                                //Expensive operation here
                            }, 
                            _lock, 
                            TimeSpan.FromSeconds(600));
    }

}

In all the example code that I've seen floating around the web people never seem to lock around expensive, but cached operations. The reason I lock is to prevent the same operation kicking off twice, duplicating the effort.

Is this approach reasonable and robust, or does it need serious modification to make it more reliable?

Answer 1

I see a minor issues with the code: you are locking every time, no matter if it's needed on not. I would implemented GetFromCache like this

public T GetFromCache<T>(string key, object cacheLock, Func<T> defaultValuePredicate, TimeSpan duration)
{
    object result = _cache[key]; // using object as T may be value type and _cache may return null

    if (result == null || !(result is T))
    {
        lock (cacheLock)
        {
            result = _cache[key];  // rechecking value from cache as another thread may already initialize it
            if (result == null || !(result is T))
            {
                result = defaultValuePredicate();
                if (!Equals(default(T), result))
                {
                    _cache.Add(key, result, null,
                            DateTime.Now.Add(duration),
                            System.Web.Caching.Cache.NoSlidingExpiration,
                            System.Web.Caching.CacheItemPriority.Default,
                            (cacheKey, value, reason) =>
                            Debug.WriteLine(string.Format("{0} Dropped from cache becasue {1}", cacheKey, reason)));
                }
            }
        }
    }

    return (T)result;
}

It's a little more code (+1 if statement) but it doesn't lock thread every time you access it.