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

…er (#177)

* reproduce race condition

* double-checked locking to fix race condition

* lock immediately

ringbuffer/ringbuffer.go

@@ -56,11 +56,12 @@ func (rb *RingBuffer[T]) Len() int64 {
 }
 
 func (rb *RingBuffer[T]) Pop() (T, bool) {
-	if rb.Len() == 0 {
+	rb.mu.Lock()
+	if rb.len == 0 {
+		rb.mu.Unlock()
 		var t T
 		return t, false
 	}
-	rb.mu.Lock()
 	rb.content.head = (rb.content.head + 1) % rb.content.mod
 	item := rb.content.items[rb.content.head]
 	var t T
@@ -71,10 +72,11 @@ func (rb *RingBuffer[T]) Pop() (T, bool) {
 }
 
 func (rb *RingBuffer[T]) PopN(n int64) ([]T, bool) {
-	if rb.Len() == 0 {
+	rb.mu.Lock()
+	if rb.len == 0 {
+		rb.mu.Unlock()
 		return nil, false
 	}
-	rb.mu.Lock()
 	content := rb.content
 
 	if n >= rb.len {

ringbuffer/ringbuffer_test.go

@@ -1,6 +1,8 @@
 package ringbuffer
 
 import (
+	"sync"
+	"sync/atomic"
 	"testing"
 )
 
@@ -37,3 +39,57 @@ func TestPushPopN(t *testing.T) {
 		}
 	}
 }
+
+func TestPopThreadSafety(t *testing.T) {
+	t.Run("Pop should be thread-safe", func(t *testing.T) {
+		testCase := func() {
+			rb := New[int](4)
+			rb.Push(1)
+			wg := sync.WaitGroup{}
+			for i := 0; i < 2; i++ {
+				wg.Add(1)
+				go func() {
+					defer wg.Done()
+					rb.Pop()
+				}()
+			}
+			wg.Wait()
+			if rb.Len() == -1 {
+				t.Fatal("item popped twice")
+			}
+		}
+
+		// Increase the number of iterations to raise the likelihood of reproducing the race condition
+		for i := 0; i < 100_000; i++ {
+			testCase()
+		}
+	})
+
+	t.Run("PopN should be thread-safe", func(t *testing.T) {
+		testCase := func() {
+			rb := New[int](4)
+			rb.Push(1)
+			counter := atomic.Int32{}
+			wg := sync.WaitGroup{}
+			for i := 0; i < 2; i++ {
+				wg.Add(1)
+				go func() {
+					defer wg.Done()
+					_, ok := rb.PopN(1)
+					if ok {
+						counter.Add(1)
+					}
+				}()
+			}
+			wg.Wait()
+			if counter.Load() > 1 {
+				t.Fatal("false positive item removal")
+			}
+		}
+
+		// Increase the number of iterations to raise the likelihood of reproducing the race condition
+		for i := 0; i < 100_000; i++ {
+			testCase()
+		}
+	})
+}