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;
    }
  }
}

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

3
  • Can you try to switch on Caching.DebugEnabled in the config, then Sitecore logs some extra info. Sep 19, 2019 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. Sep 19, 2019 at 14:02
  • Will take a look at those suggestions @TamásTárnok. Thank you. Sep 19, 2019 at 14:23

1 Answer 1

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 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
6
  • 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? Sep 19, 2019 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, 2019 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, 2019 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. Sep 19, 2019 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, 2019 at 18:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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