test_and_set makes the thread deadlock

#include<bits/stdc++.h>
using namespace std;
int cnt=0;
int locked=0;
int test_and_set(int * lock){
    int temp=*lock;
    *lock=1;
    return temp;
}
void cntOnes(int t){
    while(test_and_set(&locked));

    for(int i=0;i<2000;i++){
        cnt++;
        cout<<t<<"  ->  "<<cnt<<endl;
    }
    locked=0;
}
int main(){
    thread t1(cntOnes,1);
    thread t2(cntOnes,2);
    t1.join();
    t2.join();
}

I have used test_and_set method to make other threads waiting at first and the thread t1 can break the while loop but the even after the thread t1 sets the value to 0. thread t2 is continue running the while loop it does not break the while loop. What are the changes should be done?.

Upvotes: 0

Answers (1)

Nate Eldredge

Reputation: 58741

First of all, Why should I not #include <bits/stdc++.h>?

With that out of the way, unfortunately, your whole code is a great big data race. You simply cannot use ordinary variables to communicate between threads without using mutexes or atomics to synchronize them. Concurrently reading and writing an ordinary variable in two different threads is the definition of a data race, and the C++ standard says a data race causes undefined behavior (= makes your program completely broken).

Among the many things that can go wrong:

Because of the data race rule, the compiler assumes that an ordinary variable will not spontaneously change values without being written by this thread. Thus it thinks while(test_and_set(&locked));, if it does not succeed the first time, can never succeed after that, and might as well be optimized into an infinite loop. (Or since infinite loops are also UB, it might be deleted altogether.)
The compiler likewise assumes that no other thread is looking at the ordinary variables that we modify. So for instance, since it observes that a thread running cntOnes is inevitably going to set locked = 0 eventually, it could decide to do it before the 2000-step loop.
Your test_and_set is not atomic in any way. Even if the loads and stores were themselves atomic and sequentially consistent (which they're not), the operations could be interleaved as follows:

thread 1                               thread 2
========                               ========
temp = *lock; // temp = 0
                                       temp = *lock; // temp = 0
*lock = 1;
                                       *lock = 1;
return temp; // return 0               return temp; // return 0

Now both threads think they hold the lock. Oh dear.

So you cannot achieve a correct "lock" with an ordinary int variable. It seems like you are trying to reinvent std::mutex, so why not just use it?

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

int cnt = 0;
std::mutex my_lock;

void cntOnes(int t) {
    my_lock.lock();
    for(int i=0; i<2000; i++) {
        cnt++;
        std::cout << t << "  ->  " << cnt << std::endl;
    }
    my_lock.unlock();
}

int main() {
    std::thread t1(cntOnes,1);
    std::thread t2(cntOnes,2);
    t1.join();
    t2.join();
}

Or better, do it RAII-style with a std::scoped_lock:

void cntOnes(int t) {
    std::scoped_lock<std::mutex> guard(my_lock);
    for(int i=0; i<2000; i++) {
        cnt++;
        std::cout << t << "  ->  " << cnt << std::endl;
    }
}

Here the constructor of guard takes the mutex, and the destructor releases it. Prior to C++17 you can use std::lock_guard instead, as long as you're only working with one mutex.

If you insist on doing your own test and set, then you have to use atomics, e.g. std::atomic<bool> or std::atomic_flag. This is harder to get right, especially if you want to optimize memory ordering with anything weaker than std::memory_order_seq_cst; it requires a full understanding of the C++ memory model which is pretty abstract. So I would wait to try that approach until you can use mutexes comfortably.

test_and_set makes the thread deadlock

Answers (1)

Related Questions