3

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;
    }
  }
}
  • Can you try to switch on Caching.DebugEnabled in the config, then Sitecore logs some extra info. – Tamás Tárnok Sep 19 '19 at 13:53
  • Can you also debug the InnerCache.CacheWriteEnabled when you are trying to add a value? This property is checked in the InnerCache.Add method. – Tamás Tárnok Sep 19 '19 at 14:02
  • Will take a look at those suggestions @TamásTárnok. Thank you. – Matthew Dresser Sep 19 '19 at 14:23
2

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 cachekey 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 most sense to get rid off 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 which 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
| improve this answer | |
  • Thanks for the suggestions. I also saw this answer stackoverflow.com/questions/39112/… which mentioned about using a ConcurrentDictionary to enable locking by cache key, rather than a single lock. I had planned to implement that, but wanted to figure out what the other error was first. When you mentioned about "some small issues with the code" could you elaborate on what they are, please? – Matthew Dresser Sep 19 '19 at 14:27
  • I'm a little bit confused about the problems with the single thread but in a multi-threaded environment there are some issues. It is missing the volatile keyword on the instance which is getting cached. Depending on what happens in creator.Invoke() it might also have some issues as described here: jetbrains.com/help/resharper/2019.2/… Is there a reason you are not using Lazy<T> to initialize? – Jeroen Sep 19 '19 at 15:13
  • Also what is the reason that you are implementing this locking? You are adding locking on top of a cache which is already using ConcurrentDictionary which internally uses locking. It can be very challenging to test all scenario's and be certain this is not going to run into any unexpected issues in production. – Jeroen Sep 19 '19 at 15:30
  • I wanted to implement locking as I was seeing the same items being added twice. The reason for doing that was that we are sometimes caching something which does take a fair bit of processing to retrieve. – Matthew Dresser Sep 19 '19 at 20:00
  • Makes sense. I would update the code to make the object which is cached volatile and depending on what happens in creator.Invoke() also update the code for the issue I linked to in my earlier comment. – Jeroen Sep 20 '19 at 18:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.