Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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?

share|improve this question

1 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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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