Simple cache dictionary

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

public class CacheDictionary : Dictionary, ICacheDictionary
{
    private readonly Dictionary _expireTasks = new Dictionary();
    private int _defaultExpiration;

    public CacheDictionary()
    {
        _defaultExpiration = 30;
    }

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

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

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

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

    public T Set(string key, Func 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?

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>. 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.

StackExchange Code Review Q#105106, answer score: 7