Is this ConcurrentDictionary Thread safe?

Question

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?

Perhaps what you're looking for is a cache, like `MemoryCache`. It's thread safe and handles expiration so that you don't have to check the date of entries or "manually" delete out-of-date entries. — Scott Hannen, Jul 02 '18 at 15:03

score 5 · Answer 1 · edited Oct 31 '18 at 19:39

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.

score 0 · Accepted Answer · edited Oct 31 '18 at 19:29

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.

Is this ConcurrentDictionary Thread safe?

2 Answers2