Simple, flexible, and thread-safe key/value memory cache

Question

MemoryCache.cs

public class MemoryCache<TKey, TValue>
{
 private static readonly object Lock = new object();
 private static readonly IDictionary<TKey, TValue> Cache = new ConcurrentDictionary<TKey, TValue>();
 /// <summary>
 /// Function for setting the cache's value when the key isn't found.
 /// </summary>
 public Func<TKey, TValue> Set { get; set; }
 /// <summary>
 /// Retrieves the value marked by the indicated key in a thread-safe way.
 /// If the item is not found, uses Set to retrieve it and store it in the cache before returning the value.
 /// </summary>
 public TValue Get(TKey key)
 {
 lock (Lock)
 {
 if (Cache.TryGetValue(key, out var value) == false)
 {
 value = Set(key);
 Cache.Add(key, value);
 }
 return value;
 }
 }
 /// <summary>
 /// Clears all values in the cache.
 /// </summary>
 public void Clear()
 {
 lock (Lock)
 {
 Cache.Clear();
 }
 }
}

Usage

Set is being used to retrieve the customer information from the database via stored procedure. GetBasicCustomer is a service/repository function for retrieving a customer by customerId or sapCustomerId.

public class CustomerService : ICustomerService
{
 private static readonly MemoryCache<Tuple<int?, string>, Customer> CustomerCache = new MemoryCache<Tuple<int?, string>, Customer>()
 {
 Set = (Tuple<int?, string> key) =>
 {
 var ds = Database.ExecuteStoredProcedure("EXEC usp_Experiment_BasicCustomerInformation @CustomerID, @SAPCustomerID", key.Item1, key.Item2);
 if (ds == null || ds.Tables.Count == 0 || ds.Tables[0].Rows.Count == 0)
 {
 return null;
 }
 var dr = ds.Tables[0].Rows[0];
 return new Customer(
 id: dr.Field<int>("CustomerID"),
 sapCustomerId: dr.Field<string>("SAPCustomerID"),
 companyName: dr.Field<string>("CompanyName"),
 isSbtCustomer: dr.Field<bool>("IsSBTCustomer")
 );
 }
 };
 /// <summary>
 /// Retrieves the customer's basic information given CustomerID or SAPCustomerID (only one is required, make the other null).
 /// </summary>
 public Customer GetBasicCustomer(int? customerId, string sapCustomerId)
 {
 var key = new Tuple<int?, string>(customerId, sapCustomerId);
 return CustomerCache.Get(key); 
 }
}
public CustomerController()
{
 public ActionResult Index(int? customerId, string sapCustomerId)
 {
 var customerInfo = CustomerService.GetBasicCustomer(customerId, sapCustomerId);
 return View(customerInfo);
 }
}

Concerns

• I was thinking Set should maybe be named FindAndSet or something like that, but not really sure. I like the aesthetic of Get/Set.
• Is ConcurrentDictionary necessary to be thread-safe since I'm using a lock? Is it doing anything for me?

• Shelby115: I'm confused. I only want the MemoryCache class reviewed, I added usage for context, the thing you voted I lacked. So if you could comment and let me know what I'm lacking, that'd be appreciated.
• CharlesNRice: I didn't downvote but you know ConcurrentDictionary is already thread safe and you can just use GetOrAdd that takes a func. Having the lock seems overkill. I don't know if I would call it memorycache since MS already has a class called that
• dfhwze: You show how the cache is created from some query, but not how you would use that cache. Hence, your question seems to lack context to be on-topic for this site.
• Shelby115: @dfhwze I'm a bit thrown off by that, because it's a cache, the only context you should need is retrieving from it and setting it. Anything past that is specific to the application you're using the cache in.
• VisualMelon: Note that ConcurrentDictionart'2.GetOrAdd has different semantics to the Get here: the OP will call Set exactly once, so though it may simplify the implementation, GetOrAdd will still need wrapping if this behaviour is desirable.

Answer 1

Generally, the code is simple and easy to understand.

Concurrency

It's good that you have a dedicated Lock object. You might consider a more expressive name, but in this case I think you can get away with it.

As you have noticed, using the lock means you gain nothing from the ConcurrentDictionary'2. Since this is such a simple interface, I would keep the lock and ditch the ConcurrentDictionary unless you have a serious (and measurable) performance concern.

However, you should be aware that by locking while you call Set, you are blocking access while you wait for an operation to complete, the length of which you cannot know. Addressing this without changing the class' behaviour would be non-trivial, primarily because it would complicate the case where Set fails, but may be necessary if Set is slow or needs access to the cache itself. If you don't care how many times you call Set, you could just use ConcurrentDictionary.GetOrAdd as mentioned by CharlesNRice in a comment, which would simply things somewhat.

Other

• It's good that you have provided some level of inline documentation. I would be inclined to add an explanation of the threading behaviour (e.g. blocks while calling Set, will only call Set once). "a thread-safe way" is pretty meaningless: it's much better to explain the guarantees you will make.

• Do you anticipate needing to change Set? If not, you should consider making it readonly and assign it in a constructor. There is a design concern that changing Set means you can't know which version was used by any call to Get from another thread.

• Using a Tuple<int?, string> as a union doesn't seem a great idea. Either implement a sound union as a class of struct, or consider having a separate cache for each. If you do wish to keep this API, you should enforce the assumption that exactly one of them is null by explicitly checking and throwing if this is not the case, since it represents an error on the part of the caller, and telling them them sooner rather than later will save them lots of misery and time spent debugging.

• Shelby115: Thanks, I hadn't thought about making Set readonly. Definitely a good point about "a thread-safe way" being meaningless. The last bullet has already been fixed, noticed yesterday after posting that I wasn't even using the string portion, so I removed it.