Blocking Queue Class Unit Test

Question

I would like any feedback on making the unit test for a BlockingQueue class more robust or better? improvements to the coding style, use of the gtest framework, better test cases etc. C++ style suggestions (up to c++ 14). Feel free to add suggestion to improve the BlockingQueue class if you feel inclined to.

BlockingQueue.h

#pragma once

#include <condition_variable>
#include <mutex>
#include <thread>
#include <queue>
#include <string>
#include <iostream>
#include <sstream>

template <typename T>
class BlockingQueue
{
public:
    BlockingQueue() = default;

    BlockingQueue(BlockingQueue<T>&& blockingQ);

    BlockingQueue<T>& operator=(BlockingQueue<T>&& blockingQ);

    BlockingQueue(const BlockingQueue<T>&) = delete;
    BlockingQueue<T>& operator=(const BlockingQueue<T>&) = delete;

    T deQ();
    void enQ(const T& t);
    T& front();
    void clear();
    size_t size();

private:
    std::queue<T> queue_;
    std::mutex mtx_;
    std::condition_variable conditionVar_;
};

// Move constructor
template<typename T>
BlockingQueue<T>::BlockingQueue(BlockingQueue<T>&& blockingQ)
{
    std::lock_guard<std::mutex> lock(mtx_);
    // Here we are moving blockingQ.queue into queue_
    queue_ = blockingQ.queue_;
    while (blockingQ.queue_.size() > 0)
    {
        blockingQ.queue_.pop();
    }
    // can't copy or move mutex or condition variable, so use default members
}

//Move assignment
template<typename T>
BlockingQueue<T>& BlockingQueue<T>::operator=(BlockingQueue<T>&& blockingQ)
{
    if (this == &blockingQ)
    {
        return *this;
    }

    std::lock_guard<std::mutex> lock(mtx_);
    queue_ = blockingQ.queue_;
    while (blockingQ.queue_.size() > 0) //  clear blockingQ
    {
        blockingQ.queue_.pop();
    }

    // can't move assign mutex or condition variable so use target's
    return *this;
}

// Remove element from front of queue
template<typename T>
T BlockingQueue<T>::deQ()
{
    std::unique_lock<std::mutex> lock(mtx_);
    // This lock type is required for use with condition variables.
    // The operating system needs to lock and unlock the mutex:
    // * When wait is called, below, the OS suspends waiting thread and releases lock.
    // * When notify is called in enQ() the OS relocks the mutex, 
    // resumes the waiting thread and sets the condition variable to signaled state.
    // std::lock_guard does not have publick lock and unlock functions

    if (queue_.size() > 0)
    {
        T temp = queue_.front();
        queue_.pop();
        return temp;
    }

    // may have spurious returns so loop on !condition
    conditionVar_.wait(lock, [this]() {return queue_.size() > 0; });


    T temp = queue_.front();
    queue_.pop();
    return temp;
}

// push element onto back of queue
template<typename T>
void BlockingQueue<T>::enQ(const T& t)
{
    {
        std::unique_lock<std::mutex> lock(mtx_);
        queue_.push(t);
    }
    conditionVar_.notify_one();
}

// Peek at next item to be popped
template <typename T>
T& BlockingQueue<T>::front()
{
    std::lock_guard<std::mutex> lock(mtx_);
    if (queue_.size() > 0)
    {
        return queue_.front();
    }

    throw std::exception("attemp to deQue empty queue");
}

template <typename T>
void BlockingQueue<T>::clear()
{
    std::lock_guard<std::mutex> l(mtx_);
    while (queue_.size() > 0)
        queue_.pop();
}

// return number of elements in queue
template<typename T>
size_t BlockingQueue<T>::size()
{
    std::lock_guard<std::mutex> l(mtx_);
    return queue_.size();
}

test.cpp

#include "pch.h"
#include "../BlockingQueue/BlockingQueue.h"

// This test creates multiple threads that push and pop items from a BlockingQueue
// simultaneously. It tests if the BlockingQueue is thread-safe and can handle race
// conditions correctly.
TEST(BlockingQueueTest, MultipleThreads) 
{
    BlockingQueue<int> queue;
    std::vector<std::thread> threads;

    // Create 10 threads that push 100 items to the queue
    for (int i = 0; i < 10; ++i)
    {
        threads.emplace_back([&queue]() {
            for (int j = 0; j < 100; ++j)
            {
                queue.enQ(j);
            }
            });
    }

    // Create 10 threads that pop 100 items from the queue
    for (int i = 0; i < 10; ++i)
    {
        threads.emplace_back([&queue]() {
            for (int j = 0; j < 100; ++j)
            {
                queue.deQ();
            }
            });
    }

    // Wait for all threads to finish
    for (auto& thread : threads)
    {
        thread.join();
    }

    // Test if the queue is empty
    EXPECT_EQ(queue.size(), 0);
}

// This test creates two threads that push and pop items from a BlockingQueue.
// One thread pops items faster than the other, causing a race condition.
// This test checks if the BlockingQueue can handle this race condition correctly.
TEST(BlockingQueueTest, RaceCondition)
{
    BlockingQueue<int> queue;
    std::thread popThread([&queue]() {
        for (int i = 0; i < 1000; ++i)
        {
            queue.deQ();
            // Add a sleep to simulate a slower consumer
            std::this_thread::sleep_for(std::chrono::milliseconds(10));
        }
        });

    // Push 1000 items to the queue
    for (int i = 0; i < 1000; ++i)
    {
        queue.enQ(i);
    }

    // Wait for the pop thread to finish
    popThread.join();

    // Test if the queue is empty
    EXPECT_EQ(queue.size(), 0);
}

Answer 1

Use std::swap() to move construct/assign

I see you repeated the code to clear a queue in many places. However, the destructor of BlockingQueue should already take care of properly clearing itself. So in the move constructor and move assignment operator, I recommend you just use std::swap() to swap the queue member variables:

template<typename T>
BlockingQueue<T>::BlockingQueue(BlockingQueue<T>&& other)
{
    std::swap(queue_, other.queue_);
}

template<typename T>
BlockingQueue<T>& BlockingQueue<T>::operator=(BlockingQueue<T>&& other)
{
    std::lock_guard<std::mutex> lock(mtx_);
    std::swap(queue_, other.queue_);
    return *this;
}

std::swap() will handle the case where this == &other correctly. Your class has an implicitly generated destructor that will in turn destroy all items in queue_ correctly.

No need to take the lock in a constructor

You don't need to lock mtx_ in the constructor itself. Nothing can possibly use the object before the constructor returns after all.

Avoid repeating code unnecessarily

In deQ() you wrote code to pop an item twice, once before the call to wait() and once after. However, you don't need to do it twice. If you use wait() with a predicate, it will check the predicate before doing a wait, so you don't have to do that yourself. So you can just write:

template<typename T>
T BlockingQueue<T>::deQ()
{
    std::unique_lock<std::mutex> lock(mtx_);
    conditionVar_.wait(lock, [this]() { return !queue_.empty(); });
    T temp = std::move(queue_.front());
    queue_.pop();
    return temp;
}

std::move() items out of the queue

As shown above, you should use std::move() when taking an item out of the queue, as this might result in more optimal code being generated. Also think about T being possibly non-copyable.

Allow items to be moved or emplaced in the queue

Your enQ() takes an item by const reference. That works, but requires a copy to be made when adding it to queue_. Consider adding overloads that takes an r-value reference, so the item can be moved into the queue_. It might also make sense to add a version that works like emplace(). Again, this allows more optimal code to be generated, and allows non-copyable types to be used.

It's not thread-safe

Your front() returns a reference to an element in queue_. However, by the time front() returns, another thread might have called deQ() and caused that reference to not be valid anymore.

Calling size() in itself will not cause any issues, but by the time it returns, the size of the queue might already be different. So any code relying on the return value of size() is asking for trouble. It's best to remove this function.

You could state that your code only allows two threads at a time, one producer thread that only ever adds items to the queue, and another one that only ever looks at items or removes them. In that case, front() and size() might make sense. However, I recommend that you avoid this and make it safe for an arbitrary number of producer/consumer threads. This will result in less surprises. Also, the test cases suggest that it should handle many simultaneous producers and consumers.

Try to use the same naming convention as the standard library

For a programmer who is already used to the standard library, it would be much nicer if your class uses the same exact terminology, so they don't have to learn that your class uses enQ() instead of push() or emplace(). Even better, if you have the same interface as the standard library, some STL algorithms may also work on your queue.

Testing for race conditions

It's is really hard to test for race conditions. By their very nature, they only trigger in rare conditions. There is no guarantee that your tests will hit a race condition, so the fact that they pass cannot be used to say that your code is race-free. Even if a race condition is triggered, it might not result in an exception or a crash.

There's this comment:

// One thread pops items faster than the other, causing a race condition.

It's actually the thread that pushes that runs faster. But even if the thread popping would run faster, then your code should still run fine and never hit a race condition; after all, pop() will block until a new element is available.

Writing proper tests

Your test cases don't really test much. The only EXPECT I see is that the queue is empty after all threads finish. But what about the values that deQ() returns? It could always return zeroes for all you know, and the test just passes.

Make sure you test all the properties that a queue has: what goes in comes out. Maybe there is also some ordering guarantee you want to preserve: if you enqueue values in ascending order, a single thread calling deQ() multiple times will also only ever see ascending results.

Make sure you test all member functions, constructors, assignment operators and so on. Also test queues of various types: does it handle std::strings? Does it handle non-copyable types?