Runtime: CollectionExtensions.TryAdd doesn't provide atomic check-and-add on a ConcurrentDictionary

Created on 1 Aug 2019  路  4Comments  路  Source: dotnet/runtime

```c#
using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Threading;

namespace ConsoleApp4NETCore
{
class Program
{
static void Main(string[] args)
{
TestDicMutilThreadAdd.Test();
Console.Read();
}
}

public class TestDicMutilThreadAdd
{
    public static void Test()
    {
        IDictionary<string, string> dic = new ConcurrentDictionary<string, string>();
        //ConcurrentDictionary<string, string> dic = new ConcurrentDictionary<string, string>();

        void Action2(object _)
        {
            if (dic.TryAdd("1", "1"))
            {
                Console.WriteLine("add success");
            }
            else
            {

                Console.WriteLine("add fail");
            }
        }

        ThreadPool.QueueUserWorkItem(Action2);
        ThreadPool.QueueUserWorkItem(Action2);
        ThreadPool.QueueUserWorkItem(Action2);
    }
}

}
```
When dic is decared as IDictionary throw Unhandled Exception: Unhandled Exception: System.ArgumentException: The key already existed in the dictionary.
But When dic is decared as ConcurrentDictionary work well.

area-System.Collections

Most helpful comment

This is a design problem with the CollectionExtensions.TryAdd extension method that was added for IDictionary<> in .NET Core 2.0 in https://github.com/dotnet/corefx/pull/18091.
cc: @terrajobst, @bartonjs

When accessing TryAdd via ConcurrentDictionary, you're just calling to ConcurrentDictionary's TryAdd method.

When accessing TryAdd via IDictionary, you're actually calling this CollectionExtensions.TryAdd method, which calls IDictionary.Contains to see whether the dictionary contains the value, and then if it doesn't, calls IDictionary.Add. ConcurrentDictionary's implementation of IDictionary.Add throws an exception if the dictionary already contains the element, just as every other IDictionary.Add is supposed to. As such, whereas there's no race condition using ConcurrentDictionary.TryAdd, there is a race condition using CollectionExtensions.TryAdd.

I don't know that there's a good solution here, other than maybe considering using default interface methods, adding a TryAdd to IDictionary, and then letting ConcurrentDictionary's TryAdd override it.

All 4 comments

This is a design problem with the CollectionExtensions.TryAdd extension method that was added for IDictionary<> in .NET Core 2.0 in https://github.com/dotnet/corefx/pull/18091.
cc: @terrajobst, @bartonjs

When accessing TryAdd via ConcurrentDictionary, you're just calling to ConcurrentDictionary's TryAdd method.

When accessing TryAdd via IDictionary, you're actually calling this CollectionExtensions.TryAdd method, which calls IDictionary.Contains to see whether the dictionary contains the value, and then if it doesn't, calls IDictionary.Add. ConcurrentDictionary's implementation of IDictionary.Add throws an exception if the dictionary already contains the element, just as every other IDictionary.Add is supposed to. As such, whereas there's no race condition using ConcurrentDictionary.TryAdd, there is a race condition using CollectionExtensions.TryAdd.

I don't know that there's a good solution here, other than maybe considering using default interface methods, adding a TryAdd to IDictionary, and then letting ConcurrentDictionary's TryAdd override it.

Couldn't we just do an as-cast and defer to the specialized member in the implementation? A la Enumerable.Count() deferring to ICollection.get_Count()

@stephentoub Should not the priority of the instance method be higher than the extension method?

Couldn't we just do an as-cast

There's a layering problem. But even if we worked around that, it would still only handle this one special case.

Should not the priority of the instance method be higher than the extension method?

@newpost, there is no IDictionary<>.TryAdd instance method.

Was this page helpful?
0 / 5 - 0 ratings