patterncppMinor
Insert and Remove Element in Deque using threads in C++
Viewed 0 times
insertthreadsdequeremoveelementusingand
Problem
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.
I would appreciate any help on how to make it better, especially in terms of thread safety.
#include "stdafx.h"
#include
#include
#include
#include
#include
#define SLEEP_TIME 1
std::deque q;
std::mutex _mu;
std::condition_variable cond;
void function1()
{
int count = 10;
while (count > 0)
{
std::unique_lock 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 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;
}Solution
Here are some things that may help you improve your code.
Isolate platform-specific code
If you must have
In this code, with a single file, the use of
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,
Remember that
I realize that this code is probably written for practice using threads and locks and such, but it's important to remember that
Remember that
It's not an issue in this code because only one thread at a time prints to
Isolate platform-specific code
If you must have
stdafx.h, consider wrapping it so that the code is portable:#ifdef WINDOWS
#include "stdafx.h"
#endifIn 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, tooI 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 resourceIt'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.Code Snippets
#ifdef WINDOWS
#include "stdafx.h"
#endifContext
StackExchange Code Review Q#150665, answer score: 4
Revisions (0)
No revisions yet.