How to correctly use lock when adding/renewing cached data in asp.net cache?

Question

This is sort of a follow up question on https://stackoverflow.com/questions/754661/httpruntime-cache-best-practices/11431198 where a reply from frankadelic contains this quote and code sample from http://msdn.microsoft.com/en-us/magazine/cc500561.aspx:

The problem is that if you've got a query that takes 30 seconds and you're executing the page every second, in the time it takes to populate the cache item, 29 other requests will come in, all of which will attempt to populate the cache item with their own queries to the database. To solve this problem, you can add a thread lock to stop the other page executions from requesting the data from the database.

// check for cached results
object cachedResults = ctx.Cache["PersonList"];
ArrayList results = new ArrayList();

if  (cachedResults == null)
{
  // lock this section of the code
  // while we populate the list
  lock(lockObject)
  {
    // only populate if list was not populated by
    // another thread while this thread was waiting
    if (cachedResults == null)
    {
      ...
    }
  }
}

In my opinion this code sample would have benefitted from showing the actual cache assign/get logic as well. My assumption also is that the cachedResults reference in the sample above should be a static reference, otherwise different threads wont access the same instance.

Is this correct and would the following be a correct implementation of a property where you want to lazy-load data through the cache?

private static object _someDataCacheLock = new object();
private static object _cachedResults;
public List<object> SomeData
{
    get
    {
        HttpContext ctx = HttpContext.Current;
        _cachedResults = ctx.Cache["SomeData"] as List<object>;

        if (_cachedResults == null)
        {
            // lock this section of the code
            // while we populate the list
            lock (_someDataCacheLock)
            {
                // only populate if list was not populated by
                // another thread while this thread was waiting
                if (_cachedResults== null)
                {
                    _cachedResults= GetSomeData(); //db access inside GetSomeData()

                    ctx.Cache.Insert("SomeData",
                            _cachedResults,
                            null, DateTime.Now.AddMinutes(10),
                            TimeSpan.Zero, System.Web.Caching.CacheItemPriority.Normal, null);
                }
            }
        }

        return _cachedResults;
    }
}

---

Ok, so I'm going ahead with this code instead, where I've added one row that in my opinion must have been missing from the MSDN sample... Any feedback on this?

private static object _someDataCacheLock = new object();
public List<object> SomeData
{
    get
    {
        HttpContext ctx = HttpContext.Current;
        List<object> cachedResults = ctx.Cache["SomeData"] as List<object>;

        if (cachedResults == null)
        {
            // lock this section of the code
            // while we populate the list
            lock (_someDataCacheLock)
            {
                //This row was missing in the MSDN sample, I believe...
                cachedResults = ctx.Cache["SomeData"] as List<object>;

                // only populate if list was not populated by
                // another thread while this thread was waiting
                if (cachedResults == null)
                {
                    cachedResults = GetSomeData(); //db access inside GetSomeData()

                    ctx.Cache.Insert("SomeData",
                            cachedResults,
                            null, DateTime.Now.AddMinutes(10),
                            TimeSpan.Zero, System.Web.Caching.CacheItemPriority.Normal, null);
                }
            }
        }

        return cachedResults;
    }
}

Answer 1

First, I think your code doesn't make much sense. The only reason why Cache can be useful is if you want the cached data to expire after some time or if the memory is low. But you're preventing the memory to be freed by using the field _cachedResults, which will hold the data even after they are removed from the cache. This field is of no use for you, _cachedResults should be a local variable instead.

Second, your code is not reliably thread-safe, because of the way you're assigning to _cachedResults. What could happen is if GetSomeData() creates the result using new, an uninitialized object could be first assigned to _cachedResults and only then would be the constructor called. Such reordering could happen, because it's safe from the point of view of a single thread.

Normally, you would solve this by using a volatile write. But in your case, simply using a local variable instead of a field will be enough.

For more details, see The Famous Double-Check Locking Technique in Chapter 29 of Jeffrey Richter's CLR via C#, or the Wikipedia article Double-checked locking.