patterncppModerate
Thread-safe queue
Viewed 0 times
queuethreadsafe
Problem
This is what I have which is mostly based on code I've found on Stack Overflow. How good is it? Can you suggest better implementation?
#pragma once
#include
#include
#include
template
class BlockingQueue
{
private:
std::mutex d_mutex;
std::condition_variable d_condition;
std::deque d_queue;
public:
void push(T const& value) {
{
std::unique_lock lock(this->d_mutex);
d_queue.push_front(value);
}
this->d_condition.notify_one();
}
T pop() {
std::unique_lock lock(this->d_mutex);
this->d_condition.wait(lock, [=]{ return !this->d_queue.empty(); });
T rc(std::move(this->d_queue.back()));
this->d_queue.pop_back();
return rc;
}
bool tryPop (T & v, std::chrono::milliseconds dur) {
std::unique_lock lock(this->d_mutex);
if (!this->d_condition.wait_for(lock, dur, [=]{ return !this->d_queue.empty(); })) {
return false;
}
v = std::move (this->d_queue.back());
this->d_queue.pop_back();
return true;
}
int size() {
return d_queue.size();
}
};Solution
Overall the code looks functional to me. Everytime you pop it will wait on the condition variable if the queue is empty. Everytime you push, a notification will be sent to any thread waiting in the pop method because the queue is empty and let it know that the queue is not empty anymore. I can't speak for your intentions but that is generally how people want their queues to work.
A few minor points I noticed:
The std::move in this line is useless since deque::back() will give you an rvalue anyway.
It looks like someone on SO trying to look clever not actually understanding the code.
This will suffice.
In the lambda functions, capturing everything by value is completely unnecessary.
Capture only the this pointer or only the deque by reference.
Both approaches have the same overhead.
A few minor points I noticed:
The std::move in this line is useless since deque::back() will give you an rvalue anyway.
std::move (this->d_queue.back());It looks like someone on SO trying to look clever not actually understanding the code.
This will suffice.
v = this->d_queue.back();In the lambda functions, capturing everything by value is completely unnecessary.
[=]{ return !this->d_queue.empty(); }Capture only the this pointer or only the deque by reference.
[this]{ return !this->d_queue.empty(); }
[&d_queue]{ return !d_queue.empty(); }Both approaches have the same overhead.
Code Snippets
std::move (this->d_queue.back());v = this->d_queue.back();[=]{ return !this->d_queue.empty(); }[this]{ return !this->d_queue.empty(); }
[&d_queue]{ return !d_queue.empty(); }Context
StackExchange Code Review Q#49820, answer score: 11
Revisions (0)
No revisions yet.