Simple cache dictionary

Question

I decided to write a simple dictionary to use as a cache:

public class CacheDictionary : Dictionary<string, object>, ICacheDictionary
{
    private readonly Dictionary<string, CancellationTokenSource> _expireTasks = new Dictionary<string, CancellationTokenSource>();
    private int _defaultExpiration;

    public CacheDictionary()
    {
        _defaultExpiration = 30;
    }

    public CacheDictionary(int defaultExpiration)
    {
        _defaultExpiration = defaultExpiration;
    }

    public T GetOrDefault<T>(string key, Func<T> createDefault)
    {
        return GetOrDefault(key, createDefault, TimeSpan.FromSeconds(_defaultExpiration));
    }

    public T Set<T>(string key, Func<T> create)
    {
        return Set(key, create, TimeSpan.FromSeconds(_defaultExpiration));
    }

    public T GetOrDefault<T>(string key, Func<T> createDefault, TimeSpan expireIn)
    {
        return (T)(ContainsKey(key) ? this[key] : Set(key, createDefault, expireIn));
    }


    public T Set<T>(string key, Func<T> create, TimeSpan expireIn)
    {
        if (_expireTasks.ContainsKey(key))
        {
            _expireTasks[key].Cancel();
            _expireTasks.Remove(key);
        }

        var expirationTokenSource = new CancellationTokenSource();
        var expirationToken = expirationTokenSource.Token;

        Task.Delay(expireIn, expirationToken).ContinueWith(_ => Expire(key), expirationToken);

        _expireTasks[key] = expirationTokenSource;

        return (T)(this[key] = create());
    }

    private void Expire(string key)
    {
        if (_expireTasks.ContainsKey(key))
            _expireTasks.Remove(key);

        Remove(key);
    }
}

The functionality is simple: I can add items into the dictionary and set a timer to expire them. I am wondering if I did everything right. Should I also implement IDisposable and remove expiration tasks on Dispose? And if I want it to be thread-safe, should I use ConcurrentBag?

Answer 1

Thread Safety

Yes, there is a possible race condition here if two threads call Set with the same key. But just using a ConcurrentDictionary won't necessarily be enough, since you're effectively managing a transaction across two dictionaries - the main cache and the expiration task dictionary.

There are several ways to avoid this. One is to use lock around the entire Set statement so that concurrent Set requests on the same key will wait until the first call finishes, and the immediately expires the previous item and replaces it.

A second is to avoid having two dictionaries, and not expose IDictionary in the first place, which I think is the best way to go, for one big reason:

You're currently exposing two very different interfaces in your CacheDictionary. Both the standard IDictionary's Add/[] methods for directly setting/fetching a value, and the ICacheDictionary's Get/Set interface, which is similar to ConcurrentDictionary, and deals with value factories, default values, and a whole set of concepts that IDictionary doesn't deal with, regardless of the actual expiration logic. This can be very confusing to your user, who now has two different ways to do the same thing. If I was using this ICacheDictionary, I wouldn't know if calling myDict[key] = value directly was a good idea or not.

What I would do is just implement ICacheDictionary's Get/Set methods, and keep an internal ConcurrentDictionary<string, Tuple<object, TaskCancellationToken>>. This means you only have one data store, which is thread-safe, and it stores both the value and the expiration task for each entry.

Disposability

Generally speaking, there are two good indicators that you should implement IDisposable to clean up your state. The first is that you're using COM objects/Win32 calls/PInvoke statements to create unmanaged resources. The second is that you're holding instances of IDisposable objects that you might want to dispose explicitly when the container is disposed.

In your case, you don't have any unmanaged code. And as of managed objects, you have two types here - the Dictionary, which doesn't require disposal, and the Task objects, which you don't even bother storing, instead relying on the TaskCancellationSource to handle their cancellation with no way to catch or handle them. Since the official .NET recommendation is to not bother with Disposing Task objects, I think you're in the clear here too.

So no, don't bother implementing IDisposable. You can add a Clear method to clear all items, but that's not the same.

Answer 2

public CacheDictionary()
{
    _defaultExpiration = 30;
}

public CacheDictionary(int defaultExpiration)
{
    _defaultExpiration = defaultExpiration;
}

Personally, I would use constructor chaining instead of setting the same backing field from multiple constructors.

public CacheDictionary()
    :this(defaultExpiration: 30)
{ }

public CacheDictionary(int defaultExpiration)
{
    _defaultExpiration = defaultExpiration;
}

Note the use of a named parameter to make it clear what 30 represents.

I also probably wouldn't inherit from dictionary.

public class CacheDictionary : Dictionary<string, object>, ICacheDictionary

By doing so, you're tightly coupling the interface (not interface) of your cache to its implementation details. I would prefer composition over inheritance. Then you could change out the underlying dictionary without affecting the client code. You're getting a lot of baggage along with the convenience. Look at how many interfaces and members you've just bound yourself to implementing forever. You might run into trouble if you decide to switch over to a ConcurrentDictionary, or some other mechanism entirely. I honestly imagine that your ICacheDictionary should look more like this.

public ICacheDictionary: IDictionary<TKey, TValue>

Answer 3

In addition to the existing answers:

1. You have more than one race condition here: Since the expiration is done via asynchronous tasks you can cause problems even if the cache is used by one thread only (the continuation is most likely to be executed on a thread pool thread). So in it's current form the implementation is essentially broken and will cause random exceptions (depending on the usage patterns).

2. To re-iterate the point which has already been made (since it's important): Because you derive from Dictionary<> you have made it very hard (almost impossible) for yourself to actually fix the race conditions mentioned. Even if you protect your methods against race conditions you can't protect the Dictionary<> methods because they are not virtual (so you can't override them, you could hide them but that's rather ugly). All in all you've made it very easy to break the assumptions your class makes - classic case of leaky abstraction.

3. It always irks me when classes are being designed to take some sort of time measure and the type is an integer type. .NET has a Timespan type - use it. Storing the expiration time as Timespan would also remove the need to convert it into one on every single call to one of get/set methods. If you absolutely don't want/can't use a Timespan then at least add a suffix to the name indicating the unit. Many timeouts in the .NET framework are specified in milliseconds - so using seconds is less intuitive.

4. Regarding the IDisposable topic: One of the rules you should always stick to is that if your class owns an IDisposable object then it should also implement IDisposable and dispose of the owned object. Doing anything else will get you into trouble in the long run.

   Your class owns CancellationTokenSource objects which are IDisposable hence you should implement IDisposable. To quote MSDN:

   The CancellationTokenSource class implements the IDisposable interface. You should be sure to call the CancellationTokenSource.Dispose method when you have finished using the cancellation token source to free any unmanaged resources it holds.