Multi-thread safe buffer in C++

Question

I am trying to design thread-safe data structure that I cna use as a buffer in my application. Can you please give me comments about this code and what can be improved:

#include <deque>
#include <thread>
#include <mutex>
#include <condition_variable>

template <typename T>
class Buffer
{
public:
    void add(T num)
    {
        while (true)
        {
            std::unique_lock<std::mutex> locker(mu);
            buffer.push_back(num);
            locker.unlock();
            cond.notify_all();
            return;
        }
    }
    T remove()
    {
        while (true)
        {
            std::unique_lock<std::mutex> locker(mu);
            cond.wait(locker, [this](){return buffer.size() > 0;});
            T back = buffer.back();
            buffer.pop_back();
            locker.unlock();
            cond.notify_all();
            return back;
        }
    }
    int size()
    {
        std::unique_lock<std::mutex> locker(mu);
        int s = buffer.size();
        locker.unlock();
        return s;
    }
private:
    std::mutex mu;
    std::condition_variable cond;

    std::deque<T> buffer;
};

Answer 1

Problems with add:

• the while(true) is useless
• you want to notify_one, not notify_all because you've only added one thing, and so only one thread waiting to remove can remove it.
• you should overload for two version which take const T& and T&& (the latter would std::move). This would be optimal in all cases. (analogous to push_back in standard containers)
• You may also want to add an emplace to complement the add() versions so you can construct T in-place at the top of the Buffer.
• you can use lock_guard instead of unique_lock and scope it with {}s. I'd just say always prefer the lighter weight construct when a heavier weight one isn't strictly needed.

problems with remove:

• the while (true) is useless
• the cond var lambda can be return !buffer.empty();
• the back variable can be std::moved
• what happens if you destruct an empty Buffer while a thread is waiting on remove? That case isn't handled at all, and it's a problem
• there's no reason to notify_all in remove. Who is it notifying and why?

problems with size:

• use a lock_guard unless unique_lock is needed
• just do: std::lock_guard<std::mutex> lock{mu}; return buffer.size();

Answer 2

You should use std::stack if you want to push and pop from the back, using FILO. If you mean to use FIFO std:deque is the correct choice as it is optimized for back and front access.

As already mentioned the while is useless and notify_all is probably wrong.

The placement of the unlock is good, so that other threads don't wake, see they are still locked, then when you unlock they are awakened again, wasting lots of cycles.

Using unique_lock is an OK choice as it is needed for the condition_variable anyway.