Simple generic cache

Question

I sometimes write decorators to cache results I get from a repository or I write other caches... I don't want to write another cache anymore. I'd like to have something that'll work for anything. I've build a test-cache that I could use to replace other caches. It provides just the most basic methods.

public class Cache
{
    private readonly IDictionary<object, (object Value, DateTime CreatedOn, TimeSpan Timeout)> _cache = new Dictionary<object, (object Value, DateTime CreatedOn, TimeSpan Timeout)>();

    public TResult Get<TResult>(object key, Func<TResult> get, TimeSpan timeout)
    {
        if (_cache.TryGetValue(key, out var t) && !HasTimedout(t.CreatedOn, t.Timeout))
        {
            return (TResult)t.Value;
        }

        var value = get();
        _cache[key] = (value, DateTime.UtcNow, timeout);

        return value;

        bool HasTimedout(DateTime createdOn, TimeSpan currentTimeout) => DateTime.UtcNow - createdOn > currentTimeout;
    }

    public async Task<TResult> GetAsync<TResult>(object key, Func<TResult> get, TimeSpan timeout)
    {
        return await Task.Run(() => Get<TResult>(key, get, timeout));
    }

    public void Clear() => _cache.Clear();

    public bool Remove(object key) => _cache.Remove(key);
}

On its own this is not very useful yet so I have a few extensions for it that help me to create keys which are tuples so that I can use their comparer that compares all properties and not the object reference:

public static class CacheExtensions
{
    public static TResult Get<TResult>(this Cache cache, Func<TResult> get, TimeSpan timeout)
    {
        return cache.Get<TResult>(null, get, timeout);
    }

    public static TResult Get<T1, TResult>(this Cache cache, T1 arg1, Func<T1, TResult> get, TimeSpan timeout)
    {
        return cache.Get<TResult>((arg1), () => get(arg1), timeout);
    }

    public static  TResult Get<T1, T2, TResult>(this Cache cache, T1 arg1, T2 arg2, Func<T1, T2, TResult> get, TimeSpan timeout)
    {
        return cache.Get<TResult>((arg1, arg2), () => get(arg1, arg2), timeout);
    }
}

Example usage:

var cache = new Cache();

cache.Get(2, 4, (x, y) => 3, TimeSpan.FromSeconds(4)).Dump("New");
cache.Get(2, 4, (x, y) => 4, TimeSpan.FromSeconds(4)).Dump("Cached");
Thread.Sleep(5000);
cache.Get(2, 4, (x, y) => 5, TimeSpan.FromSeconds(4)).Dump("Timedout");

Output:

New
3 

Cached
3 

Timedout
5

What do you think of it?

Answer 1

You say cache I say memory leak.

You haven't given much thought to your eviction policy yet and that's 95% of the work in writing a decent cache. Your expired entries are only removed when someone tries to access them.

Consider the case when the cache is used poorly - i.e. you end up caching a huge number of things that are only accessed once. Without a subsequent access after they are expired, they aren't removed and you have a whole bunch of wasted memory.

Your cache also lacks thread safety - that's also a really, really bad thing for a general cache that you intend on using everywhere. A cache doesn't need to be thread safe if you intend on using it in a single threaded application. However, I'd argue that isn't how most caches are used.

Why don't you use something like MemoryCache to do all the hard work?

Answer 2

Some nice syntactic tricks in there which I didn't know about, so firstly thanks for that.

Aside from that I don't have much to say about the code, but there are a few things about the design which don't convince me.

Why one class without type parameters?

This has two aspects.

Firstly, I would prefer to use one instance per logical cache and eliminate the cast (TResult)t.Value (which can quite easily fail if I don't enforce consistency between every call to Get). Having multiple caches seems like a small cost compared to the gain which is compile-time type checking. If this class is only really intended for use cases where each value has a different type, that should be clearly communicated in the docs.

Secondly, I agree that it's not very useful without the extensions, and I think that's because the extension method Get<T1, TResult> is more fundamental than the class method Get<TResult>.

Putting those together, I think it would make more sense to have Cache<TKey, TResult> and add extension methods along the lines of TResult Get<T1, T2, TResult>(this Cache<Tuple<T1, T2>, TResult> cache, T1 arg1, T2 arg2, Func<T1, T2, TResult> get, TimeSpan timeout) to box/unbox tuples.

---

What's the async method for?

    public async Task<TResult> GetAsync<TResult>(object key, Func<TResult> get, TimeSpan timeout)
    {
        return await Task.Run(() => Get<TResult>(key, get, timeout));
    }

For a start, the async and await keywords aren't doing anything useful here: they just wrap an extra, unnecessary, task around the explicitly created one. But secondly, the only reason I can see for wanting a GetAsync is if the get callback is slow. Would it not make more sense then to take a Func which returns a Task<TResult> and await that task?

Answer 3

This use-case looks fishy to me:

cache.Get(2, 4, (x, y) => 3, TimeSpan.FromSeconds(4)).Dump("New");
cache.Get(2, 4, (x, y) => "string", TimeSpan.FromSeconds(4)).Dump("Cached"); // crash

It is error-prone and it allows you to store completely unrelated entities in single cache (bad idea in general).

I would probably move timeout and Func<TResult> to constructor, making Cache class generic and, therefore, strongly typed.

Answer 4

I use this wrapper around MemoryCache which may serve your needs. The AddOrGetExisting method is what you were asking about in the comments on the accepted answer. By using the Lazy<> class to wrap the value, it ensures thread safety and ensures that the factory is only called once.

This code does not satisfy your requirements in two key ways: first it requires a string as a key, and second it does not have an async version. The string key is a requirement of MemoryCache. I suppose you could serialize your tuples to use as key if you really need to. As noted by other answers, having an async version is not really necessary.

/// <summary>
/// Wrapper to simplify working with MemoryCache using a pattern that is efficient and thread-safe.
/// </summary>
public class MemCache
{
    /// <summary>
    /// Get data from the cache or create a new instance of the data and add it to the cache.
    /// </summary>
    /// <typeparam name="T">Type of data to be cached.</typeparam>
    /// <param name="key">Cache key for the item.</param>
    /// <param name="getter">Delegate called to get a new instance of the data if it does not exist.</param>
    /// <param name="policy">Optional cache item policy; defaults to absolute expiration in 15 minutes.</param>
    /// <returns>The requested data.</returns>
    /// <remarks>
    /// This method ensures that the getter for the data is only called once, even if multiple 
    /// threads request the same item concurrently.
    /// </remarks>
    public static T Get<T>(string key, Func<T> getter, CacheItemPolicy policy = null)
    {
        var newItem = new Lazy<T>(getter);
        var existingItem = (Lazy<T>)MemoryCache.Default.AddOrGetExisting(key, newItem, policy ?? DefaultPolicy);
        return (existingItem ?? newItem).Value;
    }

    public static CacheItemPolicy DefaultPolicy { get { return new CacheItemPolicy { AbsoluteExpiration = DateTime.Now.AddMinutes(15) }; } }
}

Answer 5

I see no reason for Task.Run(() => ...) here, why are you doing that? In fact, that's just totally wrong. You have async methods and tasks for the sake of them, even though retrieving a key is not an async operation. Remove them.