0

If I have two critical sections, and I make two corresponding mutex to protect each of them.(I think it is necessary to precisely control when to lock because they use in different times and scenario, Q1:is it really so?) For example:

bool a;//ignore atomic_bool, because in actual, the data structure is more complex
mutex mut_a;//for threadsafe of a
bool b;
mutex mut_b;//for threadsafe of b

And I need to implement this kind of logic:

if (lock_guard<mutex> lck(mut_a);a) { do something...
}
else {
    if (lock_guard<mutex> lck(mut_b);b) { do something...
  }
}

As seen, lock_guard(mutex) is used nested, Q2is it proper and threadsafe?

user17732522
  • 53,019
  • 2
  • 56
  • 105
f1msch
  • 509
  • 2
  • 12
  • 1
    I don't see any safety issue (at least not immediately from this code), but what do you want to achieve here? The thread will keep the lock on `a` while it is processing `b`. Is that intended? That makes the lock on `b` redundant. And I don't understand what your Q1 is supposed to ask. – user17732522 Jun 15 '22 at 10:32
  • 1
    When locking multiple mutex you have to be careful with the order you lock them in. If one thread locks `mut_a` then `mut_b` and another thread locks `mut_b` then `mut_a` you run the risk of a deadlock if both threads lock their first mutex at the same time, and they both end up waiting for the other thread to unlock the other mutex. [`std::lock`](https://en.cppreference.com/w/cpp/thread/lock) can help with that. – François Andrieux Jun 15 '22 at 11:10

2 Answers2

2

I think a problem here is that

if (bool x = true)
{
    // x is in scope
}
else
{
    // x is STILL in scope!
    x = false;
}

So the first lock on mut_a is still held in the else block. Which might be your intention, but I would consider this not an optimal way to write it for readability if that were so.

Also, if it is important that !a in the critical section of b you DO need to keep the lock on a.

Aedoro
  • 609
  • 1
  • 6
  • 21
1

The code is not inherently unsafe. However, ...

As already mentioned in another answer, you need to consider that the mutex is locked also in the else branch:

if (lock_guard<mutex> lck(mut_a);a) { do something...
     // mut_a is locked
} else {
    if (lock_guard<mutex> lck(mut_b);b) { do something...
        // mut_a and mut_b are locked
    }
}

Maybe this is intended. If it is not then its wrong.

Moreover you need to be careful with locking multiple mutexes at once. When elsewhere you lock them in different order there will be a deadlock:

 {
     lock_guard<mutex> lock_b(mut_b);
     lock_guard<mutex> lock_a(mut_a);
     // do something
 }

Your code locks mut_a and only releases it after it was able to acquire a lock on mut_b. This code locks mut_b and only releases it after it was able to acquire a lock on mut_a.

To avoid such deadlock you can use std::scoped_lock that takes multiple mutexes and uses some deadlock avoidance algorithm.

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