6
\$\begingroup\$

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;
}
\$\endgroup\$
0

2 Answers 2

4
\$\begingroup\$

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.

\$\endgroup\$
3
  • \$\begingroup\$ Using stdafx.h as a "master" header would be perfectly legal on all platforms. And while you're right that it doesn't gain anything for a single file out of context, precompiled headers can offer a huge improvement in build times for large projects. If you make it conditional when building on Windows, you aren't doing yourself any favors, because now you have two different lists of includes to manage, depending on which environment you're building in. \$\endgroup\$ Commented Dec 23, 2016 at 14:58
  • \$\begingroup\$ Or, one could address all of those objections by simply omitting the line. \$\endgroup\$ Commented Dec 23, 2016 at 15:01
  • \$\begingroup\$ No, you couldn't, because then you cannot gain the benefits of precompiled headers with Microsoft's compiler. \$\endgroup\$ Commented Dec 23, 2016 at 15:01
0
\$\begingroup\$

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;
}
\$\endgroup\$
1
  • \$\begingroup\$ Yes, you are right. I made changes to the second function as I read further. Also, there is another problem, that the second thread, T2 will keep waiting for a notification from T1. I also made changes for that as well. Thank you for the heads up :) \$\endgroup\$ Commented Dec 26, 2016 at 16:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.