I have written a threadsafe queue and I would like you to suggest what can be improved and how to write good unit tests for this implementation.
#include <mutex>
#include <queue>
#include <atomic>
#include <condition_variable>
#include <concepts>
#include <optional>
template<typename LockT>
concept Lockable = requires(LockT lock) {
{ lock.try_lock() } -> std::convertible_to<bool>;
{ lock.lock() } -> std::convertible_to<void>;
{ lock.unlock() }-> std::convertible_to<void>;
};
template<typename T, Lockable LockT = std::mutex>
struct ThreadSafeQueue
{
ThreadSafeQueue() = default;
void push(std::convertible_to<T> auto&& func)
{
{
std::scoped_lock lock(lockT);
queue.push(std::forward<decltype(func)>(func));
}
cv.notify_one();
}
[[nodiscard]] T pop()
{
T item = std::move(pop_opt().value());
return item;
}
[[nodiscard]] std::optional<T> try_pop()
{
auto item = pop_opt();
return item;
}
[[nodiscard]] inline std::size_t size()
{
std::scoped_lock lock(lockT);
return queue.size();
}
[[nodiscard]] inline bool empty()
{
std::scoped_lock lock(lockT);
return queue.empty();
}
inline void request_quit()
{
quit = true;
cv.notify_all();
}
[[nodiscard]] inline bool quit_requested() const
{
return quit;
}
private:
[[nodiscard]] std::optional<T> pop_opt()
{
std::unique_lock lock(lockT);
while (queue.empty() && !quit) {
cv.wait(lock);
}
if (queue.empty()) {
return std::nullopt;
}
auto item = std::move(queue.front());
queue.pop();
lock.unlock();
return item;
}
std::queue<T> queue;
LockT lockT;
std::condition_variable_any cv;
std::atomic_bool quit = false;
};
#include <gtest/gtest.h>
#include <vector>
#include "ThreadSafeQueue.h"
TEST(ThreadSafeQueueTest, MultipleThreads)
{
ThreadSafeQueue<int> queue;
constexpr static std::size_t kNumThreads = 10;
constexpr static int kNumIterations = 1000;
std::vector<std::thread> threads;
threads.reserve(kNumThreads);
for (int i = 0; i < kNumThreads; ++i) {
threads.emplace_back([&queue]() {
for (int j = 0; j < kNumIterations; ++j) {
queue.push(j);
}
});
}
for (int i = 0; i < kNumThreads; ++i) {
threads.emplace_back([&queue]() {
for (int j = 0; j < kNumIterations; ++j) {
int value = queue.pop();
EXPECT_GE(value, 0);
EXPECT_LT(value, kNumIterations);
}
});
}
for (auto& thread : threads) {
thread.join();
}
EXPECT_TRUE(queue.empty());
}
int main(int argc, char** argv)
{
::testing::InitGoogleTest(&argc, argv);
return RUN_ALL_TESTS();
}
pop()
andtop()
(AKAfront()
). See: en.cppreference.com/w/cpp/container/queue. pop() removes value without returning it, while top() (front()) returns the value. \$\endgroup\$front()
followed bypop()
, so I think it's not that bad to connect these 2 functions into 1. \$\endgroup\$pop()
that returns a value is not safe in the presence of exceptions. See: stackoverflow.com/a/25035949/14065 (I must admit I know of the rule but have not done extensive research and simply googled why they are normally seporated). Hence this is only a comment. \$\endgroup\$pop()
to return the value as you might not be able tofront()
thenpop()
the same value \$\endgroup\$