6
\$\begingroup\$

In our application, we have many function calls that we want to cache the results of. However, the result of these functions depends on database calls, so we want this cache to refresh every so often so that new data is pulled from the database on a regular basis. (Note: we know that these values are changing once a day, so we don't have to refresh the cache very often)

We currently have a caching system in place that stores the results in a ConcurrentDictionary, so I want to replace that backing with a TimeSensitiveConcurrentDictionary. If the current time is past the expiration time stored for the provided parameters, it will instead return the provided value. In our case, that provided value is a Lazy<R>

Below is the full code to create a cache

public static class FuncUtil_TimeSensitive
{
    public static readonly TimeSpan ValidTime = TimeSpan.FromHours(6);

    public static Func<A, R> Memoize<A, R>(Func<A, R> f)
    {
        var map = DictionaryHelper<Lazy<R>>.CreateConcurrent(new { a = default(A) });
        return a => map.GetOrAdd(new { a }, new Lazy<R>(() => f(a))).Value;
    }

    public static Func<A, B, R> Memoize<A, B, R>(this Func<A, B, R> f)
    {
        var map = DictionaryHelper<Lazy<R>>.CreateConcurrent(new { a = default(A), b = default(B) });
        return (a, b) => map.GetOrAdd(new { a, b }, new Lazy<R>(() => f(a, b))).Value;
    }

    //other methods

    static class DictionaryHelper<Value>
    {
        public static TimeSensitiveConcurrentDictionary<Key, Value> CreateConcurrent<Key>(Key prototype)
        {
            return new TimeSensitiveConcurrentDictionary<Key, Value>(ValidTime);
        }
    }
}

/// <summary>
/// Stores a value based on the key for a set amoutn of time. After that time has passed, accessing the same key will remove the value
/// </summary>
/// <typeparam name="TKey">Type to use as a key</typeparam>
/// <typeparam name="TValue">Type to use as a value</typeparam>
public class TimeSensitiveConcurrentDictionary<TKey, TValue>
{
    private readonly ConcurrentDictionary<TKey, TimeSensitiveConcurrentDictionaryValue<TValue>> cache
        = new ConcurrentDictionary<TKey, TimeSensitiveConcurrentDictionaryValue<TValue>>();

    private readonly TimeSpan expirationTime;

    public TimeSensitiveConcurrentDictionary(TimeSpan validTime)
    {
        expirationTime = validTime;
    }

    public TValue GetOrAdd(TKey key, TValue newValue)
    {
        var now = DateTime.Now;
        bool needsToUpdate = false;
        var currentExpirationDate = now.Add(expirationTime);
        if (cache.ContainsKey(key))
        {
            currentExpirationDate = cache[key].ExpirationDate;
            if (now > currentExpirationDate)
            {
                TryRemove(key);
                needsToUpdate = true;
            }
        }
        else
        {
            needsToUpdate = true;
        }
        if (needsToUpdate)
        {
            currentExpirationDate = now.Add(expirationTime);
        }

        return cache.GetOrAdd(key, new TimeSensitiveConcurrentDictionaryValue<TValue>(currentExpirationDate, newValue)).Value;
    }
    public bool TryRemove(TKey a)
    {
        TimeSensitiveConcurrentDictionaryValue<TValue> result;
        return cache.TryRemove(a, out result);
    }

    private struct TimeSensitiveConcurrentDictionaryValue<TTSValue>
    {
        private DateTime expirationDate;
        private TTSValue value;

        public DateTime ExpirationDate { get { return expirationDate; } }
        public TTSValue Value { get { return value; } }

        public TimeSensitiveConcurrentDictionaryValue(DateTime expirationDate, TTSValue value)
        {
            this.expirationDate = expirationDate;
            this.value = value;
        }
    }
}

Example Usage:

Func<int, string> MyCache = FuncUtil_TimeSensitive.Memoize<int, string>(a => {
    //retrieve a value from the database, return the result
    return MyDBContext.Table.Single(x => x.Field == a).SomeStringValue;
});

string myDBValue = MyCache(1); //hits database
string sameValue = MyCache(1); //hits cache
Thread.Sleep(6 * 60 * 60 * 1000); //sleep for a while
string newValueFromDB = MyCache(1); //key has expired, hit database again for new value

Other notes: this does need to be thread-safe.

Does this implementation seem valid? I am not an expert on concurrency. I would also accept public implementations that I could use instead of writing my own.

One thought I had: Would I benefit from making it inherit from IDictionary<> or from ConcurrentDictionary? From what I hear, you shouldn't inherit from collections, and I don't plan on using this for other purposes, so I'm not sure if it's worth it to make this more robust.

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! That looks like an interesting question. I hope you get good reviews! \$\endgroup\$ – Phrancis Nov 19 '14 at 17:16
  • \$\begingroup\$ @DLeh Welcome to Code Review. Please do not update the code in the question after answers have been submitted. See here. When you have time, please take a look at the tour for a complete overview of policies and etiquette on CR.SE. \$\endgroup\$ – psaxton Nov 19 '14 at 20:53
  • \$\begingroup\$ @psaxton thanks, I thought there might have been a rule about updating posts, but I was unable to find the post about it by searching. I will keep this in mind next time. I'm very familiar with the SE UI, but CR is a bit different I guess. \$\endgroup\$ – DLeh Nov 19 '14 at 20:58
3
\$\begingroup\$
  1. This class name FuncUtil_TimeSensitive is a bit unusual - FuncUtil sounds like a namespace so you should use a proper namespace rather then prefixing you class name with it.

  2. Bad parameter naming - single letter parameter names are not very descriptive.

  3. Lack of documentation - shared general utility classes should have xml doc comments for the class and the public methods.

  4. Magic hard coded constant here: new TimeSpan(6, 0, 0);

    • Better would have been to use TimeSpan.FromHours(6) because then I wouldn't have had to look up what the three parameter constructor was.
    • You can't practically unit test this because your unit test would have to sleep for 6 hours before the expiration is hit - this should be a configurable setting with a default value.
  5. I don't really think there is actually a need for the FuncUtil_TimeSensitive helper class - I would have added the factory methods to the TimeSensitiveConcurrentDictionary.

  6. I don't like that you use two ConcurrentDictionaries to track two different values for the same key (one for the timeout and one for the actual value). A better way would be to encapsulate that into a member class which is responsible for tracking the timeout.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the input. 1. The class name is a temporary name while we analyze it. 4. This is also a placeholder time. I didn't know about TimeSpan.FromHours, thanks. Any unit testing I would do would be on the dictionary object itself, not on the FuncUtil class. 5. This pattern was copied from our existing class, I believe we need something like it to enable us to have anonymous types as a type parameter, since the compiler can infer the type. I don't know of another way to provide that anonymous type as a generic param. 6. I've updated this to store the time and value in one dict \$\endgroup\$ – DLeh Nov 19 '14 at 19:44
1
\$\begingroup\$

My teammate found one bug in this code. There is a multithreaded issue with the cache access when assessing if the value has expired. TryRemove could have removed the key when it was checked from another thread.

Here's what the GetOrAdd method should look like, with the default currentExpirationDate changed, and a TryGetValue instead of index access on the cache.

    public TValue GetOrAdd(TKey key, TValue newValue)
    {
        var now = DateTime.Now;
        bool needsToUpdate = false;
        var currentExpirationDate = now;
        if (cache.ContainsKey(key))
        {
            TimeSensitiveConcurrentDictionaryValue<TValue> tempVal;
            if (cache.TryGetValue(key, out tempVal))
            {
                currentExpirationDate = tempVal.ExpirationDate;
            }
            if (now >= currentExpirationDate)
            {
                TryRemove(key);
                needsToUpdate = true;
            }
        }
        else
        {
            needsToUpdate = true;
        }
        if (needsToUpdate)
        {
            currentExpirationDate = now.Add(expirationTime);
        }

        return cache.GetOrAdd(key, new TimeSensitiveConcurrentDictionaryValue<TValue>(currentExpirationDate, newValue)).Value;
    }
\$\endgroup\$

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.