Insert and Remove Element in Deque using threads in C++

Question

The following code works fine for inserting and removing an element from a deque using two threads.

I would appreciate any help on how to make it better, especially in terms of thread safety.

#include "stdafx.h"
#include <deque>

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

#define SLEEP_TIME 1

std::deque <int> q;
std::mutex _mu;
std::condition_variable cond;

void function1()
{
    int count = 10;

    while (count > 0)
    {

        std::unique_lock <std::mutex> locker(_mu);
        q.push_front(count);
        locker.unlock();
        cond.notify_one();
        std::this_thread::sleep_for(std::chrono::seconds(SLEEP_TIME));

        count--;
    }

    return;
}

void function2()
{
    int data = 0;

    while (data != 1)
    {
        std::unique_lock<std::mutex> locker(_mu);
        cond.wait(locker);

        data = q.back();
        q.pop_back();
        locker.unlock(),

        std::cout << "T2 gets " << data << " from T1" << std::endl;
    }

    return;

}

int main() {

    std::thread t2(function1);
    std::thread t1(function2);

    t1.join();
    t2.join();

    std::cout << "Thread Function Executed Successfully" << std::endl;
}

Answer 1

Here are some things that may help you improve your code.

Isolate platform-specific code

If you must have stdafx.h, consider wrapping it so that the code is portable:

#ifdef WINDOWS
#include "stdafx.h"
#endif

In this code, with a single file, the use of stdafx.h does not gain anything on any platform, including Windows.

Don't use leading underscores in names

Anything with a leading underscore is a reserved name in C++ (and in C). See this question for details.

Be careful with conditional variables

Right now, t2 waits for a conditional variable and t1 sets it. If t1 never pushes anything to the deque, t2 would wait forever and never terminate. Similarly, if t1 is launched before t2, it could execute the cond.notify_one() before any threads are waiting for that variable, so t1 would push 10 and notify, but no thread would "hear" the notification with the effect that t2 would only be notified after t1 pushes 9. The effect is that the code would almost work but never terminate. To allow the launching of threads in either order, I'd recommend that t2 should keep popping until the deque is empty. This is a typical pattern in multithreaded code.

Remember that main is a thread, too

I realize that this code is probably written for practice using threads and locks and such, but it's important to remember that main is also a thread, so this program actually uses three threads and not two. Because the main function isn't really doing anything useful while the threads are running, an alternative approach would be to launch only t1 as a thread and then simply invoke function2 directly from main, thereby using only two threads instead of three and getting the same work accomplished.

Remember that std::cout is a shared resource

It's not an issue in this code because only one thread at a time prints to std::cout, but as you create more complex code, it's important to remember that std::cout is a shared resource, too, For that reason, you'll want to have a mutex for access to it as well if you want to avoid possible interleaving of output.

Answer 2

The code in function2 is logically incorrect. Remember that condition_variables have spurious wakeups. You must test an actual condition, otherwise you may end up taking a reference to the back of an empty queue:

while (data != 1)
{
    std::unique_lock<std::mutex> locker(_mu);

    // there must be a predicate
    while(not q.empty())
    {
        cond.wait(locker);
    }

    data = q.back();
    q.pop_back();
    locker.unlock(),

    std::cout << "T2 gets " << data << " from T1" << std::endl;
}