0

I have the following code:

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

int shared_var {0};
std::mutex shared_mutex;

void task_1()
{
    while (true)
    {
        shared_mutex.lock();
        const auto temp = shared_var;
        std::this_thread::sleep_for(std::chrono::seconds(1));
        
        if(temp == shared_var)
        {
            //do something
        }
        else
        {
            const auto timenow = std::chrono::system_clock::to_time_t(std::chrono::system_clock::now());
            std::cout << ctime(&timenow) << ": Data race at task_1: shared resource corrupt \n";
            std::cout << "Actual value: " << shared_var << "Expected value: " << temp << "\n"; 
        }
        shared_mutex.unlock();
    }
}


void task_2()
{
    while (true)
    {
        std::this_thread::sleep_for(std::chrono::seconds(2));
        ++shared_var;
    
    }
}

int main()
{
    auto task_1_thread = std::thread(task_1);
    auto task_2_thread = std::thread(task_2);
 
    task_1_thread.join();
    task_2_thread.join();
    return 0;
}

shared_var is protected in task_1 but not protected in task_2 What is expected: I was expecting else branch is not entered in task_1 as the shared resource is locked. What actually happens: Running this code will enter else branch in task_1.

Expected outcome is obtained when replace shared_mutex.lock(); with std::lock_guard<std::mutex> lock(shared_mutex); and shared_mutex.unlock(); with std::lock_guard<std::mutex> unlock(shared_mutex);

Questions:

  1. What is the problem in my current approach?
  2. Why does it work with loack_guard?

I am running the code on: https://www.onlinegdb.com/online_c++_compiler

SNR_BT
  • 133
  • 5
  • 2
    `shared_mutex.lock(shared_mutex);` ? does this compile? `std::mutex::lock` takes no parameter – 463035818_is_not_an_ai May 05 '22 at 08:29
  • Hello @463035818_is_not_a_number, please try now. That was a copy-paste error – SNR_BT May 05 '22 at 08:32
  • what do you mean when you say that there is a data race? Please include expected and actual output in the quesiton – 463035818_is_not_an_ai May 05 '22 at 08:32
  • 4
    *"Data race situation just disappears"*. Data race is still there, but you just don't see the wrong stuff happening. As any UB, seems to work is a possible output. – Jarod42 May 05 '22 at 08:36
  • Please explain why you *don't* expect a race condition with the code shown. You increment `shared_var` in `task_2` without first acquiring the `mutex`. – G.M. May 05 '22 at 08:36
  • @G.M. How would `shared_var` is accessed by `task_2` when it is locked by `task_1`? I expect `task_2` to not write the variable as long as it is locked. Am I wrong? – SNR_BT May 05 '22 at 08:40
  • *"[..] replace [..] `shared_mutex.unlock();` with `std::lock_guard unlock(shared_mutex);`"*. `std::lock_guard` unlock on destruction, Your second lock on already locked mutex is another UB (*"If lock is called by a thread that already owns the mutex, the behavior is undefined"*). – Jarod42 May 05 '22 at 08:42
  • If your shared data isn't protected in even *one* of the participants reading/writing that data, protecting it *at all* is futile. You're already in a data race condition. And it doesn't "work" by replacing that `.lock()` with a scope lock guard in only one thread; you just think it does. The race is still there. – WhozCraig May 05 '22 at 08:42
  • 1
    Use of a `mutex` to protect access to a region of memory (read as 'variables') is essentially cooperative: it only works if all threads use the `mutex` correctly. – G.M. May 05 '22 at 08:44
  • A mutex does not protect a variable. It protects a piece of code that sits between `lock` and `unlock` from executing in parallel with another piece of code in another thread that sits between `lock` and `unlock` of the same mutex. – n. m. could be an AI May 05 '22 at 08:52
  • `std::lock_guard unlock(shared_mutex);` doesn't do what you think it does – user253751 May 05 '22 at 09:20
  • Consider a door on a house that has no walls - it only prevents access when locked if everyone agrees to only pass through the door to access the house. Your shared variable is the house, the mutex is the door. – molbdnilo May 05 '22 at 09:29

3 Answers3

2

With UB (as data race), output is undetermined, you might see "expected" output, or strange stuff, crash, ...

  1. What is the problem in my current approach?

In first sample, you have data race as you write (non-atomic) shared_var in one thread without synchronization and read in another thread.

  1. Why does it work with loack_guard?

In modified sample, you lock twice the same (non-recursive) mutex, which is also UB

From std::mutex::lock:

If lock is called by a thread that already owns the mutex, the behavior is undefined

You just have 2 different behaviours for 2 different UB (when anything can happen for both cases).

Jarod42
  • 203,559
  • 14
  • 181
  • 302
2

Suppose you have a room with two entries. One entry has a door the other not. The room is called shared_var. There are two guys that want to enter the room, they are called task_1 and task_2.

You now want to make sure somehow that only one of them is inside the room at any time.

taks_2 can enter the room freely through the entry without a door. task_1 uses the door called shared_mutex.

Your question is now: Can achieve that only one guy is in the room by adding a lock to the door at the first entry?

Obviously no, because the second door can still be entered and left without you having any control over it.

If you experiment you might observe that without the lock it happens that you find both guys in the room while after adding the lock you don't find both guys in the room. Though this is pure luck (bad luck actually, because it makes you beleive that the lock helped). In fact the lock did not change much. The guy called task_2 can still enter the room while the other guy is inside.

The solution would be to make both go through the same door. They lock the door when going inside and unlock it when leaving the room. Putting an automatic lock on the door can be nice, because then the guys cannot forget to unlock the door when they leave.

Oh sorry, i got lost in telling a story.

TL;DR: In your code it does not matter if you use the lock or not. Actually also the mutex in your code is useless, because only one thread un/locks it. To use the mutex properly, both threads need to lock it before reading/writing shared memory.

463035818_is_not_an_ai
  • 109,796
  • 11
  • 89
  • 185
1

A mutex lock does not lock a variable, it just locks the mutex so that other code cannot lock the same mutex at the same time.

In other words, all accesses to a shared variable need to be wrapped in a mutex lock on the same mutex to avoid multiple simultaneous accesses to the same variable, it's not in any way automatic just because the variable is wrapped in a mutex lock in another place in the code.

You're not locking the mutex at all in task2, so there is a race condition.

The reason it seems to work when you wrap the mutex in a std::lock_guard is that the lock guard holds the mutex lock until the end of the scope which in this case is the end of the function.

Your function first locks the mutex with the lock lock_guard to later in the same scope try to lock the same mutex with the unlock lock_guard. Since the mutex is already locked by the lock lock_guard, execution stops and there is no output because the program is in effect not running anymore.

If you output "ok" in your code at the point of the "//do something" comment, you'll see that you get the output once and then the program stops all output.

Note; as of this behaviour being guaranteed, see @Jarod42s answer for much better info on that. As with most unexpected behaviour in C++, there is probably an UB involved.

Joachim Isaksson
  • 176,943
  • 25
  • 281
  • 294