Is my wait - notify mechanism using std::mutex correct?
Asked Answered
F

4

8

I started using std::mutexes to stop a thread and wait for another thread to resume it. It works like this:

Thread 1

// Ensures the mutex will be locked
while(myWaitMutex.try_lock());
// Locks it again to pause this thread
myWaitMutex.lock();

Thread 2

// Executed when thread 1 should resume processing:
myWaitMutex.unlock();

However I am not sure if this is correct and will work without problems on all platforms. If this is not correct, what is the correct way to implement this in C++11?

Frodi answered 28/4, 2017 at 8:51 Comment(3)
It seems that you didn't understand mutex at all. Read your docs again.Pentaprism
@TatsuyukiIshi Could you specify which part I didn't understand and should read again?Standley
Admittedly, flawless examples for the correct use of mutexes are hard to find even on Stack Overflow, but I suggest you start with the following: https://mcmap.net/q/978287/-waiting-for-an-atomic_boolPieria
D
9

The problems with the code

// Ensures the mutex will be locked
while(myWaitMutex.try_lock());

.try_lock() tries to aquire the lock and returns true if successful, i.e., the code says "if we aquire the lock then retry to lock it again and again until we fail". We can never "fail" as we currently own the lock ourselves that we are waiting on, and so this will be an infinite loop. Also, attempting to lock using a std::mutex that the caller have already aquired a lock on is UB, so this is guaranteed to be UB. If not successful, .try_lock() will return false and the while loop will be exited. In other words, this will not ensure that the mutex will be locked.

The correct way to ensure the mutex will be locked is simply:

myWaitMutex.lock();

This will cause the current thread to block (indefinitely) until it can aquire the lock.

Next, the other thread tries to unlock a mutex it does not have a lock on.

// Executed when thread 1 should resume processing:
myWaitMutex.unlock();

This won't work as it's UB to .unlock() on a std::mutex that you don't already have a lock on.

Using locks

When using mutex locks, it's easier to use a RAII ownership-wrapper object such as std::lock_guard. The usage pattern of std::mutex is always: "Lock -> do something in critical section -> unlock". A std::lock_guard will lock the mutex in its constructor, and unlock it in its destructor. No need to worry about when to lock and unlock and such low-level stuff.

std::mutex m;
{
    std::lock_guard<std::mutex> lk{m};
    /* We have the lock until we exit scope. */
} // Here 'lk' is destroyed and will release lock.

A simple lock might not be the best tool for the job

If what you want is to be able to signal a thread to wake up, then there's the wait and notify structure using std::condition_variable. The std::condition_variable allows any caller to send a signal to waiting threads without holding any locks.

#include <atomic>
#include <condition_variable>
#include <iostream>
#include <mutex>
#include <thread>

using namespace std::literals;

int main() {
    std::mutex m;
    std::condition_variable cond;

    std::thread t{[&] {
        std::cout << "Entering sleep..." << std::endl;
        std::unique_lock<std::mutex> lk{m};
        cond.wait(lk); // Will block until 'cond' is notified.
        std::cout << "Thread is awake!" << std::endl;
    }};

    std::this_thread::sleep_for(3s);

    cond.notify_all(); // Notify all waiting threads.

    t.join(); // Remember to join thread before exit.
}

However, to further complicate things there's this thing called spurious wakeups that mean that any waiting threads may wake up at any time for unknown reasons. This is a fact on most systems and has to do with the inner workings of thread scheduling. Also, we probably need to check that waiting is really needed as we're dealing with concurrency. If, for example, the notifying thread happens to notify before we start waiting, then we might wait forever unless we have a way to first check this.

To handle this we need to add a while loop and a predicate that tells when we need to wait and when we're done waiting.

int main() {
    std::mutex m;
    std::condition_variable cond;
    bool done = false; // Flag for indicating when done waiting.

    std::thread t{[&] {
        std::cout << "Entering sleep..." << std::endl;
        std::unique_lock<std::mutex> lk{m};
        while (!done) { // Wait inside loop to handle spurious wakeups etc.
            cond.wait(lk);
        }
        std::cout << "Thread is awake!" << std::endl;
    }};

    std::this_thread::sleep_for(3s);

    { // Aquire lock to avoid data race on 'done'.
        std::lock_guard<std::mutex> lk{m};
        done = true; // Set 'done' to true before notifying.
    }
    cond.notify_all();

    t.join();
}

There are additional reasons why it's a good idea to wait inside a loop and use a predicate such as "stolen wakeups" as mentioned in the comments by @David Schwartz.

Dolph answered 28/4, 2017 at 11:1 Comment(9)
This is a good answer except for one thing -- it incorrectly explains why you need a predicate. Even if there were no spurious wakeups, you'd still need a predicate for several reasons. The most obvious is that you don't want the thread to block on the condition variable if the wakeup code has already executed. There is no way to prevent that without a predicate.Buckles
@DavidSchwartz You're right, outside of this example you can't know if you're unnecessarily waiting. What other reasons are there, except spurious wakeups and this?Dolph
Stolen wakeups are the most common. For example, imagine you have a queue and two threads are waiting. You put an object on the queue and wake one thread. Then you put another object on the queue and wake the other thread. Then one thread takes both items off the queue before the second runs. The second thread must wait again. (But please, fix the race condition in your code!)Buckles
Even if there are no spurious wakeups, your second to last example would be broken. The code might never terminate. Imagine, for example, if stdout is a very slow terminal that takes four seconds to output a line. (It's almost impossible to write non-broken code that needs to deal with spurious wakeups, I've never seen code that was broken by spurious wakeups though I concede it might exist. There are always other races too.)Buckles
@DavidSchwartz Good points! I've updated my knowledge and my answer.Dolph
I like this answer the most. I want to note that although the code in my question is broken it actually works on both windows and linux. I also wanted to ask whether std provides any class that wraps all the predicate variable stuff.Standley
@TomášZato It doesn't really work. It has the same race condition I explained in my first comment on this answer (because it too has no predicate). You may not have hit that race condition, but it's there and someone will hit it sometime -- most likely the worst time.Buckles
@DavidSchwartz If I thought it really works, I would probably not ask this question, don't you think?Standley
@TomášZato Well, there are two different issues. One is whether the behavior you get on this platform is guaranteed on other platforms. That's what, I think, motivated you to ask this question. I'm saying, the behavior you are seeing on that platform isn't even guaranteed on that very platform because of a race condition. (Read my first comment on this answer for the explanation of why you must have a predicate.)Buckles
T
4

It sounds to me that you are looking for condition variable. In the end there should always be a way to make it work through mutexes, but condition variable is the current C++ idiomatic way to handle the `block and wait until something happens' scenario.

Transformer answered 28/4, 2017 at 8:59 Comment(0)
B
3

The behavior of a mutex when a thread that holds it attempts to lock it is undefined. The behavior of a mutex when a thread that doesn't hold it attempts to unlock it is undefined. So your code might do anything at all on various platforms.

Instead, use a mutex together with a condition variable and a predicate boolean. In pseudo-code:

To block:

  1. Acquire the mutex.

  2. While the predicate is false, block on the condition variable.

  3. If you want to re-arm here, set the predicate to false.

  4. Release the mutex.

To release:

  1. Acquire the mutex.

  2. Set the predicate to true.

  3. Signal the condition variable.

  4. Release the mutex.

To rearm:

  1. Acquire the mutex.

  2. Set the predicate to false.

  3. Release the mutex.

Buckles answered 28/4, 2017 at 8:53 Comment(10)
Be nice. Instead of just saying it's UB, try to explain what's wrong there and propose some solutions.Pentaprism
Undefined behavior is what's wrong, @Tatsuyuki. How is it not "nice" to point that out?Repetitious
I don't understand the purpose of the predicate here. Doesn't the condition variable already handle that?Standley
@TomášZato A condition variable is stateless. Here, you need a state (whether the thread should be blocked or should be permitted to continue), so something must store that state. When the thread decides whether or not it should continue, it has to check the state to make that decision. (The predicate permits the thread that needs to wait to decide whether or not it actually needs to wait or is done waiting.)Buckles
#3 and #4 under "To release:" should change places, otherwise the notified thread would immediately block again, waiting for the notifying thread to release the lock. As is would work I think, but is suboptimal.Dolph
@Snps No, you're wrong. The arrangement above is optimal. Your change makes it suboptimal. If you signal the condition variable while you hold the mutex, the implementation knows you have the mutex and knows that your signal cannot possibly make another thread ready-to-run. So that makes the signal, while holding the mutex, much cheaper than signalling without holding it. You are assuming a crappy implementation that idiotically doesn't see this even though it's totally obvious. The thing to do about crappy implementations is not use them, not pessimize code to make them suck less.Buckles
@DavidSchwartz Are you saying that std::condition_variable shouldn't be used? Because as I see it, std::condition_variable::notify_all can't know if you're holding the mutex or not. It can only know that no waiting threads hold it. Or do you mean that .notify_all() will reaquire the lock used by the waiting threads before signaling? Seems weird.Dolph
@Snps Why can't it know whether you're holding the mutex or not? You know a condition variable can only have one mutex associated with it at a time, right? (And, in practice, real implementations do know.) (Here's one example.) Are you saying this is impossible? Or are you saying it's a bad idea? Or what?Buckles
@DavidSchwartz The lock (and the mutex) is only passed when calling .wait(), not at construction. Do you mean that it keeps a reference to the mutex? That you have to call .wait() first for the std::condition_variable to work? The design doesn't seem to enforce that only one mutex can be used IMHO.Dolph
Let us continue this discussion in chat.Buckles
T
1

Please check this code....

std::mutex m_mutex;             
std::condition_variable m_cond_var;    

void threadOne(){
    std::unique_lock<std::mutex> lck(mtx);
    while (!ready){ 
        m_cond_var.wait(lck);
    }
    m_cond_var.notify_all();
}
void threadTwo(){
    std::unique_lock<std::mutex> lck(mtx);
    read = true;
    m_cond_var.notify_all();
}

I hope you will get the solution. And it is very proper!!

Township answered 28/4, 2017 at 9:49 Comment(2)
ready is never declared, read = true is maybe ready = true?, threadOne unnecessarily notifies itself, and threadTwo holds the lock while notifying which means that the notified thread can't reaquire and wake up.Dolph
Yes, you are correct its a typo. And as you said the threadOne is notifying itself. I just written that in case if there are any other threads running for this thread function. then it will be useful.Township

© 2022 - 2024 — McMap. All rights reserved.