Producer/Consumer implementation using condition_variable

Question

Trying to implement producer/consumer problem in C++. I have one question: should I use nofify_all or notify_one method here? Also any feedback is welcome

template <class T>
class Stack
{
public:
    Stack(size_t size) : size_(size)
    {
        arr_ = new T[size_];
        init();
    }

    ~Stack()
    {
        delete[] arr_;
    }

    void init()
    {
        top_ = -1;
    }

    int capacity()
    {
        return static_cast<int>(size_) - top_ - 1;
    }

    void set(T num)
    {
        std::unique_lock<std::mutex> locker(mu);    

        cond.wait(locker, [this](){ return top_ < static_cast<int>(size_) - 1; });
        top_++;
        arr_[top_] = num;
        std::cout << "Setting num: " << num << " Capacity: " << this->capacity() << std::endl;
        locker.unlock();
        cond.notify_one();
    }

    Т get()
    {
        std::unique_lock<std::mutex> locker(mu);
        T top_value;

        cond.wait(locker, [this](){ return top_ != -1; });
        top_value = arr_[top_];
        top_--;
        std::cout << "Getting num: " << top_value << " Capacity: " << this->capacity() << std::endl;
        locker.unlock();
        cond.notify_one();

        return top_value;
    }

private:
    size_t size_;
    T* arr_;
    int top_;
    std::mutex mu;
    std::condition_variable cond;
};

main:

int main()
{
    Stack<int> stack(10);
    std::vector<std::thread> producers;
    std::vector<std::thread> consumers;

    for (int i = 0; i <= 20; i++)
    {
        consumers.push_back(std::thread([&](){
            stack.get();
        }));
    }

    for (int i = 1; i <= 20; i++)
    {
        producers.push_back(std::thread([&](){
            stack.set(i);
        }));
    }

    for (auto& th : producers)
    {
        th.join();
    }

    for (auto& th : consumers)
    {
        th.join();
    }

    return 0;
}

Answer 1

A couple of comments on your test-harness code:

for (int i = 0; i <= 20; i++)
// ...
for (int i = 1; i <= 20; i++)

You've got 21 consumers and only 20 producers. This means your test code will never terminate, right?

You can avoid off-by-one errors like this by getting comfortable with the "half-open range" idiom of C-style languages. Every for-loop you write in C, C++, Java, C#, JavaScript, Perl, or whatever, should look like this:

for (int i = 0; i < N; ++i)

Notice that it starts at 0 (inclusive) and goes up to N (exclusive), so it iterates exactly N times.

---

    consumers.push_back(std::thread([&](){
        stack.get();
    }));

is a verbose way of writing

    consumers.emplace_back([&](){ stack.get(); });

---

I see you writing

arr_ = new T[size_];

— that's a red flag. Prefer to use std::vector<T> or std::unique_ptr<T[]> instead of raw pointers. The reason is that those will behave properly on copy-construction and/or move-construction, whereas your current pointer-based code requires some extra code in the special member functions (constructors and assignment operators) in order to work correctly.

Look up the Rule of Zero and try to apply it here.

Since you're relying on the destructor to delete[] the array allocated in your constructor, you should =delete the copy constructor and copy assignment operator; otherwise you're allowing the client code to make shallow copies that will lead to use-after-free and double-free bugs.

But! Your current code does actually result in an implicitly deleted copy constructor, because the default definition would have been ill-formed, because you can't copy-construct a std::condition_variable. So you're okay, modulo some really verbose error messages and the danger that someone might come along and refactor your class to remove the uncopyable members.

---

Shouldn't a Stack be copyable? or at least movable? Right now your Stack is neither copyable nor movable, because of its std::condition_variable and std::mutex members.

---

The names set and get (for push and pop respectively) are ill-chosen.

The name capacity for space_left is also ill-chosen. The word capacity already has a meaning in C++: it means "maximum possible size", as in std::vector::capacity. When you're trying to find a name for a thing, choose the name that best represents the thing itself — usually this is easy because most things already have established names (e.g. stack, push, pop). When you need a name for a thing that really doesn't yet have a name, then you just need to avoid naming it the same as something else that's already in the lexicon (e.g. capacity).

---

capacity needs to be a private member function, or else it needs to take the mutex mu, because otherwise the client code could call x->capacity() at the same time as some other thread is calling x->get() and suddenly you've got a race condition.

However, if you simply slap a mutex lock around the body of capacity, you'll find that set and get both deadlock, because each of them calls capacity while the mutex mu is already (still) locked.

std::cout << "Getting num: " << top_value << " Capacity: " << this->capacity() << std::endl;
locker.unlock();

Calling such expensive I/O routines under a mutex is a bad habit anyway. You could at least improve the code slightly by rephrasing it as

int cap = this->capacity();
locker.unlock();
std::cout << "Getting num: " << top_value << " Remaining capacity: " << cap << std::endl;

---

Consider what happens when T is not default-constructible; consider what happens when T is default-constructible but its default constructor is relatively expensive. What do you want to have happen in those cases?

Consider what happens when T is move-only. What do you want to have happen in that case?

---

Consider what happens when the following line in set throws:

top_++;
arr_[top_] = num;  // T::operator= THROWS AN EXCEPTION

What happens the next time you get from the stack? What do you want to have happen in that case?

---

But basically, this seems like reasonably good code (modulo the wrong names for things like push and pop, and the for-loops).

Your use of mutexes and condition variables seems fine to me. notify_one will probably be more efficient than notify_all, but either one is fine.