Implementing ConcurrentDictionary

Question

I'm trying to create my own Cache implementation for an API. It is the first time I work with ConcurrentDictionary and I do not know if I am using it correctly. In a test, something has thrown error and so far I have not been able to reproduce it again. Maybe some concurrency professional / ConcurrentDictionary can look at the code and find what may be wrong. Thank you!

private static readonly ConcurrentDictionary<string, ThrottleInfo> CacheList = new ConcurrentDictionary<string, ThrottleInfo>();

public override void OnActionExecuting(HttpActionContext actionExecutingContext)
{
    if (CacheList.TryGetValue(userIdentifier, out var throttleInfo))
    {
        if (DateTime.Now >= throttleInfo.ExpiresOn)
        {
            if (CacheList.TryRemove(userIdentifier, out _))
            {
                //TODO:
            }
        }
        else
        {
            if (throttleInfo.RequestCount >= defaultMaxRequest)
            {
                actionExecutingContext.Response = ResponseMessageExtension.TooManyRequestHttpResponseMessage();
            }
            else
            {
                throttleInfo.Increment();
            }
        }

    }
    else
    {
        if (CacheList.TryAdd(userIdentifier, new ThrottleInfo(Seconds)))
        {
            //TODO:
        }
    }
}

public class ThrottleInfo
{
    private int _requestCount;

    public int RequestCount => _requestCount;

    public ThrottleInfo(int addSeconds)
    {
        Interlocked.Increment(ref _requestCount);
        ExpiresOn = ExpiresOn.AddSeconds(addSeconds);
    }

    public void Increment()
    {
        // this is about as thread safe as you can get.
        // From MSDN: Increments a specified variable and stores the result, as an atomic operation.
        Interlocked.Increment(ref _requestCount);

        // you can return the result of Increment if you want the new value,
        //but DO NOT set the counter to the result :[i.e. counter = Interlocked.Increment(ref counter);] This will break the atomicity.
    }

    public DateTime ExpiresOn { get; } = DateTime.Now;
}

That's the problem, when it happened, I did not take notes and now I can not reproduce it. I'm tried, but I can not. I assume it's a concurrency problem. At some point 2 or more threads want to do something with the same object and break the code. Being few lines of code, perhaps some expert, can visualize and find some error. It's the first time I use ConcurrentDictionary — avechuche, Nov 21 '18 at 17:28
The `ConcurrentDictionary` class is only suitable for very trivial caching scenarios. For anything more advanced (like expiration policy), there are specialized classes available. Like the [`System.Runtime.Caching.MemoryCache`](https://docs.microsoft.com/en-us/dotnet/api/system.runtime.caching.memorycache) (with `string` keys), and the newer [`Microsoft.Extensions.Caching.Memory.MemoryCache`](https://docs.microsoft.com/en-us/dotnet/api/microsoft.extensions.caching.memory.memorycache) (with `object` keys). The later offers more prioritization options. I strongly recommend to use one of these. — Theodor Zoulias, Apr 22 '20 at 22:24

score 0 · Answer 1 · answered Apr 22 '20 at 18:48

If I understand what you are trying to do if the ExpiresOn has passed remove the entry else update it or add if not exists. You certainly can take advantage of the AddOrUpdateMethod to simplify some of your code. Take a look here for some good examples: https://learn.microsoft.com/en-us/dotnet/standard/collections/thread-safe/how-to-add-and-remove-items Hope this helps.

score 0 · Answer 2 · answered Apr 22 '20 at 23:03

The ConcurrentDictionary is sufficient as a thread-safe container only in cases where (1) the whole state that needs protection is its internal state (the keys and values it contains), and only if (2) this state can be mutated atomically using the specialized API it offers (GetOrAdd, AddOrUpdate). In your case the second requirement is not met, because you need to remove keys conditionally depending on the state of their value, and this scenario is not supported by the ConcurrentDictionary class.

So your current cache implementation is not thread safe. The fact that throws exceptions sporadically is a coincidence. It would still be non-thread-safe if it was totally throw-proof, because it would not be totally error-proof, meaning that it could occasionally (or permanently) transition to a state incompatible with its specifications (returning expired values for example).

Regarding the ThrottleInfo class, it suffers from a visibility bug that could remain unobserved if you tested the class extensively in one machine, and then suddenly emerge when you deployed your app in another machine with a different CPU architecture. The non-volatile private int _requestCount field is exposed through the public property RequestCount, so there is no guarantee (based on the C# specification) that all threads will see its most recent value. You can read this article by Igor Ostrovsky about the peculiarities of the memory models, which may convince you (like me) that employing lock-free techniques (using the Interlocked class in this case) with multithreaded code is more trouble than it's worth. If you read it and like it, there is also a part 2 of this article.

Implementing ConcurrentDictionary

2 Answers2