1

I have a concurrent dictionary which is a class variable:

private readonly ConcurrentDictionary<Id, int> _tracker=
            new ConcurrentDictionary<Id, int>(Id.EqualityComparer);

I'm incrementing it in a method as follows:

_tracker[id]++;

Is this safe to do so if the _tracker will be accesed concurrently? Or should I be using the

_tracker.AddOrUpdate(id, 1, (id, count) => count+ 1);
Riyafa Abdul Hameed
  • 7,417
  • 6
  • 40
  • 55
  • 3
    No, `_tracker[id]++` is not thread-safe – canton7 Aug 22 '23 at 14:10
  • 1
    can you point to any documentation on this? – Riyafa Abdul Hameed Aug 22 '23 at 14:24
  • 4
    https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/language-specification/expressions#12815-postfix-increment-and-decrement-operators. `++` on a property means a property read + modify + write. It's fairly obvious that ConcurrentDictionary cannot support this in a thread-safe way, because the read and write are separate operations with no relationship between them – canton7 Aug 22 '23 at 14:41

2 Answers2

2

_tracker[id]++ is not an atomic operation. You are calling the getter of the indexer method to retrieve the value in a thread safe way. Then you call the setter of the indexer in a thread safe way. Whatever happens in between is not thread safe. So another thread could modify that value before you set it. So better use AddOrUpdate which is thread safe (in your case, read Is AddOrUpdate thread safe in ConcurrentDictionary?).

So your code is basically:

int i = _tracker[id]; // thread safe
// not thread safe, the value in the dictionary could change here
_tracker[id] = i + 1; // thread safe
Tim Schmelter
  • 450,073
  • 74
  • 686
  • 939
  • 1
    Not a fan of how the term "thread safe" is used in this answer. People mean different things with this term, and by some definitions if a piece of code doesn't throw exceptions or cause data corruption is thread-safe. Better use the term atomic, which is more specific. For an example see [this answer](https://stackoverflow.com/questions/4628243/is-the-operator-thread-safe/4628660#4628660 "Is the ++ operator thread safe?") by Eric Lippert. – Theodor Zoulias Aug 22 '23 at 15:10
  • 1
    @TheodorZoulias: what exactly are you referring to? Because the term "thread safe" in the comments cannot be replaced with "atomic operation". Something does not need to be atomic to be thread safe(you could use a lock) and a one-liner is normally not atomic . – Tim Schmelter Aug 22 '23 at 15:17
  • 1
    Personally I wouldn't use the term "thread safe" at all. The OP hasn't used it in the question (at least not verbatim). They've asked if the `_tracker[id]++;` is safe, not thread-safe. I would focus at explaining them what the problem of the code is, what undesirable behavior might emerge by this code, not teaching them an ambiguous term with low communication value. That's my 2 cents. – Theodor Zoulias Aug 22 '23 at 15:20
  • 1
    @TheodorZoulias: OP asked if it "is this safe to do so if the _tracker will be accesed concurrently", so of course it means thread-safety. Why else he should use a `ConcurrentDictionary` and emphasize "concurrently" which means from multiple threads at the same time? The problem with his code is that it's not safe to use in a multithreading environment :D – Tim Schmelter Aug 22 '23 at 15:24
  • 1
    "Thread safe" means wildly different things to different people, that's exactly the problem, so saying, "so of course it means thread-safety" *isn't true* because neither of you even necessarily mean the same thing by that term. "Because the term 'thread safe' in the comments cannot be replaced with 'atomic operation'" Actually it can. You're using the term "thread safe" specifically to mean atomic in your post. You're claiming the read is atomic, the write is atomic, but the two operations in sequence are not atomic. That's *precisely* what you're trying to convey. – Servy Aug 22 '23 at 16:06
  • Btw another term that could be used in this answer is [race condition](https://en.wikipedia.org/wiki/Race_condition#Example). When a race condition occurs, the outcome of the operation could be wrong. – Theodor Zoulias Aug 23 '23 at 02:36
-2

_tracker[id]++ may be inconsistent in multi-thread environment since it gets the value first and then updates it.

It's safe to use GetOrUpdate or Interlocked.Increment

Interlocked.Increment(ref _tracker[id]);
Jeeva Subburaj
  • 1,881
  • 2
  • 18
  • 26
  • 2
    `ref _tracker[id]` only works if the property returns a `ref`, which `ConcurrentDictionary`'s does not (and rightly so, as that would mean you could mutate the property outside of a lock). So your code will not compile – canton7 Aug 22 '23 at 14:52