thread safety - C++ mutex doesn't work - synchronization fails

Question

I would like to apply as simple mutex as possible.

#include <iostream>
#include <thread>
#include <vector>
#include <functional>
#include <algorithm>
#include <mutex>
using namespace std;

int sum;
static mutex m;

void addValue(int value)
{
    m.lock();
    sum += value;
    m.unlock();
}

int main()
{

    int counter1 = 0;
    int counter2 = 0;
    for (int i = 0; i < 100; i++)
    {
        thread t1(addValue, 100);
        thread t2(addValue, 200);

        if (sum == 300)
        {
            counter1++;
        }
        else
        {
            counter2++;
        }
        sum = 0;
        t1.join();
        t2.join();
    }
    cout << counter1 << endl;
    cout << counter2 << endl;
}

Unfortunately above mentioned code doesn't work as expected. I expect that:

a) sum is always equal to 300
b) counter1 is always 100
c) counter2 is always 0

What is wrong?

EDIT:

When I debug the sum variable in the else condition, I see values like: 200, 400, 100, and even 0 (I assume that addition didn't even happen).

question from:https://stackoverflow.com/questions/65922844/c-mutex-doesnt-work-synchronization-fails

Answer 1

C++ mutex doesn't work - synchronization fails

Why does everyone learning this stuff for the first time assume the tried-and-tested synchronization primitives that work for everyone else are broken, and not their assumptions?

The mutex is fine. Your mental model is broken. This should be your starting assumption.

I expect that:

1. sum is always equal to 300

That would be the case if you joined both threads before checking the value. But you haven't done that, so you're doing an entirely un-sychronized read of sum while two other threads are possibly mutating it. This is a data race. A mutex doesn't protect your data unless you always use the mutex when accessing the data.

Let's say we make the minimal change so sum is always protected:

    thread t1(addValue, 100); // a
    thread t2(addValue, 200); // b

    m.lock();
    if (sum == 300)           // c
    {
        counter1++;
    }
    else
    {
        counter2++;
    }
    sum = 0;
    m.unlock();

now some of the available orderings are:

1. abc - what you expected (and what would be guaranteed if you joined both threads before reading sum)
2. acb - you read 100 at line c, increment counter2, and the second thread increments sum to 300 after you read it (but you never see this)
3. cab - you read 0 immediately, before the two threads have even been scheduled to run
4. bca - you read 200, it's later incremented to 300 after you checked
5. etc. - every permutation is permitted, unless you make some effort to explicitly order them