Bugfix // Fix race condition in Pop & PopN operation of ring buffer

[mapogolions]

The current implementation of the ring buffer has a thread-safety issue in the Pop & PopN operation.

Attempting to remove an element may lead to a race condition, resulting in the ring buffer being left in an invalid state after calling Pop. Similarly, in the case of PopN, the method might incorrectly indicate that elements were removed, even though no elements were actually removed because the ring buffer was empty.

To illustrate the problem, I have added unit tests that can reproduce the issue. Feel free to delete these tests once you have verified the existence of the bug, as including such tests might be questionable, given that they expose a non-deterministic problem.

[tprifti]

we check for the ringbuffer length twice here, 1st time using rb.Len(), and second time using rb.len

func (rb *RingBuffer[T]) Pop() (T, bool) {
	rb.mu.Lock() // lock here to avoid reading length 2 times
	if rb.Len() == 0 {
		rb.mu.Unlock()
		var t T
		return t, false
	}
	rb.content.head = (rb.content.head + 1) % rb.content.mod
	item := rb.content.items[rb.content.head]
	var t T
	rb.content.items[rb.content.head] = t
	atomic.AddInt64(&rb.len, -1)
	rb.mu.Unlock()
	return item, true
}

[mapogolions]

Yes, it's called double-checked locking pattern. There are usually two options we can use:

1. Lock immediately:

rb.mu.Lock()
if rb.len == 0 {
	rb.mu.Unlock()
	var t T
	return t, false
}

2. Use double-checked locking:

if rb.Len() == 0 { // As far as I understand, we read atomically to prevent tearing read of int64.
	var t T
	return t, false
}
rb.mu.Lock()
if rb.len == 0 {
	rb.mu.Unlock()
	var t T
	return t, false
}

[tprifti]

Personally I would prefer the first option, since it's bit more readable and we do not need to check the rb length twice.

Good job for detecting the issue!

[tprifti]

same thing here:

func (rb *RingBuffer[T]) PopN(n int64) ([]T, bool) {
	rb.mu.Lock()
	if rb.Len() == 0 {
		rb.mu.Unlock()
		return nil, false
	}
	content := rb.content
	if n >= rb.len {
		n = rb.len
	}
	atomic.AddInt64(&rb.len, -n)

	items := make([]T, n)
	for i := int64(0); i < n; i++ {
		pos := (content.head + 1 + i) % content.mod
		items[i] = content.items[pos]
		var t T
		content.items[pos] = t
	}
	content.head = (content.head + n) % content.mod

	rb.mu.Unlock()
	return items, true
}

[tprifti]

@mapogolions I just checked the updates. It's better to use atomic operation .Len() for reading length instead of just .len, because we need to make sure it's not written by other threads while we read it

[mapogolions]

@tprifti
req.len is read after acquiring the lock, which ensures exclusive access

[tprifti]

@tprifti req.len is read after acquiring the lock, which ensures exclusive access

You are technically correct. I suggested using atomic read so we can be consistent across the repo. Also, it would be helpful for other contributors to notice that reading the length of ringbuffer needs to be atomic.