Generic, thread-safe MemoryCache manager for C#

Question

Using this question as a base, and using some of the advice in the answers, I wanted to build out something that would be generic, thread-safe, and easy to use for at least one current and several future projects.

The idea is to be able to call one function, passing a key and passing another function to generate the data, if needed. It returns true/false to indicate success and saves the data to an OUT parameter. There's a default cache time but also overload methods that allow the developer to pass a new absolute duration (in minutes).

The class and functions work, I can step through the code and see that it hits the passed function the first time, then skips it in subsequent requests.

The external static call to IsCachingEnabled is basically just looking for an AppSetting in the web.config.

Some functions are marked internal but could be set to public - they're there partly as an offshoot of the original sample and partly because I may enable them in the future. For my current project, I'm only interested in using the TryGetAndSet method from outside.

Here's the code. I'm interested to hear whether anything here just doesn't make sense, isn't thread-safe, or is just plain bad practice.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Web;
using System.Runtime.Caching;
using System.Collections.Concurrent;

namespace Ektron.Com
{
    /// <summary>
    /// Uses System.Runtime.Caching to provide a thread-safe caching class.
    /// Recommended use is to employ the TryGetAndSet method as a wrapper to call
    /// your data-building function.
    /// </summary>
    public static class CacheManager
    {

        /// <summary>
        /// The cache store. A dictionary that stores different memory caches by the type being cached.
        /// </summary>
        private static ConcurrentDictionary<Type, ObjectCache> cacheStore;

        /// <summary>
        /// The default minutes (15)
        /// </summary>
        private const int DefaultMinutes = 15;

        #region constructors

        /// <summary>
        /// Initializes the <see cref="CacheManager"/> class.
        /// </summary>
        static CacheManager()
        {
            cacheStore = new ConcurrentDictionary<Type, ObjectCache>();
        }
        #endregion

        #region Setters

        /// <summary>
        /// Sets the specified cache using the default absolute timeout of 15 minutes.
        /// </summary>
        /// <typeparam name="T"></typeparam>
        /// <param name="cacheKey">The cache key.</param>
        /// <param name="cacheItem">The data to be cached.</param>
        static internal void Set<T>(string cacheKey, T cacheItem)
        {
            Set<T>(cacheKey, cacheItem, DefaultMinutes);
        }

        /// <summary>
        /// Sets the specified cache using the absolute timeout specified in minutes.
        /// </summary>
        /// <typeparam name="T"></typeparam>
        /// <param name="cacheKey">The cache key.</param>
        /// <param name="cacheItem">The data to be cached.</param>
        /// <param name="minutes">The absolute expiration (in minutes).</param>
        static internal void Set<T>(string cacheKey, T cacheItem, int minutes)
        {
            if (Ektron.Com.Helpers.Constants.IsCachingEnabled)
            {
                Type t = typeof(T);
                if (!cacheStore.ContainsKey(t))
                {
                    RegisterCache(t);
                }
                var cache = cacheStore[t];
                cache.Set(cacheKey, cacheItem, GetCacheItemPolicy(minutes));
            }
        }

        /// <summary>
        /// Sets the specified cache using the passed function to generate the data. 
        /// Uses default absolute timeout of 15 minutes.
        /// </summary>
        /// <typeparam name="T"></typeparam>
        /// <param name="cacheKey">The cache key.</param>
        /// <param name="getData">The function to generate the data to be cached.</param>
        static internal void Set<T>(string cacheKey, Func<T> getData)
        {
            Set<T>(cacheKey, getData, DefaultMinutes);
        }

        /// <summary>
        /// Sets the specified cache using the passed function to generate the data. 
        /// Uses the specified absolute timeout (in minutes).
        /// </summary>
        /// <typeparam name="T"></typeparam>
        /// <param name="cacheKey">The cache key.</param>
        /// <param name="getData">The function to generate the data to be cached.</param>
        /// <param name="minutes">The absolute expiration (in minutes).</param>
        static internal void Set<T>(string cacheKey, Func<T> getData, int minutes)
        {
            if (Ektron.Com.Helpers.Constants.IsCachingEnabled)
            {
                Type t = typeof(T);
                if (!cacheStore.ContainsKey(t))
                {
                    RegisterCache(t);
                }
                var cache = cacheStore[t];
                T data = getData();
                cache.Set(cacheKey, data, GetCacheItemPolicy(minutes));
            }
        }
        #endregion

        #region Getters
        /// <summary>
        /// Tries to retrieve data from cache first. If the data is not found in cache, the passed function
        /// will be used to generate and store the data in cache. Data is returned via the returnData parameter.
        /// Function returns true if successful.
        /// Uses the default absolute timeout of 15 minutes.
        /// </summary>
        /// <typeparam name="T"></typeparam>
        /// <param name="cacheKey">The cache key.</param>
        /// <param name="getData">The function to generate the data to be cached.</param>
        /// <param name="returnData">The return data.</param>
        /// <returns>True if successful. False if data is null.</returns>
        public static bool TryGetAndSet<T>(string cacheKey, Func<T> getData, out T returnData)
        {
            if (!Ektron.Com.Helpers.Constants.IsCachingEnabled)
            {
                Remove<T>(cacheKey);
            }
            Type t = typeof(T);
            bool retrievedFromCache = TryGet<T>(cacheKey, out returnData);
            if (retrievedFromCache)
            {
                return true;
            }
            else
            {
                returnData = getData();
                Set<T>(cacheKey, returnData);
                return returnData != null;
            }
        }

        /// <summary>
        /// Tries to retrieve data from cache first. If the data is not found in cache, the passed function
        /// will be used to generate and store the data in cache. Data is returned via the returnData parameter.
        /// Function returns true if successful.
        /// Uses the specified absolute timeout (in minutes).
        /// </summary>
        /// <typeparam name="T"></typeparam>
        /// <param name="cacheKey">The cache key.</param>
        /// <param name="getData">The function to generate the data to be cached.</param>
        /// <param name="minutes">The absolute expiration (in minutes).</param>
        /// <param name="returnData">The return data.</param>
        /// <returns>True if successful. False if data is null.</returns>
        public static bool TryGetAndSet<T>(string cacheKey, Func<T> getData, int minutes, out T returnData)
        {
            Type t = typeof(T);
            bool retrievedFromCache = TryGet<T>(cacheKey, out returnData);
            if (retrievedFromCache && Ektron.Com.Helpers.Constants.IsCachingEnabled)
            {
                return true;
            }
            else
            {
                returnData = getData();
                Set<T>(cacheKey, returnData, minutes);
                return returnData != null;
            }
        }

        /// <summary>
        /// Attempts to retrieve data from cache.
        /// </summary>
        /// <typeparam name="T"></typeparam>
        /// <param name="cacheKey">The cache key.</param>
        /// <param name="returnItem">The data from cache.</param>
        /// <returns>True if successful. False if data is null or not found.</returns>
        static internal bool TryGet<T>(string cacheKey, out T returnItem)
        {
            Type t = typeof(T);
            if (cacheStore.ContainsKey(t))
            {
                var cache = cacheStore[t];
                object tmp = cache[cacheKey];
                if (tmp != null)
                {
                    returnItem = (T)tmp;
                    return true;
                }
            }
            returnItem = default(T);
            return false;
        }
        #endregion

        /// <summary>
        /// Removes the specified item from cache.
        /// </summary>
        /// <typeparam name="T"></typeparam>
        /// <param name="cacheKey">The cache key.</param>
        static internal void Remove<T>(string cacheKey)
        {
            Type t = typeof(T);
            if (cacheStore.ContainsKey(t))
            {
                var cache = cacheStore[t];
                cache.Remove(cacheKey);
            }
        }

        /// <summary>
        /// Registers the cache in the dictionary.
        /// </summary>
        /// <param name="t">The type used as the key for the MemoryCache that stores this type of data.</param>
        private static void RegisterCache(Type t)
        {
            ObjectCache newCache = new MemoryCache(t.ToString());
            cacheStore.AddOrUpdate(t, newCache, UpdateItem);
        }

        /// <summary>
        /// Updates the item. Required for use of the ConcurrentDictionary type to make this thread-safe.
        /// </summary>
        /// <param name="t">The Type used as the key for the MemoryCache that stores this type of data.</param>
        /// <param name="cache">The cache to be updated.</param>
        /// <returns></returns>
        private static ObjectCache UpdateItem(Type t, ObjectCache cache)
        {
            var newCache = new MemoryCache(cache.Name);
            foreach (var cachedItem in cache)
            {
                newCache.Add(cachedItem.Key, cachedItem.Value, GetCacheItemPolicy(DefaultMinutes));
            }
            return newCache;
        }

        /// <summary>
        /// Gets the cache item policy.
        /// </summary>
        /// <param name="minutes">The absolute expiration, in minutes.</param>
        /// <returns>A standard CacheItemPolicy, varying only in expiration duration, for all items stored in MemoryCache.</returns>
        private static CacheItemPolicy GetCacheItemPolicy(int minutes = 15)
        {
            var policy = new CacheItemPolicy()
            {
                Priority = CacheItemPriority.Default,
                AbsoluteExpiration = DateTimeOffset.Now.AddMinutes(minutes)
            };
            return policy;
        }
    }
}

Answer 1

Short version: it looks like you're just trying to get a per-type ObjectCache. That's a basic singleton pattern, so I'm unsure what you're trying to gain through all this extra cruft in the class. If it were me, I would consider the following code:

public static class GlobalTypedObjectCache<T>
{
    public static ObjectCache Cache { get; private set; }
    static GlobalTypedObjectCache()
    {
        Cache = new MemoryCache(typeof(T).ToString());
    }
}

to be a much simpler, nearly functional equivalent to what you've got going. If you want the convenience functions, you can implement them in a much simpler fashion based on the above template. I'd take it even further, and suggest that you throw the singleton out, make a generic/templated class that derives from MemoryCache, and implement your convenience functions there:

public class TypedObjectCache<T> : MemoryCache
{
    LocalTypedObjectCache(string name, NameValueCollection nvc = null) : base(name, nvc) { }
}

That way, you can throw this class behind a singleton as-needed for specific caching purposes. With those broad observations made, down to specifics...

Scope

Because the class is static, there can be only one cache in the system. However, because it's also effectively a generic, you really wind up with one cache per type (so the key "foo" refers to different things in and ). IMHO, you should strive for consistency, and either have a non-static, generic version of the class (which you can then put behind a singleton for specific cache applications, ensuring "foo" exists only once for a well-defined functional scope, rather than an implicit key scope based arbitrarily on type), OR have a fully global, Object-based cache that exists once and only once (so that the key "foo" cannot exist in more than one form in the cache). Of these two options, I would generally consider it more appropriate to have a non-static class and leave it to your consumers to put an instance behind a singleton pattern.

Call Consistency

The class has three functions (aside from the constructor) that don't use a generic type specifier. Two of those three functions instead take a Type parameter. Either all of the functions should take a Type parameter (if you're trying to reduce the amount of code generated), or all of the functions should use template typing. In the former case, you may as well make the entire static class take a type, and just call the functions without the type (note that this will mean there's a static class for each type, rather than a single static class containing all types, which means that the code can be further refactored down).

Behavior Consistency

There's a check in some of your functions for seeing if caching is enabled, whereas other functions omit these checks. You need to determine the consistent behavior you want (no gets, no sets if caching is turned off? No sets, but gets still allowed?), and then implement it across all of your functions that will do getting, setting, or both. Note also that there is a threading issue if the caching flag can switch between on/off during runtime, as some of your code paths have multiple checks of that flag.

Default Values

Instead of two declarations, e.g.:

static internal void Set<T>(string cacheKey, T cacheItem)
{
    Set<T>(cacheKey, cacheItem, DefaultMinutes);
}
static internal void Set<T>(string cacheKey, T cacheItem, int minutes) { /* ... */ }

Just use a default argument, and implement the function once:

 static internal void Set<T>(string cacheKey, T cacheItem, int minutes = DefaultMinutes) { /* ... */ }

Your GetCacheItemPolicy function should also use the named constant instead of a hard-coded 15 for its default value. The one place that it's called (inside UpdateItem) should simply omit any value being passed to take the default.

Cache Management

The cache is being forced into a default of absolute timeout, and then you provide UpdateItem... which goes through and forcefully resets every item's expiry in the cache. This leads us to (effectively) an all-or-nothing situation -- either everything will expire out of the cache at the same time, OR nothing will ever be expired from the cache (because it's being updated faster than the expiry time). This more or less defeats the purpose of caching, especially at such a broad scale (all items of the same type at the global/static level). Consider reviewing your other options re. CacheItemPolicy (like SlidingExpiration), staying out of the way, and letting ObjectCache do the job it was written to do.

Registration Thread Unsafe

The registration code will cause your old caches to get thrown out, if you register the same type twice. If you're going to keep it, you should make sure that you don't allow nuking entries by accident, because two people tried to register the first object of a type at the same time. That said, I've listed several ways by which you could do away with the registration altogether... :)

Null Coalescing

The null coalescing operator (??) can be used to trim down your code a little bit, rather than using complete "if" statements to do null checks and data merges.

Type Constraints

If you use type constraints (and didn't need to cache structs), then you can get rid of your need for default(T). This may or may not be acceptable for your needs.

Example Rewrite

I leave comments as an exercise to the OP. I don't agree with all of the return values (e.g., IMO the bools should indicate whether the item was serviced from the cache or not -- the caller can do its own null checks, but it can't otherwise figure out a cache hit or miss), but I leave the behavior of the OP mostly intact.

using System;
using System.Collections.Specialized;
using System.Runtime.Caching;

public class TypedObjectCache<T> : MemoryCache where T : class
{
    private CacheItemPolicy HardDefaultCacheItemPolicy = new CacheItemPolicy()
    {
        SlidingExpiration = new TimeSpan(0, 15, 0)
    };

    private CacheItemPolicy defaultCacheItemPolicy;

    public TypedObjectCache(string name, NameValueCollection nvc = null, CacheItemPolicy policy = null) : base(name, nvc)
    {
        defaultCacheItemPolicy = policy??HardDefaultCacheItemPolicy;
    }

    public void Set(string cacheKey, T cacheItem, CacheItemPolicy policy = null)
    {
        policy = policy??defaultCacheItemPolicy;
        if ( true /* Ektron.Com.Helpers.Constants.IsCachingEnabled */ )
        {
            base.Set(cacheKey, cacheItem, policy);
        }
    }

    public void Set(string cacheKey, Func<T> getData, CacheItemPolicy policy = null)
    {
        this.Set(cacheKey, getData(), policy);
    }

    public bool TryGetAndSet(string cacheKey, Func<T> getData, out T returnData, CacheItemPolicy policy = null)
    {
        if(TryGet(cacheKey, out returnData))
        {
            return true;
        }
        returnData = getData();
        this.Set(cacheKey, returnData, policy);
        return returnData != null;
    }

    public bool TryGet(string cacheKey, out T returnItem)
    {
        returnItem = (T)this[cacheKey];
        return returnItem != null;
    }

}

Answer 2

This is an addition to the accepted answer.

To have a really thread safe implementation of the derived ObjectCache you need to double check the TryGet() call. True, the ObjectCache uses a Monitor to manage possible race conditions, but the Func<T> getData will be called two times if a race condition exists.

So a possible addition is adding a simple lock and double check the data.

[...]

private object WriteLock { get; } = new object();

[...]

public void TryGetOrSet( string cacheKey, Func<T> getData, out T returnData, CacheItemPolicy policy = null )
{
    if( TryGet( cacheKey, out returnData ) )
        return true;

    lock( WriteLock )
    {
        if( TryGet( cacheKey, out returnData ) )
            return true;

        returnData = getData();
        Set( cacheKey, returnData, policy );
    }

    return false;
}

Answer 3

In the rewrite TryGetAndSet, it might be better return false and not set the cache entry if the returned value is null.

With the documented implementation, the first call to TryGetAndSet will return false and out a null value. Subsequent calls will return true and a null value.