I'm in my early days of working with C# and threading and I'm in need of a data structure that can store key-value pairs like a dictionary, but after a certain time after being added I want the values to "expire", or get removed from the dictionary.

I have come up with an implementation that seems to work, but I was wondering if there might be any problems to it when used in a multi threaded environment.

1. Are there any direct issues with my current implementation?
2. As all usage of the dictionary is protected by the semaphore, is this implementation to be considered thread-safe out of the box?

public class TokenCache<TKey, TValue>
    where TKey : IEquatable<TKey>
    where TValue : class
{
    private readonly TimeSpan expirationTime;
    private readonly SemaphoreSlim semaphore;
    private readonly Dictionary<TKey, TValue> cache;

    public TokenCache(TimeSpan expirationTime)
    {
        this.expirationTime = expirationTime;
        this.semaphore = new SemaphoreSlim(1, 1);
        this.cache = new Dictionary<TKey, TValue>();
    }

    public void Add(TKey key, TValue value)
    {
        this.semaphore.Wait();

        try
        {
            this.cache[key] = value;

            Task
                .Run(() => Thread.Sleep(this.expirationTime))
                .ContinueWith(t =>
                {
                    this.Remove(key);
                });
        }
        finally
        {
            this.semaphore.Release();
        }
    }

    public void Remove(TKey key)
    {
        this.semaphore.Wait();

        try
        {
            this.cache.Remove(key);
        }
        finally
        {
            this.semaphore.Release();
        }
    }

    public TValue GetValueOrDefault(TKey key)
    {
        this.semaphore.Wait();

        try
        {
            if (this.cache.TryGetValue(key, out var value))
            {
                return value;
            }

            return null;
        }
        finally
        {
            this.semaphore.Release();
        }
    }
}

CodePudding user response：

Problems I see in no particular order (there might be more though):

Task.Run

Task.Run(() => Thread.Sleep(...)) is a very bad idea. The ThreadPool, which contains the threads that run Tasks has a limited amount of threads (it can grow, but it grows very slowly!!) and by calling Task.Run(() => Thread.Sleep(...)) instead of Task.Delay(...) you're wasting a ThreadPool thread for the duration of the wait. Task.Delay(...) doesn't use up a thread.

Wrong Item Expiration

Another problem is your Add method. Think about the following:

• You create an instance of TokenCache, for example via new TokenCache<int, string>(TimeSpan.FromMinutes(1))
• You call Add(10, "ten")
• After 50 seconds, you call Remove(10)
• After 5 more seconds, you call Add(10, "The Number 10");
• Because of the way you've written your Add method, you're now going to remove the value "The Number 10" after only 5 seconds

Pointless Semaphore

Also, I don't really see the point of using a SemaphoreSlim. In this scenario you could just use the lock keyword on something like private readonly object locker = new();

CodePudding user response：

Your implementation is certainly not thread-safe because the code that is running after the delay is not locked! But the entire design is questionable. Instead of starting a new Task for every Add you should store a timestamp with each dictionary value, and use a background timer that periodically removes expired entries. Then use a normal lock to make the dictionary accesses thread-safe.