Skip to content

ConcurrentDictionary.GetOrCreate can return a value that was never stored when two threads race on the same key #130

Description

@matt-edmondson

What's wrong

The ConcurrentDictionary<TKey, TVal> overload of DictionaryExtensions.GetOrCreate is a non-atomic check-then-act:

if (dictionary.TryGetValue(key, out TVal? val))
    return val;

bool result = dictionary.TryAdd(key, defaultValue);
Debug.Assert(result);
return defaultValue;

This overload exists specifically for concurrent use, but two threads can both miss in TryGetValue. One TryAdd wins. The other returns false, which fires Debug.Assert in Debug builds and is ignored in Release, and that thread returns its own defaultValue, which isn't in the dictionary.

Failure scenario

var counters = new ConcurrentDictionary<string, List<int>>();
Parallel.For(0, 1000, i => counters.GetOrCreate("k", new List<int>()).Add(i));

Every thread that loses the race appends to an orphaned list. counters["k"] ends up with fewer than 1000 entries and nothing reports the loss. In Debug builds or test runs the Debug.Assert dialog or failure fires intermittently instead.

Suggested fix

Use the atomic API the type already provides:

return dictionary.GetOrAdd(key, defaultValue);

Then drop the Debug.Assert. A lazily constructed overload (GetOrAdd(key, _ => new TVal())) would also avoid allocating a default on every hit.

Acceptance criteria

  • A stress test like the one above gives every caller the same instance for a given key: counters["k"].Count == 1000, and no assert fires.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions