Added a Clear method that resets the read and write indexes.

[perencia-wc]

No description provided.

[DNedic]

Thanks for the contribution! One issue with adding a Clear() method like this is that it is not thread safe.

We can solve this by either:

1. Adding a note to the javadoc comment
2. Creating separate ClearFromConsumer(), ClearFromProducer() methods

I am not a fan of either approach tbh, as you can also create a new Ring Buffer over the old one.
On the other hand I also want the library to be convenient for the people actually using it, so we can go with either of these.

[DNedic]

I would also add a test for writing to the buffer after clearing.

[perencia-wc]

I am not a fan of either approach tbh, as you can also create a new Ring Buffer over the old one. On the other hand I also want the library to be convenient for the people actually using it, so we can go with either of these.

I really don't like the first option, and regarding the second I'm not sure to understand why do we need two methods. I'm thinking in something like

template<typename T, size_t size>
void RingBuf<T, size>::Clear() {
    _r.store(0, std::memory_order_release);
    _w.store(0, std::memory_order_release);
}

wouldn't that be thread safe?

[DNedic]

This operation cannot be thread safe, as it changes both the read and write index. A SPSC queue based data structure works with the assumption that the write index will only ever be changed by the producer and the read index only ever changed by the consumer.

A store with the release memory ordering pairs with a load of the same variable with acquire memory ordering in another thread and they ensure that everything that happens before the release store is visible before the acquire load.

With this in mind, let's take a look at a scenario where the consumer is clearing the buffer, while the producer is writing for the Queue:

template <typename T, size_t size> bool Queue<T, size>::Push(const T &element) {
    /*
       The full check needs to be performed using the next write index not to
       miss the case when the read index wrapped and write index is at the end
     */
    const size_t w = _w.load(std::memory_order_relaxed);
    size_t w_next = w + 1;
    if (w_next == size) {
        w_next = 0U;
    }

    /* Full check  */
    const size_t r = _r.load(std::memory_order_acquire);
    if (w_next == r) {
        return false;
    }

    /* Place the element */
    _data[w] = element;

    /* Store the next write index */
    _w.store(w_next, std::memory_order_release);
    return true;
}

The load and store on _r ensure that this part will be visible (happens before) to the writer trying to clear the buffer:

 const size_t w = _w.load(std::memory_order_relaxed);
    size_t w_next = w + 1;
    if (w_next == size) {
        w_next = 0U;
    }

As this is the only guarantee that we get, it can happen that the writer will think that _w is a completely different value from 0 we are setting in the consumer and later down the line we will replace it with w_next.

Even if we were to replace const size_t w = _w.load(std::memory_order_relaxed); with const size_t w = _w.load(std::memory_order_acquire);, this is not going to work as within the producer and the consumer the ordering of modifications to the indexes is going to be the same, but it would need to be different (and I'm not sure it would work even then, would need to check).

[DNedic]

As for the second approach, it would be thread safe because the consumer could change the read index to match the write index, whatever it is, and vice versa for the producer.

[perencia-wc]

Thanks for your detailed explanation.
I think I understand the problem, though not the solution 😅.
Thinking it twice, a Clear method is not really necessary, it could be replaced with a kind of "Flush" skipping any remaining items until _r == _w, though this can require to walk all the buffer. But anyway I think some kind of clearing can be convenient, i.e as is our case, when the holder of the buffer is neither the consumer nor the producer, and some event requires the buffer to be cleared/flushed. As I can't provide a reliable solution (at least not until I fully understand it), I think it is better to let you decide if that feature of clearing/flush actually fits in the project and how to implement it 😄

[DNedic]

After analyzing this a bit I've found that even _r = _w, or doing Read() in a loop might not be enough, because we can't know if there is a write in progress and, _w will change immedeately after.

The reason I think this is a problem is because usually you want to flush when some assumption has changed and buffered data might not be valid anymore.

[DNedic]

A possible solution would be to use atomic flags for signalling from the consumer to the producer and vice versa. This would add overhead for every single operation though.

[perencia-wc]

Thanks for the contribution! One issue with adding a Clear() method like this is that it is not thread safe.

We can solve this by either:

1. Adding a note to the javadoc comment
2. Creating separate ClearFromConsumer(), ClearFromProducer() methods

[DNedic]

I would preffer for the PR to stay open for a while to have time to perhaps get comments from more users, and for me to think about this some more.