ConcurrentDictionary GetOrAdd value factory is not thread safe

[nrusitska]

When using GetOrAdd with a Value Factory, the execution of the factory is not thread safe and may be called multiple times.

Being called from multiple threads, this method first tries to find a present value (threadsafe),
then executes the value factory (unsafe) and finally inserts the value (safe again).
If the execution of the value factory takes longer, it's quite likely that is will be called each time.
just by the following TryAddInternal all the duplicate values are dismissed.

https://github.com/dotnet/corefx/blob/655af2fab48c1d0fdae352786d8371d0d13a81a7/src/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs#L987-L990

I suppose, a lock must be acquired before executing the value function.
(BTW this problem also applies to GetOrAdd with the value parameter, but is way less likely to happen in a real scenario)

Best regards!
Niko

[stephentoub]

It is thread-safe, and yes, the value factory may be called multiple times. It's required that the delegate provided be able to be invoked concurrently with itself; if that's not the case, that's a bug in the usage rather than in the implementation of the dictionary. We explicitly do not want to take a lock here.

[benaadams]

The Value Factory is provided by the caller. Whether or not that is thread-safe (or needs to be) is up to the caller and controlled by them.