Why is this threaded counter wrong, and how to fix it?
counter is already a std::atomic<int>, yet after several std::threads run worker() and are joined, the final counter is too small.
std::atomic<int> counter{0};
void worker() { for (int i = 0; i < 100000; ++i) counter = counter + 1; }
// launch worker() on several std::threads, then join them allFind and fix the bug.
counter = counter + 1 is an atomic load and a separate atomic store, so two threads can read the same value and an increment is lost. No UB, only lost updates. Fix: ++counter or counter.fetch_add(1), one atomic step.
- Believing every expression on a
std::atomicis atomic as a whole, when only each single operation on it is - Calling lost updates on an atomic undefined behavior, when the program is well defined and only the logic is wrong
- Blaming the memory order, when the default
seq_cstis already the strongest and the bug is the split load and store
- Why is
std::memory_order_relaxedenough forfetch_addon a counter read only afterjoin? - When does an update need a
compare_exchange_weakloop instead offetch_add?
With std::atomic each single operation is atomic, not the whole expression. counter = counter + 1 reads the value (load), adds one in an ordinary temporary and writes the result (store). Between the read and the write another thread can store its own value, and that increment is overwritten. There is no data race and the behavior is defined, but the total comes out too small.
std::atomic<int> counter{0};
void worker() {
for (int i = 0; i < 100000; ++i) {
counter.fetch_add(1); // or ++counter, or counter += 1
}
}fetch_add performs the read, the addition and the write as one indivisible operation, so no increment is lost. A mutex would fix it too, but for a single counter it is overkill.
