Minimal mutexes for std::queue producer/consumer

struct MyData{ int a; int b; }; class SpeedyAccess{ public: void AddDataFromThread1(MyData data){ const std::lock_guard<std::mutex> queueMutexLock(queueAccess); workQueue.push(data); } void CheckFromThread2(){ if(!workQueue.empty()) // Un-protected access...is this dangerous? { queueAccess.lock(); MyData data = workQueue.front(); workQueue.pop(); queueAccess.unlock(); ExpensiveComputation(data); } } private: void ExpensiveComputation(MyData& data); std::queue<MyData> workQueue; std::mutex queueAccess; }

As written in the comments above, you should call empty() only under a lock.

But I believe there is a better way to do it.
You can use a std::condition_variable together with a std::mutex, to achieve synchronization of access to the queue, without locking the mutex more than you must.

However - when using std::condition_variable, you must be aware that it suffers from spurious wakeups. You can read about it here: Spurious wakeup - Wikipedia.
You can see some code examples here: Condition variable examples.

The correct way to use a std::condition_variable is demonstrated below (with some comments). This is just a minimal example to show the principle.

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

using MyData = int;

std::mutex mtx;
std::condition_variable cond_var;
std::queue<MyData> q;

void producer()
{
    MyData produced_val = 0;
    while (true)
    {
        std::this_thread::sleep_for(std::chrono::milliseconds(1000));   // simulate some pause between productions
        ++produced_val;
        std::cout << "produced: " << produced_val << std::endl;
        {
            // Access the Q under the lock:
            std::unique_lock<std::mutex> lck(mtx);
            q.push(produced_val);
            cond_var.notify_all();  // It's not a must to nofity under the lock but it might be more efficient (see @DavidSchwartz's comment below).
        }
    }
}

void consumer()
{
    while (true)
    {
        MyData consumed_val;
        {
            // Access the Q under the lock:
            std::unique_lock<std::mutex> lck(mtx);
            // NOTE: The following call will lock the mutex only when the the condition_varible will cause wakeup
            //       (due to `notify` or spurious wakeup).
            //       Then it will check if the Q is empty.
            //       If empty it will release the lock and continue to wait. 
            //       If not empty, the lock will be kept until out of scope.
            //       See the documentation for std::condition_variable.
            cond_var.wait(lck, []() { return !q.empty(); }); // will loop internally to handle spurious wakeups
            consumed_val = q.front();
            q.pop();
        }
        std::cout << "consumed: " << consumed_val << std::endl;
        std::this_thread::sleep_for(std::chrono::milliseconds(200));    // simulate some calculation
    }
}

int main()
{
    std::thread p(producer);
    std::thread c(consumer);
    while(true) {}
    p.join(); c.join(); // will never happen in our case but to remind us what is needed.
    return 0;
}

Some notes:

In your real code, none of the threads should run forever. You should have some mechanism to notify them to gracefully exit.
The global variables (mtx,q etc.) are better to be members of some context class, or passed to the producer() and consumer() as parameters.
This example assumes for simplicity that the producer's production rate is always low relatively to the consumer's rate. In your real code you can make it more general, by making the consumer extract all elements in the Q each time the condition_variable is signaled.
You can "play" with the sleep_for times for the producer and consumer to test varios timing cases.
For more complex data types that benefit from move semantics, you can change:
```
consumed_val = q.front();
```
to:
```
consumed_val = std::move(q.front());
```
for better efficiency (since the front element in the queue is poped right afterwards anyway).

The code above assumes that MyData is default constructable.
If this is not the case, you can use an immediatly invoked lambda in the loop body in the consumer:

MyData consumed_val = 
     [](){ std::unique_lock<std::mutex> lck(mtx);
           cond_var.wait(lck, []() { return !q.empty(); }); 
           MyData val = std::move(q.front()); 
           q.pop(); 
           return val; }
     ();   // <-- immediatly invoke the lambda to initialize consumed_val

The usage of std::move together with NRVO should make it efficient.

Recommended topics

Hot tags