How to implement Sitecore.Caching.CustomCache with locking

Question

I'm trying to use (a derivative of) the Sitecore CustomCache class in my solution but am seeing that during the course of my load testing, certain values are not being retrieved from the cache and are being re-requested (and hence re-added to the cache). I'm trying to adopt the double-checked locking pattern. From the console debug statements added to my code, I am even seeing that the same thread is not retrieving the cached value immediately after setting it, although it does later serve the value from cache.

I'm wondering if there is simply a bug in the code, or if I'm just trying to do this in the wrong way?

I have created a class in my solution that inherits Sitecore.Caching.CustomCache

public class CustomCache : Sitecore.Caching.CustomCache
{
  public CustomCache(string name, long maxSize) : base(name, maxSize)
  {
  }

  public CustomCache(long maxSize) : base("My.Foundation.Cache", maxSize)
  {
  }

  public void AddCacheObject(string key, object value)
  {
    InnerCache.Add(key, value);
  }

  public object GetCacheObject(string key)
  {
    return !InnerCache.ContainsKey(key) ? null : InnerCache.GetValue(key);
  }
}

And I have created a wrapper around this with static methods

public class CacheManager
{
  private static readonly CustomCache _customCache = new CustomCache(
  Sitecore.StringUtil.ParseSizeString(
    Settings.GetSetting("My.Foundation.Caching.CacheSize", "500MB")));

  private static readonly object _customCacheLock = new object();

  internal static object Get(string key)
  {
    if (_customCache == null)
    {
      return null;
    }

    return _customCache.InnerCache.GetValue(key);
  }

  internal static void Set(string key, object value)
  {
    if (_customCache == null)
    {
      return;
    }

    _customCache.AddCacheObject(key, value);
  }

  internal static string GetCacheKey(string key)
  {
    return $"{Context.Site?.Name}{Context.Database?.Name}{Context.Language?.Name}{key}";
  }

  public void ClearCache(object sender, EventArgs args)
  {
    _customCache?.Clear();
  }

  public static TObj GetCachedObject<TObj>(string cacheKey, Func<TObj> creator) where TObj : class
  {
    var fullKey = GetCacheKey(cacheKey);

    var obj = Get(fullKey) as TObj;
    if (obj != null)
    {
      System.Diagnostics.Debug.WriteLine($"CacheKey:{fullKey} (get - without lock) ThreadId:{System.Threading.Thread.CurrentThread.ManagedThreadId} RemainingSpace:{_customCache.InnerCache.RemainingSpace}");
    return obj;
    }

    lock (_customCacheLock)
    {
      obj = Get(fullKey) as TObj;
      if (obj != null)
      {
        System.Diagnostics.Debug.WriteLine($"CacheKey:{fullKey} (get - lock) ThreadId:{System.Threading.Thread.CurrentThread.ManagedThreadId} RemainingSpace:{_customCache.InnerCache.RemainingSpace}");
        return obj;
      }

      obj = creator.Invoke();
      System.Diagnostics.Debug.WriteLine($"CacheKey:{fullKey} (set - lock) ThreadId:{System.Threading.Thread.CurrentThread.ManagedThreadId} RemainingSpace:{_customCache.InnerCache.RemainingSpace}");
      Set(fullKey, obj);
      return obj;
    }
  }
}

EDIT Jeroen was correct in that I was approaching this in the wrong way and was totally correct in his suggestions. It turned out that in this instance caching was actually disabled hence the constant cache misses. Sometimes when looking at a problem we (I) look for a complex solution before double-checking the simple things...

Answer 1

There are some small issues with the code but more importantly, you are going about this the wrong way. You have adapted a locking pattern for a singleton to something that is not a singleton. Currently, you are locking on a single static object regardless of what the cache key is. This can easily cause excessive locking in a real-world scenario. I would recommend you fix this in 1 of 2 ways:

1. If the instantiation of the object you are caching is thread-safe and not very expensive, then it might make the most sense to get rid of the lock altogether and accept that sometimes an object gets added to the cache twice. Behind the scenes, the cache is implemented as a ConcurrentDictionary and therefore is thread-safe.
2. If option 1 is not an option then you should implement a solution that is not based on a singleton pattern but locks based on the cache key. This will require a good amount of code to implement properly but a good example can be found in this SO answer