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;
};

• Mathieu Guindon: I have rolled back the last edit. Please see what you may and may not do after receiving answers .

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();

• Surt: Good point about the emplace (emplace_back?). But I don't agree with letting the lock_guard cover the notify as that means the other thread potentially wakes, sees its still locked goes to sleep again, then is re-awakened by the unlock, wasting a lot of cycles.
• sebap123: I am thinking the same way as @Surt regarding the locking. Actually it (unique_lock) has been taken from C++ documentation: en.cppreference.com/w/cpp/thread/condition_variable
• David: @sebap123 you only need that in remove. See updated answer
• David: @sebap123 one way to handle it would be to return a std::optional<T> from remove and have an additional mechanism to tell remove to exit and return nullopt
• sebap123: @David I've read about std::optional but unfortunately this is only since C++17, and since I am using up to C++14 I cannot use it. Is there any other solution here?

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.

• sebap123: You are correct with my FIFO approach. I just forget to change from my previous FILO solution. Thank you for pointing that out.