Locking to load data to cache

问题:

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?

回答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.

回答2:

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