CODEWITHSUNDEEP
c#
CJM
CJM

Reputation: 259

Is this ConcurrentDictionary Thread safe?

Is this ConcurrentDictionary ThreadSafe?

    private static ConcurrentDictionary<string, DateTime> dictionary= new ConcurrentDictionary<string, DateTime>();

    public bool TextInputRecently(string text)
    {
        dictionary = new ConcurrentDictionary<string, DateTime>( dictionary.Where(pair => pair.Value.Ticks >= DateTime.Now.AddMinutes(-5).Ticks)
                             .ToDictionary(pair => pair.Key,
                                           pair => pair.Value));

        if (dictionary.ContainsKey(text))
            return true;

        dictionary.TryAdd(text, DateTime.Now);
        return false;
    }

I am thinking that it is not as the dictionary could be recreated whilst another thread is checking if the key exists.

Would i be better off looping through the dictionary removing the out of date values?

Upvotes: 1

Views: 1292

Answers (2)

SLaks
SLaks

Reputation: 887215

No; the dictionary could change between ContainsKey() & TryAdd().

You should never call two methods in a row on ConcurrentDictionary, unless you're sure you don't care if it changes between them.
Similarly, you can't loop through the dictionary, since it might change during the loop.

Instead, you should use its more-complex methods (like TryAdd(), which will check and add in a single atomic operation.

Also, as you suggested, the entire dictionary might be replaced.

Upvotes: 5

CJM
CJM

Reputation: 259

Thanks for the answer SLaks.

I have followed Scott Hannen's suggestion and replaced it with a MemoryCache. Here is the code in case someone wants it, followed Locking pattern for proper use of .NET MemoryCache.

private static MemoryCache cache;
    static readonly object cacheLock = new object();
    private int expiry_timeout = 5;

    public bool TextInputRecently(string text)
    {
        //Returns null if the string does not exist, prevents a race condition where the cache invalidates between the contains check and the retrieval.
        var cachedString = MemoryCache.Default.Get(text, null) as string;

        if (cachedString != null)
            return true;

        lock (cacheLock)
        {
            //Check to see if anyone wrote to the cache while we where waiting our turn to write the new value.
            cachedString = MemoryCache.Default.Get(text, null) as string;

            if (cachedString != null)
                return true;

            //The value still did not exist so we now write it in to the cache.                
            CacheItemPolicy cip = new CacheItemPolicy()
            {
                AbsoluteExpiration = new DateTimeOffset(DateTime.Now.AddSeconds(expiry_timeout))
            };
            MemoryCache.Default.Set(text, "", cip);
            return false;
        }
    }

There may be a better solution but this works for my needs.

Upvotes: 0

Related Questions