Locked Caching using Cache Keys

Question

We have code below we use to improve performance. It works fine, but every few days we start receiving a ton of exceptions (below). It's not related to volume, but it's random.

Comment: /// Performs the locked code to produce the result if necessary while thread locking it and then caching the result.

line 45 is: lock (_keys.First(k => k == key))

Any Ideas?

Code:

    public class LockedCaching
{
    private static List<string> _keys = new List<string>();

    public class Result
    {
        public object Value { get; set; }
        public bool ExecutedDataOperation { get; set; }
    }

    /// <summary>
    /// Performs the locked code to produce the result if necessary while thread locking it and then caching the result.
    /// </summary>
    /// <param name="key"></param>
    /// <param name="expiration"></param>
    /// <param name="data"></param>
    /// <returns></returns>
    public static Result Request(string key, DateTime expiration, RequestDataOperation data)
    {
        if (key == null)
        {
            return new Result { Value = data(), ExecutedDataOperation = true };
        }

        //Does the key have an instance for locking yet (in our _keys list)?
        bool addedKey = false;
        bool executedDataOperation = false;
        if (!_keys.Exists(s => s == key))
        {
            _keys.Add(key);
            addedKey = true;
        }
        object ret = HttpContext.Current.Cache[key];
        if (ret == null)
        {
            lock (_keys.First(k => k == key))
            {
                ret = HttpContext.Current.Cache[key];
                if (ret == null)
                {
                    ret = data();
                    executedDataOperation = true;
                    if(ret != null)
                        HttpContext.Current.Cache.Insert(key, ret, null, expiration, new TimeSpan(0));
                }
            }
        }
        if (addedKey)
            CleanUpOldKeys();
        return new Result { Value = ret, ExecutedDataOperation = executedDataOperation };
    }

    private static void CleanUpOldKeys()
    {
        _keys.RemoveAll(k => HttpContext.Current.Cache[k] == null);
    }
}

Exception:

Exception: System.Web.HttpUnhandledException (0x80004005): Exception of type 'System.Web.HttpUnhandledException' was thrown. ---> System.ArgumentNullException: Value cannot be null. Parameter name: key at System.Web.Caching.CacheInternal.DoGet(Boolean isPublic, String key, CacheGetOptions getOptions) at PROJECT.LockedCaching.b__8(String k) in PROJECT\LockedCaching.cs:line 64 at System.Collections.Generic.List1.RemoveAll(Predicate1 match) at PROJECT.LockedCaching.CleanUpOldKeys() in PROJECT\LockedCaching.cs:line 64 at PROJECTLockedCaching.Request(String key, DateTime expiration, RequestDataOperation data) in PROJECT\LockedCaching.cs:line 58 at FeaturesWithFlags1.DataBind() at System.Web.Util.CalliHelper.EventArgFunctionCaller(IntPtr fp, Object o, Object t, EventArgs e) at System.Web.UI.Control.LoadRecursive() at System.Web.UI.Control.LoadRecursive() at System.Web.UI.Control.LoadRecursive() at System.Web.UI.Control.LoadRecursive() at System.Web.UI.Control.LoadRecursive() at System.Web.UI.Control.LoadRecursive() at System.Web.UI.Page.ProcessRequestMain(Boolean includeStagesBeforeAsyncPoint, Boolean includeStagesAfterAsyncPoint) at System.Web.UI.Page.HandleError(Exception e) at System.Web.UI.Page.ProcessRequestMain(Boolean includeStagesBeforeAsyncPoint, Boolean includeStagesAfterAsyncPoint) at System.Web.UI.Page.ProcessRequest(Boolean includeStagesBeforeAsyncPoint, Boolean includeStagesAfterAsyncPoint) at System.Web.UI.Page.ProcessRequest() at System.Web.UI.Page.ProcessRequest(HttpContext context) at System.Web.HttpApplication.CallHandlerExecutionStep.System.Web.HttpApplication.IExecutionStep.Execute() at System.Web.HttpApplication.ExecuteStep(IExecutionStep step, Boolean& completedSynchronously)

Web control where it's used - This web control requests a list of locations from a webservice. We use this lockedcache request almost everywhere we call the webservice.:

public override void DataBind()
{
    try
    {
        string cacheKey = "GetSites|";
        mt_site_config[] sites = (mt_site_config[])LockedCaching.Request(cacheKey, DateTime.UtcNow.AddMinutes(10),
        () =>
        {
            WebServiceClient service = new WebServiceClient();
            sites = service.GetSites();
            service.Close();
            return sites;
        }).Value;
        ddlLocation.Items.Clear();
        ddlLocation.Items.Add(new ListItem("Please Select"));
        ddlLocation.Items.Add(new ListItem("Administration"));
        ddlLocation.Items.AddRange
        (
            sites.Select
            (
                s => new ListItem(s.site_name + " " + s.site_location, s.th_code.ToString())
            ).ToArray()
        );
    }
    catch (Exception ex) {
        Logger.Error("ContactUs Control Exception: Exp" + Environment.NewLine + ex.Message);
    }
    base.DataBind();

}

Thank you for your comments. ConcurrentDictionary was the way to go. The issue to why we received errors was because the linq code "lock (_keys.First(k => k == key))" was returning an exception rather than null. Using the concurrentdictionary will be much safer and hopefully not cause any lock issues.

Modified Code:

public class LockedCaching
{

    public class Result
    {
        public object Value { get; set; }
        public bool ExecutedDataOperation { get; set; }
    }

    public static Result Request(string key, DateTime expiration, RequestDataOperation data)
    {
        if (key == null)
        {
            return new Result { Value = data(), ExecutedDataOperation = true };
        }

        object results = HttpContext.Current.Cache[key];
        bool executedDataOperation = false;

        if (results == null)
        {
            object miniLock = _miniLocks.GetOrAdd(key, k => new object());
            lock (miniLock)
            {
                results = HttpContext.Current.Cache[key];
                if (results == null)
                {
                    results = data();
                    executedDataOperation = true;
                    if (results != null)
                        HttpContext.Current.Cache.Insert(key, results, null, expiration, new TimeSpan(0));

                    object temp;
                    object tempResults;
                    if (_miniLocks.TryGetValue(key, out temp) && (temp == miniLock))
                        _miniLocks.TryRemove(key, out tempResults);

                }
            }
        }
        return new Result { Value = results, ExecutedDataOperation = executedDataOperation };
    }

    private static readonly ConcurrentDictionary<string, object> _miniLocks =
                              new ConcurrentDictionary<string, object>();

}

Answer 1

Your code has a race condition on the collection. You write to it concurrently. This can have all kinds of effects.

_keys.Add(key);
...
_keys.RemoveAll(k => HttpContext.Current.Cache[k] == null);

There are other races as well. You probably should revise expand the amount of code that you put under your global lock. Be careful not to destroy too much concurrency by using that global lock.

Maybe you can switch to a ConcurrentDictionary<string, Lazy<CacheValue>>. This is the canonical pattern for a cache that works like yours. It does not suffer from cache stampeding.

Be careful with threading. It is easy to introduce subtle races like in this case.

Answer 2

The exception you're seeing indicates that _keys has a null element in it. From your code snippet this shouldn't happen. So either other code we can't see is adding nulls, or you have a thread safety issue. Since you almost certainly have thread-safety bugs (see question comments) I'd start looking there.