Simple MemoryCache implementation for thread safe caching

Question

After much prompting from this post I wanted to build a simple, in-memory, thread-safe cache.

The only caveat (as far as I was originally concerned) was the need for two different absolute expiration times for cached objects - those being based on a property of the item being cached (IsFailureItem). This is for a .NET Framework 4.6.1 solution.

I am primarily interested in anything which makes this either thread-unsafe, leak memory or simply bad practice. I know the class itself could be generically typed, but that is a decision for the future.

public class CacheItem
{
 public IEnumerable<DataItem> Response { get; set; }
 public bool IsFailureItem { get; set; }
}
public class CacheHelper
{
 public static CacheHelper Cache { get; set; }
 private static IMemoryCache InMemoryCache { get; set; }
 static CacheHelper()
 {
 Cache = new CacheHelper();
 InMemoryCache = new MemoryCache(new MemoryCacheOptions { });
 }
 private CacheHelper() { }
 public CacheItem this[string key]
 {
 get => InMemoryCache.Get<CacheItem>(key);
 set => InMemoryCache.Set<CacheItem>(key, value, value.IsFailureItem ? FailureCacheEntryOption : SuccessCacheEntryOption );
 }
 private static MemoryCacheEntryOptions FailureCacheEntryOption => new MemoryCacheEntryOptions()
 { AbsoluteExpirationRelativeToNow = TimeSpan.FromSeconds(EnvironmentHelper.CacheFailureSecondsToLive) };
 private static MemoryCacheEntryOptions SuccessCacheEntryOption => new MemoryCacheEntryOptions()
 { AbsoluteExpirationRelativeToNow = TimeSpan.FromSeconds(EnvironmentHelper.CacheSuccessSecondsToLive) };
}

• aepot: Looks almost fine. InMemoryCache can be readonly field not property. Also you can remove setters from all other properties then introduce a constructor for CacheItem.
• Matt W: @aepot Thanks, changes added to post.
• aepot: Don't edit the code in the post. That's not allowed here because it's making reviews not suitable to the post. Please rollback the edits through the edit history. If you want to share the final solution, you may post it in a separate answer. Btw, you may accept the review below in case it was helpful.
• Matt W: Thank you @aepot - very wise. Reverted.

Answer 1

No reason to have a public set on public static CacheHelper Cache { get; set; } not anyway to set it so why expose it?

I think CacheItem should be immutable. If calling Set with a CacheItem that IsFailureItem is true but later can be set to false, wouldn't effect time. Same with Response. Is it to be expected to be able to retrieve the CacheItem and update/replace the Response property?

• Matt W: Thanks, changes applied in posted code. You're right about the immutability.

Answer 2

Here is the code, after applying changes as advised by the wise commenters here:

public class CacheItem
{
 public IEnumerable<DataItem> Response { get; }
 public bool IsFailureItem { get; }
 public CacheItem(bool isFailureItem, IEnumerable<DataItem> response = null)
 {
 IsFailureItem = isFailureItem;
 Response = response;
 }
}
public class CacheHelper
{
 public static CacheHelper Cache { get; }
 private static readonly IMemoryCache InMemoryCache;
 static CacheHelper()
 {
 Cache = new CacheHelper();
 InMemoryCache = new MemoryCache(new MemoryCacheOptions { });
 }
 private CacheHelper() { }
 public CacheItem this[string key]
 {
 get => InMemoryCache.Get<CacheItem>(key);
 set => InMemoryCache.Set<CacheItem>(key, value, value.IsFailureItem ? FailureCacheEntryOption : SuccessCacheEntryOption );
 }
 private static MemoryCacheEntryOptions FailureCacheEntryOption => new MemoryCacheEntryOptions()
 { AbsoluteExpirationRelativeToNow = TimeSpan.FromSeconds(EnvironmentHelper.CacheFailureSecondsToLive) };
 private static MemoryCacheEntryOptions SuccessCacheEntryOption => new MemoryCacheEntryOptions()
 { AbsoluteExpirationRelativeToNow = TimeSpan.FromSeconds(EnvironmentHelper.CacheSuccessSecondsToLive) };
}

• CharlesNRice: I would recommend in the constructor of CacheItem when setting Response to be 'Response = response ?? Enumerable.Empty<DataItem>()' I personally don't like null IEnumerables as linq will throw but on flip side setting value if someone passed in null would also be strange. Its a design choice you will need to make. To me I always lean to IEnumerables shouldn't be null
• Matt W: Actually, it would be great to be able to force that parameter/property to be non-null, but that's not possible at present. I guess I will actually throw an InvalidArgumentException. What do you think?
• Matt W: Actually, in retrospect, the IEnumerable<DataItem> response = null is there because it should only be null if bool isFailureItem is true. This is an application specific decision.