Per-Method Thread Synchronization Locking

Question

I'd like to create a static Cached class for an ASP.NET MVC site for quick access to cached items like dropdown lists. It needs to have locking implemented so that when a key comes back empty it can be pulled from the repository while any other request threads wait on it to come back. As such, it needs per-method thread locking (versus a shared lock). My first thought was to use nameof as the lock for each method instead of creating a separate object to lock for each method. A simplified version would look something like...

public static class Cached
{
    public static List<Country> GetCountriesList()
    {
        List<Country> cacheItem = null;

        if (HttpContext.Current.Cache["CountriesList"] != null)
            cacheItem = (List<Country>)HttpContext.Current.Cache["CountriesList"];
        else
        {
            lock (nameof(GetCountriesList))            
            {
                // Check once more in case it got stored while waiting on the lock
                if (HttpContext.Current.Cache["CountriesList"] == null)    
                {
                    using (var repo = new Repository())
                    {
                        cacheItem = repo.SelectCountries();
                        HttpContext.Current.Cache.Insert("CountriesList", cacheItem, null, DateTime.Now.AddHours(2), TimeSpan.Zero);
                    }
                }
                else
                    cacheItem = (List<Country>)HttpContext.Current.Cache["CountriesList"];
            }
        }

        return cacheItem;
    }

    public static List<State> GetStatesList()
    {
        List<State> cacheItem = null;

        if (HttpContext.Current.Cache["StatesList"] != null)
            cacheItem = (List<State>)HttpContext.Current.Cache["StatesList"];
        else
        {
            lock (nameof(GetStatesList))            
            {
                // Check once more in case it got stored while waiting on the lock
                if (HttpContext.Current.Cache["StatesList"] == null)    
                {
                    using (var repo = new Repository())
                    {
                        cacheItem = repo.SelectStates();
                        HttpContext.Current.Cache.Insert("StatesList", cacheItem, null, DateTime.Now.AddHours(2), TimeSpan.Zero);
                    }
                }
                else
                    cacheItem = (List<State>)HttpContext.Current.Cache["StatesList"];
            }
        }

        return cacheItem;
    }
}

Is there anything glaringly wrong with an approach like this?

UPDATE:

Per the advice that it is a bad idea to lock on strings, I've changed it to a pattern that I found in SO's Opserver code that uses a ConcurrentDictionary to store a lock object per cache key. Is there anything wrong with the following:

public static class Cached
{
    private static readonly ConcurrentDictionary<string, object> _cacheLocks = new ConcurrentDictionary<string, object>();

    private const string KEY_COUNTRIES_LIST = "CountriesList";
    public static List<Country> GetCountriesList()
    {
        List<Country> cacheItem = null;
        var nullLoadLock = _cacheLocks.AddOrUpdate(KEY_COUNTRIES_LIST, k => new object(), (k, old) => old);

        if (HttpContext.Current.Cache[KEY_COUNTRIES_LIST] != null)
            cacheItem = (List<Country>)HttpContext.Current.Cache[KEY_COUNTRIES_LIST];
        else
        {
            lock (nullLoadLock)
            {
                // Check once more in case it got stored while waiting on the lock
                if (HttpContext.Current.Cache[KEY_COUNTRIES_LIST] == null)
                {
                    using (var repo = new Repository())
                    {
                        cacheItem = repo.SelectCountries();
                        HttpContext.Current.Cache.Insert(KEY_COUNTRIES_LIST, cacheItem, null, DateTime.Now.AddHours(2), TimeSpan.Zero);
                    }
                }
                else
                    cacheItem = (List<Country>)HttpContext.Current.Cache[KEY_COUNTRIES_LIST];
            }
        }

        return cacheItem;
    }

    private const string KEY_STATES_LIST = "StatesList";
    public static List<State> GetStatesList()
    {
        List<State> cacheItem = null;
        var nullLoadLock = _cacheLocks.AddOrUpdate(KEY_COUNTRIES_LIST, k => new object(), (k, old) => old);

        if (HttpContext.Current.Cache[KEY_STATES_LIST] != null)
            cacheItem = (List<State>)HttpContext.Current.Cache[KEY_STATES_LIST];
        else
        {
            lock (nullLoadLock)
            {
                // Check once more in case it got stored while waiting on the lock
                if (HttpContext.Current.Cache[KEY_STATES_LIST] == null)
                {
                    using (var repo = new Repository())
                    {
                        cacheItem = repo.SelectStates();
                        HttpContext.Current.Cache.Insert(KEY_STATES_LIST, cacheItem, null, DateTime.Now.AddHours(2), TimeSpan.Zero);
                    }
                }
                else
                    cacheItem = (List<State>)HttpContext.Current.Cache[KEY_STATES_LIST];
            }
        }

        return cacheItem;
    }
}

Answer 1

Based on what you posted so far, I think you're over-thinking this. :) I don't see a need to populate yet another dictionary with your locking objects. Since you are using them in explicitly named methods, just declare them as fields as needed.

First, the advice to not lock on string values is sound, but based on the problem that two string values can appear identical while still being different objects. You could avoid that in your scenario by storing the appropriate string value in a const field:

public static class Cached
{
    private const string _kcountries = "CountriesList";
    private const string _kstates = "StatesList";

    public static List<Country> GetCountriesList()
    {
        List<Country> cacheItem = (List<Country>)HttpContext.Current.Cache[_kcountries];

        if (cacheItem == null)
        {
            lock (_kcountries)
            {
                // Check once more in case it got stored while waiting on the lock
                cacheItem = (List<Country>)HttpContext.Current.Cache[_kcountries];

                if (cacheItem == null)    
                {
                    using (var repo = new Repository())
                    {
                        cacheItem = repo.SelectCountries();
                        HttpContext.Current.Cache.Insert(_kcountries, cacheItem, null, DateTime.Now.AddHours(2), TimeSpan.Zero);
                    }
                }
            }
        }

        return cacheItem;
    }

    public static List<State> GetStatesList()
    {
        // Same as above, except using _kstates instead of _kcountries
    }
}

Note that you shouldn't be using string literals throughout the code anyway. It's much better practice to define const fields to represent those values. So you kill two birds with one stone doing the above. :)

The only remaining problem is that you are still using a possibly-public value to lock, since the string literals are interned, and if the exact same string was used somewhere else, it would likely be the same interned value as well. This is of debatable concern; it's my preference to avoid doing so, to ensure no other code outside my control could take the same lock my code is trying to use, but there are those who feel such concerns are overblown. YMMV. :)

If you do care (as I do) about using the possibly-public value, then you can associate a unique object value instead of using the string reference:

public static class Cached
{
    private const string _kcountriesKey = "CountriesList";
    private const string _kstatesKey = "StatesList";

    private static readonly object _kcountriesLock = new object();
    private static readonly object _kstatesLock = new object();

    public static List<Country> GetCountriesList()
    {
        List<Country> cacheItem = (List<Country>)HttpContext.Current.Cache[_kcountriesKey];

        if (cacheItem == null)
        {
            lock (_kcountriesLock)
            {
                // Check once more in case it got stored while waiting on the lock
                cacheItem = (List<Country>)HttpContext.Current.Cache[_kcountriesKey];

                if (cacheItem == null)    
                {
                    using (var repo = new Repository())
                    {
                        cacheItem = repo.SelectCountries();
                        HttpContext.Current.Cache.Insert(_kcountriesKey, cacheItem, null, DateTime.Now.AddHours(2), TimeSpan.Zero);
                    }
                }
            }
        }

        return cacheItem;
    }

    // etc.
}

I.e. use the ...Key field for your cache (since it does require string values for keys) but the ...Lock field for locking (so that you are sure no code outside your control would have access to the object value used for the lock).

I'll note that you do have an opportunity to reduce the repetition in the code, by writing a single Get...() implementation that can be shared by your various types of data:

public static class Cached
{
    private const string _kcountriesKey = "CountriesList";
    private const string _kstatesKey = "StatesList";

    private static readonly object _kcountriesLock = new object();
    private static readonly object _kstatesLock = new object();

    public static List<Country> GetCountriesList()
    {
        // Assuming SelectCountries() is in fact declared to return List<Country>
        // then you should actually be able to omit the type parameter in the method
        // call and let type inference figure it out. Same thing for the call to
        // _GetCachedData<State>() in the GetStatesList() method.
        return _GetCachedData<Country>(_kcountriesKey, _kcountriesLock, repo => repo.SelectCountries());
    }

    public static List<State> GetStatesList()
    {
        return _GetCachedData<State>(_kstatesKey, _kstatesLock, repo => repo.SelectStates());
    }

    private static List<T> _GetCachedData<T>(string key, object lockObject, Func<Repository, List<T>> selector)
    {
        List<T> cacheItem = (List<T>)HttpContext.Current.Cache[key];

        if (cacheItem == null)
        {
            lock (lockObject)
            {
                // Check once more in case it got stored while waiting on the lock
                cacheItem = (List<T>)HttpContext.Current.Cache[key];

                if (cacheItem == null)    
                {
                    using (var repo = new Repository())
                    {
                        cacheItem = selector(repo);
                        HttpContext.Current.Cache.Insert(key, cacheItem, null, DateTime.Now.AddHours(2), TimeSpan.Zero);
                    }
                }
            }
        }

        return cacheItem;
    }

    // etc.
}

Finally, I'll note that since the underlying cache (i.e. System.Web.Caching.Cache) is thread-safe, you could just skip all of this altogether, and instead choose to blindly populate the cache if your item (the List<T> in question) isn't found. The only downside is that you in some cases could retrieve the same list more than once. The upside is that the code is a lot simpler.