What's wrong
PR #131 (commit 01b92d0) made the ConcurrentDictionary<TKey,TVal>.GetOrCreate(key, defaultValue) overload atomic by delegating to GetOrAdd (Extensions/DictionaryExtensions.cs:65-99). The one-argument form has no ConcurrentDictionary overload, though. concurrentDict.GetOrCreate(key) binds to the IDictionary overload at Extensions/DictionaryExtensions.cs:20:
public static TVal GetOrCreate<TKey, TVal>(this IDictionary<TKey, TVal> dictionary, TKey key) ... => GetOrCreate(dictionary, key, new TVal());
Inside that method dictionary has the static type IDictionary, so the call goes to the non-atomic IDictionary 3-argument overload (:31-58):
if (dictionary.TryGetValue(key, out TVal? val)) { return val; }
dictionary.Add(key, defaultValue); // IDictionary.Add on ConcurrentDictionary throws if the key now existsFailure scenario
- Thread A calls
d.GetOrCreate(1) on an empty ConcurrentDictionary<int, List<int>>, and TryGetValue misses.
- Thread B does the same, and its
TryGetValue also misses.
- Thread A runs
Add(1, listA) and succeeds.
- Thread B runs
Add(1, listB), which throws ArgumentException ("An item with the same key has already been added").
- Expected: both callers get the stored instance, as the
(key, defaultValue) overload now guarantees.
- Actual: the losing caller crashes.
How verified
Built the library for net10.0 and ran a console program. In each of 2000 iterations, 8 tasks released through a Barrier all call d.GetOrCreate(1) on a fresh ConcurrentDictionary<int, List<int>>. Output:
GetOrCreate(key) on ConcurrentDictionary: 1466 ArgumentExceptions, 0 iterations with differing instances
The existing test GetOrCreateConcurrentDictionaryShouldReturnSameInstanceToParallelCallers only covers the two-argument form, so it does not catch this.
Suggested fix / acceptance criteria
- Add a
ConcurrentDictionary overload for the one-argument form, for example public static TVal GetOrCreate<TKey,TVal>(this ConcurrentDictionary<TKey,TVal> dictionary, TKey key) where TKey : notnull where TVal : new() => dictionary.GetOrAdd(key, _ => new TVal());, with the same null checks. Alternatively, have the IDictionary path detect ConcurrentDictionary and call GetOrAdd.
- Add a test that runs
Parallel.For over dictionary.GetOrCreate("key1").Add(i) on a ConcurrentDictionary<string, List<int>>. It should not throw and should end with 1000 items in one list.
What's wrong
PR #131 (commit 01b92d0) made the
ConcurrentDictionary<TKey,TVal>.GetOrCreate(key, defaultValue)overload atomic by delegating toGetOrAdd(Extensions/DictionaryExtensions.cs:65-99). The one-argument form has noConcurrentDictionaryoverload, though.concurrentDict.GetOrCreate(key)binds to theIDictionaryoverload atExtensions/DictionaryExtensions.cs:20:Inside that method
dictionaryhas the static typeIDictionary, so the call goes to the non-atomicIDictionary3-argument overload (:31-58):Failure scenario
d.GetOrCreate(1)on an emptyConcurrentDictionary<int, List<int>>, andTryGetValuemisses.TryGetValuealso misses.Add(1, listA)and succeeds.Add(1, listB), which throwsArgumentException("An item with the same key has already been added").(key, defaultValue)overload now guarantees.How verified
Built the library for net10.0 and ran a console program. In each of 2000 iterations, 8 tasks released through a
Barrierall calld.GetOrCreate(1)on a freshConcurrentDictionary<int, List<int>>. Output:The existing test
GetOrCreateConcurrentDictionaryShouldReturnSameInstanceToParallelCallersonly covers the two-argument form, so it does not catch this.Suggested fix / acceptance criteria
ConcurrentDictionaryoverload for the one-argument form, for examplepublic static TVal GetOrCreate<TKey,TVal>(this ConcurrentDictionary<TKey,TVal> dictionary, TKey key) where TKey : notnull where TVal : new() => dictionary.GetOrAdd(key, _ => new TVal());, with the same null checks. Alternatively, have theIDictionarypath detectConcurrentDictionaryand callGetOrAdd.Parallel.Foroverdictionary.GetOrCreate("key1").Add(i)on aConcurrentDictionary<string, List<int>>. It should not throw and should end with 1000 items in one list.