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?

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.