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

If you want that, join the threads **before** reading the sum. — Cem, Jan 27 '21 at 16:07
That sounds not great since you'd be killing and respawning the threads every iteration, wouldn't you? — sweenish, Jan 27 '21 at 16:09
@500-InternalServerError so I have to create seperate method for condition 'sum == 300'? — BartekS, Jan 27 '21 at 16:14
Why do you expect `sum` to be 300 *before* you know that the threads have finished? — molbdnilo, Jan 27 '21 at 16:15
On a side note, you should not be locking and unlocking the `std::mutex` manually. Use a `std::lock_guard` or `std::scoped_lock` instead, eg: `{ std::lock_guard lock(m); /* read-write sum as needed */ }` — Remy Lebeau, Jan 27 '21 at 16:54
I believe the underlying issue is to assume that multithread programming is the same or similar to single-thread programming, and writing multithreaded programs using the same assumptions as single-thread programming. — PaulMcKenzie, Jan 27 '21 at 16:57
@PaulMcKenzie Assuming MT programming has the same effect as single threaded programming... why even *use* thread? Why would anyone bother learning a tool that has zero effect? — curiousguy, Jan 28 '21 at 20:49

Useless · Accepted Answer · 2021-09-08T11:53:30.693

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:

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:

abc - what you expected (and what would be guaranteed if you joined both threads before reading sum)
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)
cab - you read 0 immediately, before the two threads have even been scheduled to run
bca - you read 200, it's later incremented to 300 after you checked
etc.

every permutation is permitted, unless you make some effort to explicitly order them

score 0 · Answer 2 · answered Jan 27 '21 at 17:57

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

C++ mutex doesn't work - synchronization fails

2 Answers2