Threadsafe circular buffer for storing pointers

Question

I am currently working on a threadsafe circular buffer for storing pointers. As basis I used the code from this thread: Thread safe implementation of circular buffer. My code is shown below.

Because I want to store pointers in the circular buffer, I need to make sure that allocated memory is deleted, in case the buffer is full and the first element is overwritten, as mentioned in the boost documentation:

When a circular_buffer becomes full, further insertion will overwrite the stored pointers - resulting in a memory leak. 1

So I tried to delete the first element in the add method, in case the buffer is full and the type T of the template is actually a pointer. This leads to a C2541-error, because I try to delete an object, which is not seen as a pointer.

Is my basic approach correct? How can I solve the above issue?

#pragma once

#include <boost/thread/condition.hpp>
#include <boost/thread/mutex.hpp>
#include <boost/thread/thread.hpp>
#include <boost/circular_buffer.hpp>
#include <type_traits>
#include "Settings.hpp"


template <typename T>
class thread_safe_circular_buffer : private boost::noncopyable
{
public:
    typedef boost::mutex::scoped_lock lock;
    thread_safe_circular_buffer(bool *stop) : stop(stop) {}
    thread_safe_circular_buffer(int n, bool *stop) : stop(stop) {cb.set_capacity(n);}
    void add (T imdata) {
        monitor.lock();
        std::cout << "Buffer - Add Enter, Size: " << cb.size() << "\n";

        if(cb.full())
        {
          std::cout << "Buffer - Add Full.\n";
          T temp = cb.front();

          if(std::is_pointer<T>::value)
            delete[] temp;
        }

        std::cout << "Buffer - Push.\n";
        cb.push_back(imdata);
        monitor.unlock();
        std::cout << "Buffer - Add Exit.\n";
    }
    T get() {
        std::cout << "Buffer - Get Enter, Size: " << cb.size() << "\n";
        monitor.lock();
        while (cb.empty())
        {
        std::cout << "Buffer - Get Empty, Size: " << cb.size() << "\n";
          monitor.unlock();
          std::this_thread::sleep_for(std::chrono::milliseconds(1000));

          if(*stop)
            return NULL;

          monitor.lock();
        }

        T imdata = cb.front();
        // Remove first element of buffer
        std::cout << "Buffer - Pop.\n";
        cb.pop_front();
        monitor.unlock();

        std::cout << "Buffer - Get Exit.\n";
        return imdata;
    }
    void clear() {
        lock lk(monitor);
        cb.clear();
    }
    int size() {
        lock lk(monitor);
        return cb.size();
    }
    void set_capacity(int capacity) {
        lock lk(monitor);
        cb.set_capacity(capacity);
    }

    bool *stop;
private:
    boost::condition buffer_not_empty;
    boost::mutex monitor;
    boost::circular_buffer<T> cb;
};

also I don't understand why you delete `temp` as an array (`delete[] temp;`) when it's `T temp;` — Andriy Tylychko, Jun 17 '15 at 14:22
This is a class template. It depends on the type paramter if T is a pointer type. So the type of `T Temp` is not clear. — harper, Jun 17 '15 at 14:24

score 2 · Accepted Answer · 2015-06-17T15:51:33.180

The error tells you the problem: you can't delete things that aren't pointers. When T isn't a pointer type, delete[] temp; doesn't make sense. It's also a bad idea if your buffer is storing things that weren't allocated with new[], or when your circular buffer doesn't conceptually 'own' the pointers.

I think you maybe misunderstand the whole problem. The warning from the boost documentation only applies to situations where you can't afford to "lose" any of the data stored in the buffer. One example where this is a problem — the one they highlight specifically — is if you were storing raw pointers in the buffer that are your only references to some dynamically allocated memory.

There are, I think, only three reasonable designs:

Don't use a circular buffer when you can't afford to lose data (e.g. this can mean modifying your data so you can afford to lose it; e.g. circular_buffer<unique_ptr<T[]>> for storing dynamically allocated arrays) . And consequently, make your class not worry about what to do with lost data.
Make your class take a 'deleter'; i.e. a function object that specifies what to do with a data element that is about to be overwritten. (and you probably want to have a default parameter of "do nothing")
Change the functionality of the buffer to do something other than overwriting when full (e.g. block)

score 1 · Answer 2 · answered Jun 17 '15 at 14:24

Do as boost does: let the user of your code handle this. If objects are stored, its destructor should handle possible memory management, if pointers are stored, you have no way of knowing what it actually points to: arrays, objects, memory that needs to be freed, memory that is managed elsewhere, not dynamic memory.

Threadsafe circular buffer for storing pointers

2 Answers2