15
\$\begingroup\$

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;
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ I am providing a small suggestion. Please invert your if statements. It's easier to return from a quick if than to wrap the contents of entire methods in them. Makes code more readable, and less layers of "nestiness"... \$\endgroup\$ – bleepzter Sep 19 '16 at 19:02
18
\$\begingroup\$

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;
    }

}
\$\endgroup\$
  • \$\begingroup\$ In the TryGet method, use returnItem = this[cacheKey] as T; instead. It won't throw an exception then if the type cast fails. \$\endgroup\$ – Stuart Blackler Apr 27 '14 at 8:52
  • \$\begingroup\$ @StuartBlackler If the typecast fails, something has gone horribly, horribly wrong, and we want to know about it. The whole point of this wrapper seems to be so only objects of one type wind up in the collection... so if a type that isn't T got in, an explosion is quite warranted. With that said, it would probably be a Good Thing to enforce that by overriding all the base class methods that could add non T type items to the collection, and throw at the point of addition, instead of the point of extraction. :) \$\endgroup\$ – Travis Snoozy Apr 27 '14 at 17:59
  • \$\begingroup\$ @TravisSnoozy You make a lot of great points. Thank you. While I can't say that supporting structs is absolutely necessary, I also don't want to rule it out. So I would keep default(T). I was not comfortable with the UpdateItem method, but couldn't point out why (code smell). With the sample rewrite you provided, would that mean that the developer would need to set up a global singleton for each type they wanted to cache? \$\endgroup\$ – egandalf Apr 28 '14 at 14:13
  • \$\begingroup\$ @egandalf that is correct -- this rewrite is not static, and therefore one way you could share it with the parts of your program that need access to the cache would be to stick an instance behind a singleton pattern. However, by not being static itself, it does not force a single, global cache -- allowing for other access patterns (esp. in cases where you don't want any random bit of code being able to add/remove items from the cache). If you do make a global singleton, consider making it "internal" scope. :) \$\endgroup\$ – Travis Snoozy Apr 30 '14 at 0:42
  • \$\begingroup\$ MemoryCache class is thread safe. stackoverflow.com/a/6738179/665783 \$\endgroup\$ – Jacob Mar 2 '17 at 14:27
3
\$\begingroup\$

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;
}
\$\endgroup\$
  • \$\begingroup\$ Would you always initialise WriteLock or use InterLocked.CompareExchange instead? \$\endgroup\$ – dfhwze May 19 at 8:06
  • \$\begingroup\$ What would you compare with InterLocked.CompareExchange? \$\endgroup\$ – evilpurple May 22 at 15:16
  • \$\begingroup\$ now you always initialize your lock, even if you never call TryGetOrSet, you could also lazy initialize that lock with InterLocked. Or is that overkill? \$\endgroup\$ – dfhwze May 22 at 15:27
  • \$\begingroup\$ You could use something like private object writeLock; private object WriteLock => writeLock ?? (writeLock = new object()). But I'm not quite sure about concurrent lock race conditions. \$\endgroup\$ – evilpurple May 22 at 15:42
  • \$\begingroup\$ social.msdn.microsoft.com/Forums/vstudio/en-US/… -> the first paragraph, the lock is created with InterLocked \$\endgroup\$ – dfhwze May 22 at 16:02
0
\$\begingroup\$

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.

\$\endgroup\$
  • 3
    \$\begingroup\$ Hi! Welcome to Code Review. A good first answer, but I would suggest to add an example of what you are trying to do. This way, it would be easier for the asker to just put that into his/her code, or edit it to match yours. \$\endgroup\$ – TheCoffeeCup May 15 '15 at 19:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.