14
\$\begingroup\$

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?

\$\endgroup\$
  • 1
    \$\begingroup\$ I would look at how Microsoft is building their MemoryCache for Asp.net Core. github.com/aspnet/Caching/blob/dev/src/… It has proper multithreading and cache eviction support. \$\endgroup\$ – Andrew C Jul 17 '17 at 21:26
  • \$\begingroup\$ @AndrewC oh, this is interesting, the asp.net core cache has an object key but the normal framework cache only a string. \$\endgroup\$ – t3chb0t Jul 18 '17 at 4:15
27
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ I didn't know the MemoryCache ;-( ... on the other hand it does not support (or I cannot see it) a Func for getting new values. It's a lot of work to refresh an item. I'd like to be able to do GetValueOrAddOrUpdate, everything in a single call. I'll need to fix the other things... I don't want to implement the same logic over and over again ;-) \$\endgroup\$ – t3chb0t Jul 17 '17 at 11:53
  • \$\begingroup\$ Does MemoryCache really work? I have never been able to find much information on it but I implemented it for something once and even with a sliding expiry of 15 minutes it seems like every other request the contents are no longer cached. \$\endgroup\$ – Marie Jul 17 '17 at 14:24
  • \$\begingroup\$ @Marie - I've never had an issue with it but I don't generally require sliding expiration so I don't know for your case. \$\endgroup\$ – RobH Jul 17 '17 at 14:37
  • \$\begingroup\$ Whether lack of thread safety is "a really really bad thing" or not depends on why you want a cache. Caching is not inherently restricted to multi-threaded scenarios, and thread safety is not free. IMO if you want thread safety you should do it with a wrapper, and at that point you should extract an ICache interface for the thread-safe and non-thread-safe implementations to share. \$\endgroup\$ – Peter Taylor Jul 17 '17 at 14:45
  • 1
    \$\begingroup\$ @PeterTaylor - I agree somewhat. In the OP, it sound as if this was going to be a general cache used by everything. "I don't want to write another cache anymore" - I was just trying to say that this implementation isn't suitable for that. \$\endgroup\$ – RobH Jul 17 '17 at 14:57
8
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ These are all good points and I think expecially the one about the cache not being generic itself requries additional explanation (also mentioned by @NikitaB). I was going to replace with this casche all other decorators that I use for my repositories. Instantiating multiple caches for each return type is a lot of work but I think I know how I can make it more reliable and not crashing on invalid types and support various methods with same parameters and same return types... I'll use expressions to add the member name and type to the key so that it's not longer ambiguous. \$\endgroup\$ – t3chb0t Jul 17 '17 at 11:05
5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Or I can make the result type part of the key... \$\endgroup\$ – t3chb0t Jul 17 '17 at 10:17
  • 1
    \$\begingroup\$ @t3chb0t, well, yes, this would solve the problem. However by doing this you promote the idea of storing everything in single cache instance instead of creating a dedicated caches for every data source. God have mercy upon your soul, if you also want to create a static instance of this cache... \$\endgroup\$ – Nikita B Jul 17 '17 at 11:24
3
\$\begingroup\$

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) }; } }
}
\$\endgroup\$
  • \$\begingroup\$ I like it and I'll keep it for another time ;-) \$\endgroup\$ – t3chb0t Jul 17 '17 at 17:54
  • \$\begingroup\$ I like this solution but it seems we should remove the cache entry if (existingItem ?? newItem).Value; throws an exception.to avoid caching the exception \$\endgroup\$ – NicoD Jan 10 at 16:17
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I have no other explanation for it then because I thought it would be correct. \$\endgroup\$ – t3chb0t Jul 17 '17 at 14:24
  • \$\begingroup\$ It's definitely not correct unless your cache was doing something asynchronously. \$\endgroup\$ – user9993 Jul 17 '17 at 14:29

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.