IDictionary - ConcurrentDictionary TryAdd throws Exception

[Flatscha]

Description

If 2 processes call TryAdd on the same dictionary TryAdd will throw the Exception on the second one

Exception Info: System.ArgumentException: The key already existed in the dictionary.
   at System.Collections.Concurrent.ConcurrentDictionary`2.System.Collections.Generic.IDictionary<TKey,TValue>.Add(TKey key, TValue value)
   at System.Collections.Generic.CollectionExtensions.TryAdd[TKey,TValue](IDictionary`2 dictionary, TKey key, TValue value)

Reproduction Steps

Create a dictionary
IDictionary<string, object> Dict = new ConcurrentDictionary<string, object>();
Start two tasks that call TryAdd onto that dictionary at the same time

Expected behavior

TryAdd should not throw any Exceptions besides ArgumentNullException

Actual behavior

TryAdd throws System.ArgumentException: The key already existed in the dictionary.

Regression?

No response

Known Workarounds

No response

Configuration

• Which version of .NET is the code running on?
  .NET 7.0
• What OS and version, and what distro if applicable?
  Windows 10 Enterprise / Windows 10 IoT / Windows Server 2016 Standard
• What is the architecture (x64, x86, ARM, ARM64)?
  x64
• Do you know whether it is specific to that configuration?
  I don't think so
• If you're using Blazor, which web browser(s) do you see this issue in?
  newest release of chrome

Other information

There should be a catch which returns false in the TryAdd method

[ghost]

Tagging subscribers to this area: @dotnet/area-system-collections
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

If 2 processes call TryAdd on the same dictionary TryAdd will throw the Exception on the second one

Exception Info: System.ArgumentException: The key already existed in the dictionary.
   at System.Collections.Concurrent.ConcurrentDictionary`2.System.Collections.Generic.IDictionary<TKey,TValue>.Add(TKey key, TValue value)
   at System.Collections.Generic.CollectionExtensions.TryAdd[TKey,TValue](IDictionary`2 dictionary, TKey key, TValue value)

Reproduction Steps

Create a dictionary
IDictionary<string, object> Dict = new ConcurrentDictionary<string, object>();
Start two tasks that call TryAdd onto that dictionary at the same time

Expected behavior

TryAdd should not throw any Exceptions besides ArgumentNullException

Actual behavior

TryAdd throws System.ArgumentException: The key already existed in the dictionary.

Regression?

No response

Known Workarounds

No response

Configuration

• Which version of .NET is the code running on?
  .NET 7.0
• What OS and version, and what distro if applicable?
  Windows 10 Enterprise / Windows 10 IoT / Windows Server 2016 Standard
• What is the architecture (x64, x86, ARM, ARM64)?
  x64
• Do you know whether it is specific to that configuration?
  I don't think so
• If you're using Blazor, which web browser(s) do you see this issue in?
  newest release of chrome

Other information

There should be a catch which returns false in the TryAdd method

Author:	Flatscha
Assignees:	-
Labels:	area-System.Collections, untriaged
Milestone:	-

[Tornhoof]

That's because TryAdd will be resolved as the extension method on IDictionary and that one basically calls ContainsKey and then Add.
To use the ConcurrentDictionary one, don't cast your dictionary to IDictionary.

[DaZombieKiller]

This makes me wonder if CollectionExtensions.TryAdd could special-case ConcurrentDictionary here, similar to how LINQ special-cases List<T> and others for perf reasons.

[LeaFrock]

This inconsistent behavior between extension methods and self-owned methods is indeed confusing. A special-case is also not very satisfying... What I'm curious is why IDictionary.Add of ConcurrentDictionary need to throw an exception if TryAdd is false?

[stephentoub]

What I'm curious is why IDictionary.Add of ConcurrentDictionary need to throw an exception if TryAdd is false?

Because that's the documented behavior of IDictionary<TKey, TValue>.Add and how it communicates that the key already exists and that the supplied value is being dropped on the floor.

[LeaFrock]

Oh, thanks. I see the doc.

Therefore, this problem is just by design, which increases a little bit learning cost for developers 😿.

This drives me turning to support a special case for ConcurrentDictionary as CollectionExtensions.TryAdd doesn't mention it will throw an exception, and it's also a convention with Try-*.

[huoyaoyuan]

Also, a layering or implementation-wise issue: CollectionExtensions lives in the same assembly of base IDictionary interface definition (the CoreLib). ConcurrentDictionary is in the concurrent collection assembly, which is built on top of the base assembly.

This is a case where default interface methods can perform better than extension methods.

[rmannibucau]

up, any way default impls are supported in default extensions? this is really bothering ina big codebase where the actual impl type cannot be propagated for good reasons

[rmannibucau]

@huoyaoyuan at some point the question is what is the worse: breaking code and API (current state) or a negligible overhead.

There are strictly NO internal access between assemblies.

About the link i was thinking a build hack to enable InternalsVisibleTo which would be not neat for core devs but very valuable for end users.

[colejohnson66]

Even if they are in different assemblies it should be possible, worse case with an internal (copy on write pattern to avoid locks) Dictionary<Predicate, TryAddDelegate> the concurrent dictionary can register statically (when class is loaded) in the extension, this way it will behave as expected even if modules are not the same and this dictionary can be opened for concurrent assembly but not end users to enable to break it when a proper solution is found. So clearly there are multiple solution without even reflection and with low enough cost - dictionary option can even just be a dedicated static attribute if concurrent dictionary is the only case, overhead will be pretty much 0 and it will be functional for end users.

Nothing is never impossible in dev ;)

A static dictionary of type->delegate would itself need to be a ConcurrentDictionary to ensure safety in registration. But then you're back to square one because the current runtime library setup doesn't allow System.Collections.dll to use anything in System.Collections.Concurrent.dll.

Sure, it might be possible with TypeForwarded[From|To] attributes and some clever work, but that's a relatively large task with relatively little return. As shown, TryAdd is nothing more than a helper for !HasKey > Add. If you need concurrency checks, you can write your own extension:

public static class MyCollectionExtensions
{
    public static bool SafeTryAdd<TKey, TValue>(this IDictionary<TKey, TValue> d, TKey key, TValue value)
    {
        if (d is ConcurrentDictionary<TKey, TValue> d2)
            return d2.TryAdd(key, value);

        return d.TryAdd(key, value); // fallback to runtime impl
   }
}

Combine that with the banned API analyzer, and you can enforce this usage in your own codebase.

[rmannibucau]

A static dictionary of type->delegate would itself need to be a ConcurrentDictionary to ensure safety in registration.

Not really since it is internal the usage can be guaranteed (can't be a Dictionary, ack on this but still be can an array or just dedicated handlers to impl depending the number of impl to support - if only concurrent one it is 1 and no need of associative array at all).

That said there are other way to impl that, maybe using an internal (+ visible to runtime assemblies) interface or alike.
Otherwise goal is to enable to couple both assemblies to make it right by default without forcing end user to redevelop the provided extensions.

Sure, it might be possible with TypeForwarded[From|To] attributes and some clever work, but that's a relatively large task with relatively little return. As shown, TryAdd is nothing more than a helper for !HasKey > Add. If you need concurrency checks, you can write your own extension

Don't think ROI is low, why does TryAdd not use TryAdd (from an user standpoint it is literally that). Concurrent dictionaries are more and more common and this API is very misleading.
Indeed you can also bypass existing API and do your own but what's the point?

[huoyaoyuan]

why does TryAdd not use TryAdd

A generic entry can never use specialized one without some sort of polymorphism. The real solution of the problem can only be polymorphism.

Concurrent dictionaries are more and more common and this API is very misleading.

Using concurrent dictionary as IDictionary is not common. There are many more algorithms assuming single thread can break. That's why I was suggesting to catch all the casts. User should review and suppress cases which are determined to be safe.

Don't think ROI is low

It will never be considered to break all the rules to benefit only a single method, which shouldn't be used in best pattern.

[rmannibucau]

The real solution of the problem can only be polymorphism.

agree but you said it will not happen anytime soon so we need something else

Using concurrent dictionary as IDictionary is not common. There are many more algorithms assuming single thread can break. That's why I was suggesting to catch all the casts. User should review and suppress cases which are determined to be safe.

Yes and not, when you assume you own all the code and all the methods you call, maybe, but as soon as you are using libs it depends more and shouldn't be assumed IMHO

It will never be considered to break all the rules to benefit only a single method, which shouldn't be used in best pattern.

s/single/extension methods/

also if it shouldn't be used it shouldn't be there or be obsolete probably

once again c# has interfaces and inheritances but extensions methods don't support it - until there it is fair, but there is no way to identify it from a code perspective until you own all the code of all you bundle which is almost never the case or is not practicable (an analyzer analyzing all the transitive deps for ex)
so it needs a proper short term solution until extension polymorphism is there IMHO, it creates real prod issues almost impossible to detect at build time