2

This is the link to the core guideline CP.42: Link to core guidelines

Currently I am trying to write a small program using the condition variable in the standard library when I came across this core guidline of using the wait(...), it seems that the thread waiting for the condition will be accidentally wake up by some events other than cond.notify_one() or cond.notify_all()?

My current approach uses cond.wait(lock) without the predicate because I am sure there will only be one thread calling the notify_one().

My problem is with this code example from the core guideline:

template<typename T>
class Sync_queue {
public:
    void put(const T& val);
    void put(T&& val);
    void get(T& val);
private:
    mutex mtx;
    condition_variable cond;    // this controls access
    list<T> q;
};

template<typename T>
void Sync_queue<T>::put(const T& val)
{
    lock_guard<mutex> lck(mtx);
    q.push_back(val);
    cond.notify_one();
}

template<typename T>
void Sync_queue<T>::get(T& val)
{
    unique_lock<mutex> lck(mtx);
    cond.wait(lck, [this] { return !q.empty(); });    // prevent spurious wakeup
    val = q.front();
    q.pop_front();
}

And the statement for this example:

Now if the queue is empty when a thread executing get() wakes up (e.g., because another thread has gotten to get() before it), it will immediately go back to sleep, waiting.

Well this sounds weird to me (at the line with comment // prevent spurious wakeup), even if another thread called get() before the current thread, shouldn't the cond.wait(lck) block until some other thread called cond.notify_one() or cond.notify_all()?

UPDATE

I will show my approach here as a reference of a bad practice to wait without a condition:

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

class Acquirer;

class Lockable {
    // Stores the last acquirer in the queue.
    std::atomic<Acquirer *> last_queued = std::atomic<Acquirer *>(nullptr);

    friend Acquirer;
};

class Acquirer {
public:
    enum Status {IDLE, ACQUIRED, SIGNALED_WAITING};

    Acquirer();
    void acquire(Lockable *lockable);
    void release();
private:
    std::mutex blocking_mutex;
    std::condition_variable blocking_condition_variable;
    Lockable *acquired_lockable;
    Acquirer::Status status;
};

Acquirer::Acquirer() {
    this->acquired_lockable = nullptr;
    this->status = Acquirer::IDLE;
}

void Acquirer::acquire(Lockable *lockable) {
    // Put myself in the queue for the lockable, done atomically.
    Acquirer *last_queued = lockable->last_queued.exchange(this);
    if (last_queued != nullptr) {
        // If there are someone in the queue, block myself and wait for the acquirer before me.
        {
            std::unique_lock<std::mutex> mutex_lock(last_queued->blocking_mutex);
            last_queued->blocking_condition_variable.wait(mutex_lock);
        }
        // Signal the acquirer before me that I am awake.
        last_queued->status = Acquirer::SIGNALED_WAITING;
    }
    // Acquired the lockable.
    this->status = Acquirer::ACQUIRED;
    this->acquired_lockable = lockable;
}

void Acquirer::release() {
    if (this->acquired_lockable == nullptr)
        return;
    // Attempt to reset the queued acquirer in the lockable.
    Acquirer *expected_operator = this;
    if (!acquired_lockable->last_queued.compare_exchange_strong(expected_operator, nullptr)) {
        // If this operation fails, it is guaranteed someone else have queued after me,
        // signal the waiting acquirer thread that it can continue until it replies.
        // The subsequent acquirer will set my status to Acquirer::SIGNALED_WAITING if it is awake.
        while(this->status != Acquirer::SIGNALED_WAITING) {
            this->blocking_condition_variable.notify_one();
        }
        this->acquired_lockable = nullptr;
    }
    this->status = Acquirer::IDLE;
}

Lockable lockable;

void execute(Acquirer *acquirer) {
    for (int i = 0; i < 10; i++) {
        acquirer->acquire(&lockable);
        std::cout << std::this_thread::get_id() << std::endl;
        std::this_thread::sleep_for(std::chrono::seconds(1));
        acquirer->release();
    }
}

int main() {
    Acquirer acquirer_1;
    Acquirer acquirer_2;
    Acquirer acquirer_3;

    std::thread thread_1(execute, &acquirer_1);
    std::thread thread_2(execute, &acquirer_2);
    std::thread thread_3(execute, &acquirer_3);

    thread_1.join();
    thread_2.join();
    thread_3.join();
    return 0;
}
Alphaharrius
  • 210
  • 1
  • 9
  • 2
    A condition variable is allowed to wake up spuriously, without anyone calling `notify_*`. But usually, a code that calls `wait` without a predicate, and not in a loop checking for some condition, can be shown to be broken without even considering spurious wakeups. Show how you'd write this example "your way", and I'll try to explain how it breaks. – Igor Tandetnik Jul 30 '22 at 04:37
  • @alphaharrius So your current approach is is akin to the "bad example" from the core guidelines? ***In that case*** it could work fine for your needs, but as indicated in the guidelines, it's not best practice. "shouldn't the cond.wait(lck) block until some other thread called cond.notify_one() or cond.notify_all()?" -> ideally, yes, ***if*** your program logic is okay with it--in some cases, it could lead to the thread waiting forever; but spurious wake-ups can happen nevertheless and it can lead to issues. – Nick S. Jul 30 '22 at 05:00
  • Thanks for your comment, I have updated the post and my approach is at the bottom. @IgorTandetnik – Alphaharrius Jul 30 '22 at 05:05
  • Are there any source of waking that I can take a note of? Like interference from the operating system, or there is a time limit for the wait ...etc? @NickS. – Alphaharrius Jul 30 '22 at 05:07
  • 1
    @Alphaharrius I'll refer you to this thread, is has some good details: https://stackoverflow.com/questions/8594591/why-does-pthread-cond-wait-have-spurious-wakeups. From what I see, there is no "time limit" unless you include the predicate at cpp-reference: https://en.cppreference.com/w/cpp/thread/condition_variable/wait or at the source code: https://gcc.gnu.org/onlinedocs/gcc-4.6.2/libstdc++/api/a00818_source.html. In the listing you've made, it seems you can get spurious wake up/race conditions with 1 thread being blocked by the other 2. How likely is that? That's a different question. – Nick S. Jul 30 '22 at 05:38
  • You have a straight-up data race on `Acquirer::status`. One instance calls `notify_one` and then checks `this->status`, while another instance comes out of `wait` and writes to the first instance's `status`. – Igor Tandetnik Jul 30 '22 at 15:02
  • @IgorTandetnik I would like to know more on this, what kind of concrete problem will this data race summons? Will there be a data corruption or some sort, Thanks! – Alphaharrius Aug 01 '22 at 16:55
  • It breaks the logic. Concretely it affects the `while(this->status != Acquirer::SIGNALED_WAITING) { ... }`. That can't legally notice a change in `this->status`, so it can be transformed to `if (this->status != Acquirer::SIGNALED_WAITING) while (true) { ... }`, an infinite loop. This is a transformation compilers can do in practice. – Jeff Garrett Aug 01 '22 at 20:08

0 Answers0