Wrapper for caching methods in ASP.NET I have written a caching wrapper method for some of my services. The actual wrapper method is written as follows:

public T GetFromCache&lt;T&gt;(string key, Func&lt;T&gt; 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) =&gt;
                            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", () =&gt; 
                            {
                                //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?
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&lt;T&gt;(string key, object cacheLock, Func&lt;T&gt; 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) =&gt;
                            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.