Thread Safe Circular Queue Implementation

Question

#pragma once

#include <atomic>
#include <memory>

template <typename T> class RingBuffer {
public:

    explicit RingBuffer(std::size_t capactiy) : _buffer_capacity(capactiy){
        std::unique_ptr <T[], RingBufferFree> buffer_ptr(new T[capactiy]);
        _ring_buffer_array = std::move(buffer_ptr);
    }

    ~RingBuffer() {

    }

    void Push(T val) {
        while (!TryPush(val));
    }

    T Pop() {
        T val;
        while (!TryPop(&val));
        return val;
    }

private:

    //Private Struct to ensure allocated memory is freed.
    struct RingBufferFree{
        void operator()(T * ring_buffer_memory) { delete[] ring_buffer_memory; }
    };

    //Private Member Functions
    bool TryPush(T & val) {
        const std::size_t current_write = write_position.load(std::memory_order_acquire);
        const std::size_t current_read = read_position.load(std::memory_order_acquire);
        const std::size_t next_write = increment_index(current_write);

        if (next_write == current_read) { return false; }

        _ring_buffer_array[current_write] = val;
        write_position.store(next_write, std::memory_order_release);

        return true;
    }

    bool TryPop(T * val) {
        const std::size_t current_write = write_position.load(std::memory_order_acquire);
        const std::size_t current_read = read_position.load(std::memory_order_acquire);

        if (current_read == current_write) { return false; }

        *val = _ring_buffer_array[current_read];

        const std::size_t next_read = increment_index(current_read);
        read_position.store(next_read, std::memory_order_release);

        return true;
    }

    std::size_t increment_index(std::size_t index) {
        return (index + 1) % _buffer_capacity;
    }

    //Private Member Variables
    std::atomic<std::size_t> read_position = 0;
    std::atomic<std::size_t> write_position = 0;

    std::size_t _buffer_capacity;
    std::unique_ptr<T[], RingBufferFree> _ring_buffer_array;
};

I have tested my code and it appears to work, but I would like some second opinions on what I did. This first time I have tried lock free programming with atomic variables and so I would appreciate it if I had more experienced programmers provide feed back.

Answer 1

First some style notes:

• capactiy should be capacity. Spelling is important in all kinds of writing, because all writing is primarily communication, intended for humans just as much as for computers.

• The entire RingBufferFree widget is unnecessary. std::unique_ptr<T[]> will do the right thing by default.

• Temporary variables are often a code smell, if they're not serving an explanatory purpose. In the constructor, buffer_ptr is unnecessary.

  explicit RingBuffer(std::size_t capacity) :
      _buffer_capacity(capacity),
      _ring_buffer_array(new T[capacity])
  {}
  

• Personally, I'd never write std::size_t, since size_t works just as well; but your mileage may vary.

• ~RingBuffer() {} is unnecessary; remove it.

• It's unclear to me why you're using a std::unique_ptr<T[]> at all, when a std::vector<T> would work just as well.

• It's odd that TryPush takes T& val, instead of const T& val or (better) T val or (perhaps best in this context) T&& val — where you move-assign either of the last two into place with _ring_buffer_array[current_write] = std::move(val);.

• Likewise, in TryPop you probably ought to be moving-from the buffer element: *val = std::move(_ring_buffer_array[current_read]);

Okay, let's go find some concurrency bugs!

---

Obviously you'll have problems if two different threads try to call Push at the same time (because one will overwrite the other's pushed item); and you'll have problems if two different threads try to call Pop at the same time (because both could pop the same item).

So let's assume that this is just supposed to be a single-producer single-consumer queue, and look for bugs in that scenario.

---

Actually, assuming it's only supposed to be single-producer single-consumer, it's pretty clean! The only potential issue I see is that if the user calls Q.Pop() on an empty queue, or Q.Push(x) on a full one, then the thread in question will busy-loop forever — or at least until some other thread manages to break in and get some actual work done. It would be significantly friendlier if you put something in the while loop's body, e.g. std::this_thread::yield() — or even better, just return false or throw QueueEmptyException() or something like that, so that the calling code can decide what it wants to do, instead of getting blocked for an indefinite amount of time.

In other words, TryPush and TryPop smell like the real primitive operations here; Push and Pop are non-primitive operations of dubious utility.