4
\$\begingroup\$

I have started learning multi-threading using C++ 11 and here I have posted the code for thread-safe FIFO queue for multiple producers and consumers.
Though it works fine but I want to know any suggestions,best practices or even any loop hole if you guys can find it.

#ifndef BOUNDEDBUFFER_H
#define BOUNDEDBUFFER_H

#include <thread>
#include <mutex>
#include <condition_variable>
#include <iostream>

// T must implement operator << 

template< typename T>
class BoundedBuffer
{
public:
    BoundedBuffer(size_t size) :m_size(size), m_front(0), m_rear(-1), m_count(0)
    {
        m_queue = new T[m_size];
    }

    void enqueue( const T& obj )
    {
        std::unique_lock<std::mutex> writerLock(m_mutex);

        while ( m_size == m_count )
        {
            writers.wait(writerLock);
        }
        m_rear = (m_rear + 1) % m_size;
        m_queue[m_rear] = obj;
        ++m_count;
        std::cout << std::this_thread::get_id() << " has Enqueued Object =" << obj << std::endl;
        readers.notify_all();
    }

    void dequeue( T& obj )
    {
        std::unique_lock<std::mutex> readerLock(m_mutex);
        while (0 == m_count)
        {
            readers.wait(readerLock);
        }
        obj = m_queue[m_front];
        m_front = (m_front + 1) % m_size;
        --m_count;
        std::cout << std::this_thread::get_id() << " has Dequeued Object =" << obj << std::endl;
        writers.notify_all();
    }

    ~BoundedBuffer()
    {
        if (m_queue)
        {
            delete[] m_queue;
        }
    }

private:
    size_t m_size;
    T * m_queue;
    int m_front, m_rear, m_count;
    std::mutex m_mutex;
    std::condition_variable readers;
    std::condition_variable writers;
};

#endif
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$
  1. If the allocation in the ctor throws, the dtor will have to contend with an uninitialized owning raw pointer. In other words, you have Undefined Behavior.

    The minimal fix would be value-initializing m_queue before trying to allocate the memory. Moving the allocation into the ctor-init-list would not work, as constructing a std::condition_variable can throw.

    The better fix would be using a std::unique_ptr<T[]>, and maybe allocating the memory with a back-port of C++14 std::make_unique, though assigning in the ctor-body would then also work.

  2. delete [] pointer; and if(pointer) delete [] pointer differ in exactly one point:
    The second is longer.

    Not that you should have any raw owning pointers.

  3. You know remainder (%) is a fairly expensive operation? And the dividend is in the range [0, divisor] inclusive.

  4. You can suppress the notification of writers if the buffer wasn't full before the call to dequeue().

  5. Likewise, you can suppress the notification of readers if the buffer wasn't empty before dequeue().

  6. You should direct debug-output to std::cerr instead. Which would obviate the need for manually flushing with std::endl.

    Of course, the debug-output should be removed from the release version.

  7. Strictly speaking, one of m_front, m_rear is superfluous because you also have m_count.

  8. It's a curious decision to make m_rear always point to the last object you enqueue()d, instead of past it, and thus adjusting it before you actually copy the object. It means that enqueue() depends on copy-assignment of T not throwing for correctness.

  9. Your decision to use different names for the std::unique_lock in enqueue() and dequeue() threw me for a loop. For a moment I thought there was a reason for them, namely two different mutexes. Better change that...

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.