4

Application without std::condition_variable:

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

std::mutex mutex;
std::queue<int> queue;
int counter;

void loadData()
{
    while(true)
    {
        std::unique_lock<std::mutex> lock(mutex);
        queue.push(++counter);
        lock.unlock();
        std::this_thread::sleep_for(std::chrono::seconds(1));
    }
}

void writeData()
{
    while(true)
    {
        std::lock_guard<std::mutex> lock(mutex);
        while(queue.size() > 0)
        {
            std::cout << queue.front() << std::endl;
            queue.pop();
        }
    }
}

int main()
{
    std::thread thread1(loadData);
    std::thread thread2(writeData);
    thread1.join();
    thread2.join();
    return 0;
}

Application with std::condition_variable:

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

std::mutex mutex;
std::queue<int> queue;
std::condition_variable condition_variable;
int counter;

void loadData()
{
    while(true)
    {
        std::unique_lock<std::mutex> lock(mutex);
        queue.push(++counter);
        lock.unlock();
        condition_variable.notify_one();
        std::this_thread::sleep_for(std::chrono::seconds(1));
    }
}

void writeData()
{
    while(true)
    {
        std::unique_lock<std::mutex> lock(mutex);
        condition_variable.wait(lock, [](){return !queue.empty();});
        std::cout << queue.front() << std::endl;
        queue.pop();
    }
}

int main()
{
    std::thread thread1(loadData);
    std::thread thread2(writeData);
    thread1.join();
    thread2.join();
    return 0;
}

If I am right, it means that second version of this application is unsafe, because of queue.empty() function, which is used without any synchronization, so there are no locks. And there is my question: Should we use condition_variables if they cause problems like this one mentioned before?

Akasata Akasata
  • 333
  • 4
  • 10
  • 1
    Not 100% sure, but I think that the predicate you pass to `wait` is only called with the lock acquired, so in this case it is safe (since every operation on `queue` is locked with `mutex`). – Holt May 01 '17 at 19:58
  • Your second version is "safe"... ... – WhiZTiM May 01 '17 at 19:59
  • And I am aware of fact that second example causes lower CPU usage. – Akasata Akasata May 01 '17 at 20:00
  • Oh God, what a mistake.. predicate is used after locking a mutex. There was no question at all. – Akasata Akasata May 01 '17 at 20:02
  • It's not necessary that clear, since `.wait()` actually releases the lock. but it does not check the condition without holding the lock. Would be nice if the standard would actually mention that, would it? ;-) – dhke May 01 '17 at 20:04
  • @dhke The standard mandates that `wait(lock, pred)` to behave as `while (!pred()) wait(lock);`, and due to the behavior of `wait(lock)`, it mandates that `pred()` is never called without lock acquired. – Holt May 01 '17 at 20:06
  • @Holt Yeah, and then you go to `wait(lock)` to find out what that actually does. Would be nice if we went back to documenting invariants. – dhke May 01 '17 at 20:07
  • @dhke `wait(lock)` will indeed release `lock` at the beginning, but it will re-acquire it when notified, so your call to `pred()` will be guarded. – Holt May 01 '17 at 20:08
  • @Holt *sigh* I got that at the first comment. I still find the C++ docs annoying, since they are usually correct, but often misleadingly so. – dhke May 01 '17 at 20:13

2 Answers2

4

Your first example busy waits -- there is a thread pounding on the lock, checking, then releasing the lock. This both increases contention of the mutex and wastes up to an entire CPU when nothing is being processed.

The second example has the waiting thread mostly sleeping. It only wakes up when there is data ready, or when there is a "spurious wakeup" (with the standard permits).

When it wakes up, it reacquires the mutex and checks the predicate. If the predicate fails, it releases the lock and waits on the condition variable again.

It is safe, because the predicate is guaranteed to be run within the mutex you acquired and passed to the wait function.

Yakk - Adam Nevraumont
  • 262,606
  • 27
  • 330
  • 524
0

The second code is safe because the call to wait(lock, pred) is equivalent to (directly from the standard):

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

And a call to wait(lock) release (unlock) lock, and reacquire (lock) it on notification.

In your case, this is equivalent to:

auto pred = [](){return !queue.empty();};
std::unique_lock<std::mutex> lock(mutex); // acquire
while (!pred) { // Ok, we are locked
    condition_variable.wait(lock); // release
    // if you get here, the lock as been re-acquired
}

So all the calls to your pred are made with lock locked/acquired - No issue here as long as all other operations to queue are also guarded.

Holt
  • 36,600
  • 7
  • 92
  • 139