trjhtr

I'm trying to implement a cache available in a data provider (repository).

The data provider is a normal class, instanciated at every request made to the application.

The reason I have a static class is that I want all requests to share the same _lock object.

The data being cached doesn't change very often and is quite small (a few deserialized JSON documents) but they are often requested by other logic in the application.

Is this good design? I have test ran it under stress and it seems to work fine.

I think the locking in the Cache class should do what it is supposed to be doing (making sure data is not changed by two concurent requests and data that is being read is always up to date).

I also lock when reading from the database in the case where a request comes in and for some reason is slower than a second one (the first request would then populate the cache with possibly outdated data).

Are there any obvious flaws you can spot? Is the use of MemoryCache overkill in this case? I don't really need managing expiration and other options MemoryCache offers, so I'm split about just using a Dictionary.

public class DataProvider 

 static SemaphoreSlim dbLock = new SemaphoreSlim(1);

 //This class has a bunch of methods to access the DB directly, when needed.

 /// <summary>
 /// Get items from the MemoryCache
 /// </summary>
 /// <typeparam name="T"></typeparam>
 /// <returns></returns>
 public async Task<List<T>> GetCacheItems<T>() where T : IDocumentModel
 
 if (Cache.Items<T>() == null)
 
 await dbLock.WaitAsync();
 try
 
 var items = await GetItemsFromDatabase<T>();
 Cache.SetItems(items);
 
 finally
 
 dbLock.Release();
 
 

 return Cache.Items<T>();


 

 /// <summary>
 /// Update a cached item when DataProvider updates one
 /// </summary>
 /// <typeparam name="T"></typeparam>
 /// <param name="item"></param>
 /// <returns></returns>
 public async Task UpdateCacheItem<T>(string itemData) where T : IDocumentModel
 
 var item = JsonConvert.DeserializeObject<T>(itemData);
 var cachedItems = await GetCacheItems<T>();

 if (cachedItems == null)
 throw new Exception($"Error retrieving cached items of type 'typeof(T).Name'");

 var cachedItem = cachedItems.SingleOrDefault(x => x.Id == item.Id);

 if (cachedItem == null)
 
 cachedItems.Add(item);
 
 else
 
 cachedItems.Remove(cachedItem);
 cachedItems.Add(item);
 

 Cache.SetItems(cachedItems);

 

 public static class Cache
 
 private static object _lock = new object();
 private static DateTimeOffset offset = DateTimeOffset.UtcNow.AddYears(5);

 /// <summary>
 /// Retrieve items of specified type
 /// </summary>
 /// <typeparam name="T"></typeparam>
 /// <returns></returns>
 public static List<T> Items<T>()
 
 var itemKey = typeof(T).Name;

 lock (_lock)
 
 return MemoryCache.Default.Get(itemKey) as List<T>;
 

 

 /// <summary>
 /// Load items in MemoryCache
 /// </summary>
 /// <typeparam name="T"></typeparam>
 /// <param name="items"></param>
 public static void SetItems<T>(List<T> items)
 
 var itemKey = typeof(T).Name;

 lock (_lock)
 
 MemoryCache.Default.AddOrGetExisting(itemKey, items, offset);
 
 
 

The MemoryCache is thread safe, all those locks and semaphore are useless.

Also, the Memory Cache by itself makes some serializations so at every update you are deserializing the whole collection of given type, add the item and re-serialize the whole collection.

If you can really use a Dictionary use it, a ConcurrentDictionary < Type,List < T >> will resolve all your problem