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

Question

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?

Answer 1

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.