Is it safe to wait/notify on unique_lock mutexes when same mutexes are used with lock_guard on somewhere else?

Question

I was using following kind of wait/signal way to let threads inform each other.

std::condition_variable condBiz;
std::mutex mutexBar;
..
void Foo::wait()
{
    std::unique_lock<std::mutex> waitPoint(mutexBar);
    if (waitPoint.owns_lock())
    {
        condBiz.wait(waitPoint);
    }
}
void Foo::signal()
{
    std::unique_lock<std::mutex> waitPoint(mutexBar);
    condBiz.notify_all();
}
void Foo::safeSection(std::function<void(void)> & f)
{
   std::unique_lock<std::mutex> waitPoint(mutexBar); 
   f();
}

Then converted lock/unlock mechanism from unique_lock to lock_guard because I'm not returning unique_lock to use somewhere else(other than wait/signal) and lock_guard is said to have less overhead:

void Foo::safeSection(std::function<void(void)> & f)
{
   std::lock_guard<std::mutex> waitPoint(mutexBar);  // same mutex object
   f();
}

and it works.

Does this work for all platforms or just looks like working for current platform? Can unique_lock and lock_guard work with each other using same mutex object?

Your code doesn't make sense, why check `owns_lock()` after locking it ? What condition are you waiting ? — llllllllll, Mar 18 '18 at 15:50
You need to read the documentation of `unique_lock`, it must be already locked at that point. And your way of using condition variable is very wrong. — llllllllll, Mar 18 '18 at 15:54
I'm just waiting for a signal to wake it up, not necessarily a special condition. — huseyin tugrul buyukisik, Mar 18 '18 at 15:57
A typical race condition. What if the signal is sent *before* sleeping at a condition_variable ? — llllllllll, Mar 18 '18 at 15:59
Thats why there is `std::unique_lock waitPoint(mutexBar);` before `notify_all` — huseyin tugrul buyukisik, Mar 18 '18 at 16:01

Chris Vine · Answer 1 · 2018-03-18T16:29:19.727

2

It has been pointed out in the comments to your post that checking if the unique_lock is owned in Foo::wait() is pointless, because the associated mutex must be owned by the lock at that point in order for the thread to be proceeding.

Instead your condition variable should be checking some meaningful condition, and it should do so in a while loop or by using the overload of condition_variable::wait which takes a predicate as its second argument, which is required by the C++ standard to have effect as:

while (!pred()) wait(lock);

The reason for checking the predicate in a while loop is that, apart from the fact that the condition may already be satisfied so no wait is necessary, the condition variable may spuriously wake up even when not signalled to do so.

Apart from that there is no reason why the signalling thread should not use a lock_guard with respect to the associated mutex. But I am not clear what you are trying to do.

edited Mar 18 '18 at 16:29

answered Mar 18 '18 at 16:18

Chris Vine

677
3
7

Do you mean that I should set some bool or something in the `signal` method and check it as `pred()` in `wait()` just to elliminate spurious wakeups? What kind of performance does this give? Does this decrease lock contention? – huseyin tugrul buyukisik Mar 18 '18 at 16:28
You should have some test of the condition, yes, such as on a boolean flag. But what is the wait actually intended to be conditional on? – Chris Vine Mar 18 '18 at 16:30
Its just waiting a signal(but not for spurious ones, unfortunately). For example, N threads consuming, 1 thread producing. Producer warns consumers "there is more work", using a separate locker class, not the queue itself. Thats why there isn't any special condition here. – huseyin tugrul buyukisik Mar 18 '18 at 16:31
Normally, apart from spurious wake-ups, you need a condition test to avoid a race condition, in particular to cover a case where the condition is already satisfied and the signal has already been delivered.Where you know there can be no race condition then you still need a while loop because the standard explicitly states that the wait function "will unblock when signaled by a call to notify_one() or a call to notify_all(), or spuriously". – Chris Vine Mar 18 '18 at 16:35
@huseyintugrulbuyukisik If there's more work, then that is the condition to check for, e.g. a flag `while (!more_work) condBiz.wait(waitPoint);` or if it is a queue with some work orders `while(q.size() == 0) condBiz.wait(waitPoint);` – Olaf Dietsche Mar 18 '18 at 17:54

score 2 · Accepted Answer · edited Jun 20 '20 at 09:12

Both std::unique_lock and std::lock_guard lock the associated mutex in the constructor and unlock it in the destructor.

std::unique_lock:

Member functions

(constructor) constructs a unique_lock, optionally locking the supplied mutex
(destructor) unlocks the associated mutex, if owned

and the same for std::lock_guard:

Member functions

(constructor) constructs a lock_guard, optionally locking the given mutex
(destructor) destructs the lock_guard object, unlocks the underlying mutex

Since both behave the same, when used as a RAII style wrapper, I see no obstacle to use them together, even with the same mutex.

Is it safe to wait/notify on unique_lock mutexes when same mutexes are used with lock_guard on somewhere else?

2 Answers2