HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Multi-thread safe buffer in C++

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
bufferthreadmultisafe

Problem

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 
#include 
#include 
#include 

template 
class Buffer
{
public:
    void add(T num)
    {
        while (true)
        {
            std::unique_lock locker(mu);
            buffer.push_back(num);
            locker.unlock();
            cond.notify_all();
            return;
        }
    }
    T remove()
    {
        while (true)
        {
            std::unique_lock 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 locker(mu);
        int s = buffer.size();
        locker.unlock();
        return s;
    }
private:
    std::mutex mu;
    std::condition_variable cond;

    std::deque buffer;
};

Solution

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 lock{mu}; return buffer.size();

Context

StackExchange Code Review Q#151881, answer score: 6

Revisions (0)

No revisions yet.