Locking to load data to cache

Question

I have a helper class in a web app, and among the things it does is to present common, unchanging data objects as static properties. I'm loading these object as below:

public static class MyWebHelper
{
    #region - Constants & Fields 
    private const string LocationCacheKey = "MyWebHelper_Locations";
    private static object LocationLoadLock = new object();
    private static MemoryCache m_SiteCache;
    #endregion

    #region - Properties 
    /// <summary>
    /// Gets the uneditable collection of locations.
    /// </summary>
    public static ReadOnlyCollection<Location> Locations
    {
        get
        {
            EnsureLocationsCache();
            return (ReadOnlyCollection<Location>)SiteCache[LocationCacheKey];
        }
    }

    /// <summary>
    /// Gets the MemoryCache object specific for my site cache.
    /// </summary>
    public static MemoryCache SiteCache
    {
        get
        {
            if (m_SiteCache == null)
            {
                m_SiteCache = new MemoryCache("MyWeb_SiteCache");
            }
            return m_SiteCache;
        }
    }
    #endregion

    #region - Methods 
    private static void EnsureLocationsCache()
    {
        lock (LocationLoadLock)
        {
            if (SiteCache[LocationCacheKey] == null)
            {
                //
                // Load all Locations from the database and perform default sorting on location code.
                //
                List<Location> lLocations = DataAccess.GetAllLocations();
                lLocations.Sort(delegate(Location l1, Location l2) { return string.CompareOrdinal(l1.Code, l2.Code); });
                SiteCache[LocationCacheKey] = new ReadOnlyCollection<Location>(lLocations);
            }
        }
    }
    #endregion
}

My question is, does the locking help anything? I'm trying to reduce calls to the database, but is the locking just introducing overhead? The cached data is used pretty commonly throughout the site, and will almost never change. Also, am I locking in the right place?

You could look at Lazy, it will help abstract the locking mechanics out. — asawyer, Mar 08 '12 at 14:06
@asawyer I coded up a test for Lazy solution, and would like to get it checked if done correctly. Should I post that code as edit to this question, answer to this question, or new question altogether? — Mike Guthrie, Mar 08 '12 at 16:17

Joe · Answer 1 · 2012-03-08T14:41:59.703

If you do lock, I would use double-checked locking so that you don't incur the overhead of acquiring the lock every time you read from the cache.

I would also question the need to lock at all. If you don't lock, then multiple threads may attempt to refresh the cache concurrently, but this will be rare and unless it is very expensive, won't cause a problem for static readonly data.

If it is expensive, then you could follow the advice in @asawyer's comment and insert a Lazy<ReadOnlyCollection<Location>> into the cache, so that the locking will be handled by Lazy<T>.

score 1 · Accepted Answer · edited May 23 '17 at 10:24

1

Since each web request to the server is creating a new thread, and your static helper is shared across these threads, a lock of some type is useful. Without a lock, you would be risking an entry into the EnsureLocationsCache method while a database read was already in process. Now this may not affect the correctness of the twice-loaded data but if the DB read is expensive it will impact your overall performance and negate the effect of caching.

This is really dependant on the volume of concurrent threads trying to access the EnsureLocationsCache() method an application start, which is likely to be low as it's a one-time call for each LocationCacheKey.

The overhead of acquiring the lock is a valid concern as the code acquires a lock even when the cache has already been loaded. @asawyer, @TomTom and @Joe have suggested some alternatives.

EDIT:

Consider calling EnsureLocationsCache() in a static constructor. In this case you should not require a lock at all.

See discussion on thread-safety of static constructors.

edited May 23 '17 at 10:24

Community

1
1

answered Mar 08 '12 at 14:05

Zaid Masud

13,225
9
67
88

-1 for ingoring realities. the lock is NOT needed, this can be doen better with a ReadWriterLock, only escalating to full lock when a db access is needed. Most of the time loaded data will be hit and a readlock only is more efficient (allowing parallel reads, onlyseralizing when a write to the cache is needed). You could possibly even go lock less. Lock is nice, simple - and sadly naive in a technical sense that it is a heavy solution. – TomTom Mar 08 '12 at 14:29
@TomTom Would the ReadWriterLock solution be to invoke EnterReadLock before the check for null, then EnterWriteLock to read from db into cache? To community as a whole, have I accepted wrong answer? If so, is it good practice to un-accept? Not sure of any ramifications there. – Mike Guthrie Mar 08 '12 at 14:49
Under load, yes- you would et a read lock before you read, because the readl ock will stall if anotehr thread has the write lock. So, unless something is written 100 threads can read at the same time. If the cache normally is filled with the correct answer... this is better. Depends a lot on perforamcne, though - I write stuff running hundreds of thousands of cache loops per second. Naive solutions do not get you far there. – TomTom Mar 08 '12 at 14:51
@TomTom, please re-read the question. Agreed, the full lock may not be the best lock in this case. However the question is whether the lock "helps anything" and that is the only question that I have attempted to answer. I have not attempted to answer the un-asked question of what the best locking approach is. – Zaid Masud Mar 08 '12 at 15:07
1

@zooone9243 That was the intent of my question. I would not be adverse to you editing your answer to be more complete, and keeping the selected answer status. Not sure if that is against some policy, however. – Mike Guthrie Mar 08 '12 at 15:14
1

"Consider calling EnsureLocationsCache() in a static constructor rather than on every call to the Locations property" - this won't work if the item is evicted from the MemoryCache. The whole point of `EnsureLocaltionsCache` is to ensure the cache is automatically refreshed when needed again. – Joe Mar 09 '12 at 14:33

Locking to load data to cache

2 Answers2

Linked