Generic, thread-safe MemoryCache manager for C#

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
{
///
/// 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.
///
public static class CacheManager
{

///
/// The cache store. A dictionary that stores different memory caches by the type being cached.
///
private static ConcurrentDictionary cacheStore;

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

#region constructors

///
/

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
{
    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 : 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(string cacheKey, T cacheItem)
{
    Set(cacheKey, cacheItem, DefaultMinutes);
}
static internal void Set(string cacheKey, T cacheItem, int minutes) { /* ... */ }

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

static internal void Set(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

StackExchange Code Review Q#48148, answer score: 19