GetOrAdd is not thread safe which is counter intuitive

[yepeekai]

I expect the concurrentDictionary to be thread safe for all of its write operation. The documentation of the GetOrAdd method doesn't specify that it is not thread safe which is a problem. I really think that there should be a GetOrAdd overload with a bool specifying that we want thread safety. I also think that the current GetOrAdd should be renamed GetOrAddUnsafe to make it clear that it doesn't respect the expected behavior of the class

---

Document Details

⚠ Do not edit this section. It is required for docs.microsoft.com ➟ GitHub issue linking.

• ID: 806c6485-9396-e6bf-2f8e-f27866115626
• Version Independent ID: 3e499bf3-fa82-0b82-5d85-84a0b3fe0515
• Content: ConcurrentDictionary<TKey,TValue> Class (System.Collections.Concurrent)
• Content Source: [xml/System.Collections.Concurrent/ConcurrentDictionary2.xml](https://github.com/dotnet/dotnet-api-docs/blob/master/xml/System.Collections.Concurrent/ConcurrentDictionary2.xml)
• Product: dotnet-api
• GitHub Login: @mairaw
• Microsoft Alias: mairaw

[pkulikov]

@yepeekai what makes you think that GetOrAdd is not thread-safe?

[yepeekai]

The valueFactory call is not thread safe. A good documentation on the issue by Andrew Lock: https://andrewlock.net/making-getoradd-on-concurrentdictionary-thread-safe-using-lazy/
As for the documentation of the method, I mean XML documentation (intellisense in visual studio)

[yepeekai]

Well, I think I meant atomic... It is clear in my head but maybe not that clear how to write it :S
From the documentation:

All these operations are atomic and are thread-safe with regards to all other operations on the ConcurrentDictionary<TKey,TValue> class. The only exceptions are the methods that accept a delegate, that is, AddOrUpdate and GetOrAdd.

[yepeekai]

In my opinion, if I pass a delegate to create the value if it doesn't exist, I expect the delegate to be called only once (since I use a specific dictionary that support thread safety and has atomic operation)

[pkulikov]

With addition to the docs quoted by @yepeekai:

However, delegates for these methods are called outside the locks to avoid the problems that can arise from executing unknown code under a lock. Therefore, the code executed by these delegates is not subject to the atomicity of the operation.

That is on the main page of the ConcurrentDictionary doc. I think that should be repeated in the docs of the specific overloads as well.

[pkulikov]

@yepeekai

In my opinion, if I pass a delegate to create the value if it doesn't exist, I expect the delegate to be called only once (since I use a specific dictionary that support thread safety and has atomic operation)

I can understand why it's not atomic in case of the methods with delegates:

• What if the delegate takes too much time to run? Then all other requests to the dictionary wait.
• What if the delegate tries to update the same dictionary? Deadlock?

Anyway, implementation cannot be changed anymore. To my concern, the good way for now is to add remarks to the docs of the overload methods.

[yepeekai]

I agree for the documentation.
I understand the reason why it is excluded of the lock.

But I would say that writing bulletproof code using lock is hard/scary and that a default implementation to help the developer do this in an atomic way would be a great addition in a different overload of the method. The Lazy used in Andrew Lock example could be used and be abstracted in the implementation and then call value at the end outside of the lock... (this would eliminate the deadlock inside the lock. The behavior would be the same except that the time taken by the valuefactory would be after inserting in the dictionary instead of before) There would still be possible problems if the valuefactory throws an exception though.

I understand this is outside the scope of documentation.

Thanks for your time Pkulikov

[svick]

@yepeekai

But I would say that writing bulletproof code using lock is hard/scary and that a default implementation to help the developer do this in an atomic way would be a great addition in a different overload of the method.

If you want to propose a new .Net API, the right place for that is the corefx repo. Ideally, you should follow the API review process.

[rpetrusha]

@pkulikov , would you be interested in adding some variation of the text on non-thread safe, non-atomic operations to the AddOrUpdate and GetOrAdd methods?

[pkulikov]

@rpetrusha yes, please assign this issue to me. For sprint 142, though.