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).

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

Answer 2

It is working as intended, the problem is that you didn't expected that "time" will not be the same for all 3 threads and you dismist the obvious thing that one thread starts before the other, this clearly adds an avantage, even more if it only needs to do is loop 100 times a increment.

#include <iostream>
#include <thread>
#include <mutex>

bool keep_alive;

void add_value_mutex(std::mutex * mx, int * trg, int value) {
    while (keep_alive){
        mx->lock();
        (*trg) += value;
        mx->unlock();
    }
}

int main(){

    std::thread thread_1;
    std::thread thread_2;

    int count_targ = 2000;
    int * counter_1 = new int(0);
    int * counter_2 = new int(0);

    /* --- */

    std::mutex mx_1;
    std::mutex mx_2;

    keep_alive = true;
    thread_1 = std::thread(add_value_mutex, &mx_1, counter_1, 1);
    thread_2 = std::thread(add_value_mutex, &mx_2, counter_2, 1);

    while(1){

        if (mx_1.try_lock()){
            if (count_targ <= * counter_1){
                mx_1.unlock();
                break;
            }
            mx_1.unlock();
        }

        if (mx_2.try_lock()){
            if (count_targ <= * counter_2){
                mx_2.unlock();
                break;
            }
            mx_2.unlock();
        }

    }
    
    keep_alive = false;
    thread_1.join();
    thread_2.join();
        
    std::cout << "Thread 1 (independent mutex) -> " << * counter_1 << std::endl;
    std::cout << "Thread 2 (independent mutex) -> " << * counter_2 << std::endl;

    /* --- */

    keep_alive = true;
    (*counter_1) = 0;
    (*counter_2) = 0;

    std::mutex mx_s;
    thread_1 = std::thread(add_value_mutex, &mx_s, counter_1, 1);
    thread_2 = std::thread(add_value_mutex, &mx_s, counter_2, 1);

    while(1){

        if (mx_s.try_lock()){
            if (count_targ <= * counter_1 || count_targ <= * counter_2){
                mx_s.unlock();
                break;
            }
            mx_s.unlock();
        }
    }

    std::cout << "Thread 1 (shared mutex) -> " << * counter_1 << std::endl;
    std::cout << "Thread 2 (shared mutex) -> " << * counter_2 << std::endl;

    keep_alive = false;
    thread_1.join();
    thread_2.join();


    delete counter_1;
    delete counter_2;

    return 0;
}

If you want another example of mine that measures the time a thread is waiting check this one