From ab7ea15f8a930de862a07cc1cf3e24aeb7bb0d4f Mon Sep 17 00:00:00 2001 From: Jakob Ersson <11717405+jakobilobi@users.noreply.github.com> Date: Thu, 27 Nov 2025 14:11:12 +0100 Subject: [PATCH 1/7] docs: bugfix instructions --- improvement_proposals.md | 61 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) create mode 100644 improvement_proposals.md diff --git a/improvement_proposals.md b/improvement_proposals.md new file mode 100644 index 0000000..6dd2d03 --- /dev/null +++ b/improvement_proposals.md @@ -0,0 +1,61 @@ +Thread-safe Set and Queue Fixes (PRD) + +## 1) RWMutexSet zero-value panic + +- **Problem**: `RWMutexSet` embeds an uninitialized `map[T]struct{}`. Calling `Add` on a zero-value set (e.g., `var s threadsafe.RWMutexSet[int]`) writes to a nil map and panics. +- **Impact**: Violates the repository pattern where most types are safe at zero value (e.g., `RWMutexQueue`), surprises callers, and can crash services if a zero value escapes DI/init code. +- **Repro**: + ```go + var s threadsafe.RWMutexSet[int] + s.Add(1) // panic: assignment to entry in nil map + ``` +- **Proposed fix**: lazily initialize `items` on first mutation so the zero value is usable. No API change. + ```go + // Add stores an item in the set. + func (s *RWMutexSet[T]) Add(item T) (added bool) { + s.mu.Lock() + if s.items == nil { // allow zero-value usage + s.items = make(map[T]struct{}) + } + if _, exists := s.items[item]; !exists { + s.items[item] = struct{}{} + s.size++ + s.mu.Unlock() + return true + } + s.mu.Unlock() + return false + } + ``` +- **Notes**: No behavioral change for existing constructor users. Optional follow-up: add a doc comment stating zero-value is ready to use. + +## 2) RWMutexQueue Range concurrency race + +- **Problem**: `Range` takes a slice view under `RLock`, unlocks, then iterates. Concurrent `Push`/`Pop` under `Lock` mutate the same backing array, so `Range` reads without synchronization. +- **Impact**: Data race in concurrent scenarios; possible stale or corrupted reads. Violates the “All operations must be safe for concurrent use” contract of `Queue`. +- **Repro (conceptual)**: + ```go + q := &threadsafe.RWMutexQueue[int]{} + var wg sync.WaitGroup + wg.Add(2) + go func() { defer wg.Done(); q.Range(func(int) bool { time.Sleep(time.Microsecond); return true }) }() + go func() { defer wg.Done(); q.Push(1); q.Pop() }() + wg.Wait() // race detector flags unsynchronized access + ``` +- **Proposed fix**: Snapshot under lock (matching `All`) or keep the lock during iteration. Snapshot keeps callers’ callback isolated from locks. + ```go + // Range calls f sequentially for each item from front to back. + func (q *RWMutexQueue[T]) Range(f func(item T) bool) { + q.mu.RLock() + snapshot := make([]T, len(q.items)-q.head) + copy(snapshot, q.items[q.head:]) + q.mu.RUnlock() + + for _, it := range snapshot { + if !f(it) { + break + } + } + } + ``` +- **Notes**: This aligns `Range` with `All`, preserves lock-free callback execution, and removes the race. Performance impact is minimal because a copy already exists in `All` and the queue’s size is bounded by workload. From aa907b1cf20bafc5ca776f2aa36dc2024f21e14a Mon Sep 17 00:00:00 2001 From: Jakob Ersson <11717405+jakobilobi@users.noreply.github.com> Date: Mon, 1 Dec 2025 09:33:46 +0100 Subject: [PATCH 2/7] fix: two RWMutex* inconsistencies --- queue_rwmutex.go | 5 +++-- set_rwmutex.go | 8 ++++++++ 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/queue_rwmutex.go b/queue_rwmutex.go index 73f2a67..edc26ad 100644 --- a/queue_rwmutex.go +++ b/queue_rwmutex.go @@ -100,10 +100,11 @@ func (q *RWMutexQueue[T]) Slice() []T { // the queue or its items. func (q *RWMutexQueue[T]) Range(f func(item T) bool) { q.mu.RLock() - items := q.items[q.head:] + snapshot := make([]T, len(q.items)-q.head) + copy(snapshot, q.items[q.head:]) q.mu.RUnlock() - for _, it := range items { + for _, it := range snapshot { if !f(it) { break } diff --git a/set_rwmutex.go b/set_rwmutex.go index 79d5056..7df05cc 100644 --- a/set_rwmutex.go +++ b/set_rwmutex.go @@ -18,6 +18,10 @@ func (s *RWMutexSet[T]) Add(item T) (added bool) { s.mu.Lock() defer s.mu.Unlock() + if s.items == nil { + s.items = make(map[T]struct{}) + } + if _, exists := s.items[item]; !exists { s.items[item] = struct{}{} s.size++ @@ -31,6 +35,10 @@ func (s *RWMutexSet[T]) Delete(item T) (removed bool) { s.mu.Lock() defer s.mu.Unlock() + if s.items == nil { + return false + } + if _, exists := s.items[item]; exists { delete(s.items, item) s.size-- From e56dc64ff2b67f09bb6ef15fc3161bfcb1807f06 Mon Sep 17 00:00:00 2001 From: Jakob Ersson <11717405+jakobilobi@users.noreply.github.com> Date: Mon, 1 Dec 2025 09:41:09 +0100 Subject: [PATCH 3/7] test: add RWMutexSet zero value test and Queue concurrent range test --- queue_test.go | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++ set_test.go | 45 ++++++++++++++++++++++++++++++++++ 2 files changed, 112 insertions(+) diff --git a/queue_test.go b/queue_test.go index 9efca4d..676bad4 100644 --- a/queue_test.go +++ b/queue_test.go @@ -123,10 +123,32 @@ func (s *queueTestSuite[T]) TestAllIterator(t *testing.T) { assert.Equal(t, 4, q.Len()) } +func (s *queueTestSuite[T]) TestRangeSnapshot(t *testing.T) { + q := s.newQueue() + q.Push(s.item1, s.item2, s.item3) + + // Range should provide a snapshot - mutations during iteration shouldn't affect what we see + var observed []T + q.Range(func(item T) bool { + observed = append(observed, item) + // Mutate the queue during iteration + if len(observed) == 1 { + q.Push(s.item1) // Add a duplicate + } + return true + }) + + // Should only observe the original 3 items (snapshot behavior) + assert.Equal(t, []T{s.item1, s.item2, s.item3}, observed) + // But the queue should now have 4 items + assert.Equal(t, 4, q.Len()) +} + func runQueueTestSuite[T any](t *testing.T, s *queueTestSuite[T]) { t.Run("BasicOperations", s.TestBasicOperations) t.Run("Slice", s.TestSlice) t.Run("Range", s.TestRange) + t.Run("RangeSnapshot", s.TestRangeSnapshot) t.Run("AllIterator", s.TestAllIterator) } @@ -208,3 +230,48 @@ func TestQueueConcurrentAccess(t *testing.T) { q := NewRWMutexQueue[string]() testConcurrentQueueAccess(t, q) } + +func TestQueueConcurrentRange(t *testing.T) { + q := NewRWMutexQueue[int]() + + // Pre-populate the queue + for i := 0; i < 100; i++ { + q.Push(i) + } + + var wg sync.WaitGroup + wg.Add(3) + + // Goroutine 1: Concurrent Range calls + go func() { + defer wg.Done() + for i := 0; i < 20; i++ { + count := 0 + q.Range(func(int) bool { + count++ + return true + }) + // Verify we got some items (exact count may vary due to concurrent mutations) + assert.Greater(t, count, 0) + } + }() + + // Goroutine 2: Concurrent Push operations + go func() { + defer wg.Done() + for i := 0; i < 100; i++ { + q.Push(i + 1000) + } + }() + + // Goroutine 3: Concurrent Pop operations + go func() { + defer wg.Done() + for i := 0; i < 50; i++ { + q.Pop() + } + }() + + wg.Wait() + // Test should complete without data races +} diff --git a/set_test.go b/set_test.go index 75910fe..a9cbd95 100644 --- a/set_test.go +++ b/set_test.go @@ -372,6 +372,51 @@ func TestSetConcurrentRemoval(t *testing.T) { } } +func TestRWMutexSetZeroValue(t *testing.T) { + // Test that RWMutexSet can be used at zero value without initialization + var s RWMutexSet[int] + + // Add on zero-value should initialize map + assert.True(t, s.Add(1)) + assert.True(t, s.Add(2)) + assert.Equal(t, 2, s.Len()) + + // Verify items exist + assert.True(t, s.Has(1)) + assert.True(t, s.Has(2)) + assert.False(t, s.Has(3)) + + // Delete existing item + assert.True(t, s.Delete(1)) + assert.Equal(t, 1, s.Len()) + assert.False(t, s.Has(1)) + + // Delete non-existent item + assert.False(t, s.Delete(99)) + + // Test zero-value with Delete before Add + var s2 RWMutexSet[string] + assert.False(t, s2.Delete("nonexistent")) + assert.Equal(t, 0, s2.Len()) + + // Then Add should still work + assert.True(t, s2.Add("test")) + assert.Equal(t, 1, s2.Len()) + + // Test read operations on zero-value + var s3 RWMutexSet[string] + assert.False(t, s3.Has("anything")) + assert.Equal(t, 0, s3.Len()) + assert.Empty(t, s3.Slice()) + + visited := 0 + s3.Range(func(string) bool { + visited++ + return true + }) + assert.Equal(t, 0, visited) +} + // // BENCHMARKS // From 2edd6759ee091ef7393a07a8296ebe21832f76c4 Mon Sep 17 00:00:00 2001 From: Jakob Ersson <11717405+jakobilobi@users.noreply.github.com> Date: Mon, 1 Dec 2025 09:56:17 +0100 Subject: [PATCH 4/7] fix: zero value errors for several types --- map_mutex.go | 21 +++++++++++++ map_rwmutex.go | 21 +++++++++++++ map_test.go | 81 ++++++++++++++++++++++++++++++++++++++++++++++++ queue_test.go | 47 +++++++++++++++++++++++----- slice_sharded.go | 12 +++++++ slice_test.go | 71 ++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 246 insertions(+), 7 deletions(-) diff --git a/map_mutex.go b/map_mutex.go index b463725..0c56d01 100644 --- a/map_mutex.go +++ b/map_mutex.go @@ -29,6 +29,9 @@ func (m *MutexMap[K, V]) Set(key K, value V) { m.mu.Lock() defer m.mu.Unlock() + if m.values == nil { + m.values = make(map[K]V) + } m.values[key] = value } @@ -37,6 +40,9 @@ func (m *MutexMap[K, V]) Delete(key K) { m.mu.Lock() defer m.mu.Unlock() + if m.values == nil { + return + } delete(m.values, key) } @@ -62,6 +68,10 @@ func (m *MutexMap[K, V]) CompareAndSwap(key K, oldValue, newValue V) bool { m.mu.Lock() defer m.mu.Unlock() + if m.values == nil { + return false + } + current, exists := m.values[key] if !exists { // Handle case where key doesn't exist @@ -84,6 +94,10 @@ func (m *MutexMap[K, V]) Swap(key K, value V) (V, bool) { m.mu.Lock() defer m.mu.Unlock() + if m.values == nil { + m.values = make(map[K]V) + } + oldValue, loaded := m.values[key] m.values[key] = value if !loaded { @@ -99,6 +113,10 @@ func (m *MutexMap[K, V]) LoadOrStore(key K, value V) (V, bool) { m.mu.Lock() defer m.mu.Unlock() + if m.values == nil { + m.values = make(map[K]V) + } + if v, ok := m.values[key]; ok { return v, true } @@ -150,6 +168,9 @@ func (m *MutexMap[K, V]) SetMany(entries map[K]V) { m.mu.Lock() defer m.mu.Unlock() + if m.values == nil { + m.values = make(map[K]V) + } maps.Insert(m.values, maps.All(entries)) } diff --git a/map_rwmutex.go b/map_rwmutex.go index 23bf204..769f829 100644 --- a/map_rwmutex.go +++ b/map_rwmutex.go @@ -29,6 +29,9 @@ func (m *RWMutexMap[K, V]) Set(key K, value V) { m.mu.Lock() defer m.mu.Unlock() + if m.values == nil { + m.values = make(map[K]V) + } m.values[key] = value } @@ -37,6 +40,9 @@ func (m *RWMutexMap[K, V]) Delete(key K) { m.mu.Lock() defer m.mu.Unlock() + if m.values == nil { + return + } delete(m.values, key) } @@ -62,6 +68,10 @@ func (m *RWMutexMap[K, V]) CompareAndSwap(key K, oldValue, newValue V) bool { m.mu.Lock() defer m.mu.Unlock() + if m.values == nil { + return false + } + current, exists := m.values[key] if !exists { // Handle case where key doesn't exist @@ -84,6 +94,10 @@ func (m *RWMutexMap[K, V]) Swap(key K, value V) (V, bool) { m.mu.Lock() defer m.mu.Unlock() + if m.values == nil { + m.values = make(map[K]V) + } + oldValue, loaded := m.values[key] m.values[key] = value if !loaded { @@ -99,6 +113,10 @@ func (m *RWMutexMap[K, V]) LoadOrStore(key K, value V) (V, bool) { m.mu.Lock() defer m.mu.Unlock() + if m.values == nil { + m.values = make(map[K]V) + } + if v, ok := m.values[key]; ok { return v, true } @@ -150,6 +168,9 @@ func (m *RWMutexMap[K, V]) SetMany(entries map[K]V) { m.mu.Lock() defer m.mu.Unlock() + if m.values == nil { + m.values = make(map[K]V) + } maps.Insert(m.values, maps.All(entries)) } diff --git a/map_test.go b/map_test.go index 3f192dd..87a419e 100644 --- a/map_test.go +++ b/map_test.go @@ -520,6 +520,87 @@ func TestConcurrentAccess(t *testing.T) { } } +func TestMapZeroValue(t *testing.T) { + t.Run("RWMutexMap", func(t *testing.T) { + var m RWMutexMap[string, int] + + // Set on zero-value should initialize map + m.Set("key1", 1) + m.Set("key2", 2) + assert.Equal(t, 2, m.Len()) + + // Get should work + val, ok := m.Get("key1") + assert.True(t, ok) + assert.Equal(t, 1, val) + + // Delete should work + m.Delete("key1") + assert.Equal(t, 1, m.Len()) + + // Read operations on zero-value + var m2 RWMutexMap[int, string] + _, ok = m2.Get(999) + assert.False(t, ok) + assert.Equal(t, 0, m2.Len()) + + // Delete on zero-value with nil map + var m3 RWMutexMap[string, int] + m3.Delete("anything") // Should not panic + assert.Equal(t, 0, m3.Len()) + }) + + t.Run("MutexMap", func(t *testing.T) { + var m MutexMap[string, int] + + // Set on zero-value should initialize map + m.Set("key1", 1) + m.Set("key2", 2) + assert.Equal(t, 2, m.Len()) + + // Get should work + val, ok := m.Get("key1") + assert.True(t, ok) + assert.Equal(t, 1, val) + + // Delete should work + m.Delete("key1") + assert.Equal(t, 1, m.Len()) + + // Read operations on zero-value + var m2 MutexMap[int, string] + _, ok = m2.Get(999) + assert.False(t, ok) + assert.Equal(t, 0, m2.Len()) + + // Delete on zero-value with nil map + var m3 MutexMap[string, int] + m3.Delete("anything") // Should not panic + assert.Equal(t, 0, m3.Len()) + }) + + t.Run("SyncMap", func(t *testing.T) { + // SyncMap is already zero-value safe (sync.Map is zero-value safe) + var m SyncMap[string, int] + + // Set on zero-value + m.Set("key1", 1) + m.Set("key2", 2) + assert.Equal(t, 2, m.Len()) + + // Get should work + val, ok := m.Get("key1") + assert.True(t, ok) + assert.Equal(t, 1, val) + + // Read operations work on zero-value + var m2 SyncMap[int, string] + _, ok = m2.Get(999) + assert.False(t, ok) + assert.Equal(t, 0, m2.Len()) + }) +} + // // BENCHMARKS // diff --git a/queue_test.go b/queue_test.go index 676bad4..24730d6 100644 --- a/queue_test.go +++ b/queue_test.go @@ -202,10 +202,10 @@ func testConcurrentQueueAccess(t *testing.T, q Queue[string]) { // Concurrent enqueues wg.Add(goroutines) - for i := 0; i < goroutines; i++ { + for i := range goroutines { go func(id int) { defer wg.Done() - for j := 0; j < perGoroutine; j++ { + for j := range perGoroutine { q.Push(strconv.Itoa(id*perGoroutine + j)) } }(i) @@ -216,7 +216,7 @@ func testConcurrentQueueAccess(t *testing.T, q Queue[string]) { // Now dequeue everything sequentially total := goroutines * perGoroutine - for i := 0; i < total; i++ { + for range total { item, ok := q.Pop() assert.True(t, ok) _ = item // value not important for this test @@ -235,7 +235,7 @@ func TestQueueConcurrentRange(t *testing.T) { q := NewRWMutexQueue[int]() // Pre-populate the queue - for i := 0; i < 100; i++ { + for i := range 100 { q.Push(i) } @@ -245,7 +245,7 @@ func TestQueueConcurrentRange(t *testing.T) { // Goroutine 1: Concurrent Range calls go func() { defer wg.Done() - for i := 0; i < 20; i++ { + for range 20 { count := 0 q.Range(func(int) bool { count++ @@ -259,7 +259,7 @@ func TestQueueConcurrentRange(t *testing.T) { // Goroutine 2: Concurrent Push operations go func() { defer wg.Done() - for i := 0; i < 100; i++ { + for i := range 100 { q.Push(i + 1000) } }() @@ -267,7 +267,7 @@ func TestQueueConcurrentRange(t *testing.T) { // Goroutine 3: Concurrent Pop operations go func() { defer wg.Done() - for i := 0; i < 50; i++ { + for range 50 { q.Pop() } }() @@ -275,3 +275,36 @@ func TestQueueConcurrentRange(t *testing.T) { wg.Wait() // Test should complete without data races } + +func TestRWMutexQueueZeroValue(t *testing.T) { + // RWMutexQueue documents that zero-value is ready to use + var q RWMutexQueue[int] + + // Push on zero-value + q.Push(1, 2, 3) + assert.Equal(t, 3, q.Len()) + + // Peek should work + item, ok := q.Peek() + assert.True(t, ok) + assert.Equal(t, 1, item) + + // Pop should work + item, ok = q.Pop() + assert.True(t, ok) + assert.Equal(t, 1, item) + assert.Equal(t, 2, q.Len()) + + // Read operations on empty zero-value + var q2 RWMutexQueue[string] + assert.Equal(t, 0, q2.Len()) + _, ok = q2.Peek() + assert.False(t, ok) + _, ok = q2.Pop() + assert.False(t, ok) + + // Clear on zero-value should not panic + var q3 RWMutexQueue[int] + q3.Clear() + assert.Equal(t, 0, q3.Len()) +} diff --git a/slice_sharded.go b/slice_sharded.go index 356c101..f9e13ea 100644 --- a/slice_sharded.go +++ b/slice_sharded.go @@ -19,6 +19,9 @@ import ( // across goroutines is not critical. // // All methods are wait-free with bounded work and require no global locks. +// +// The zero value defaults to a single shard for compatibility, though NewShardedSlice should +// be used for performance-sensitive use cases to configure the optimal shard count. type ShardedSlice[T any] struct { shards []Slice[T] counter uint64 // used for round-robin shard selection in Append @@ -28,10 +31,19 @@ type ShardedSlice[T any] struct { // manner using an atomic counter. This ensures good key distribution without // requiring hashing the items themselves. func (s *ShardedSlice[T]) Append(item ...T) { + s.ensureInitialized() idx := int(atomic.AddUint64(&s.counter, 1)-1) % len(s.shards) s.shards[idx].Append(item...) } +// ensureInitialized lazily initializes the shards if needed for zero-value usage. +func (s *ShardedSlice[T]) ensureInitialized() { + if s.shards == nil { + // Default to single shard for zero-value usage + s.shards = []Slice[T]{NewRWMutexSlice[T](0)} + } +} + // Len returns the combined length of all shards. func (s *ShardedSlice[T]) Len() int { total := 0 diff --git a/slice_test.go b/slice_test.go index aad87c8..17d7a81 100644 --- a/slice_test.go +++ b/slice_test.go @@ -219,6 +219,77 @@ func TestSliceImplementations(t *testing.T) { }) } +func TestSliceZeroValue(t *testing.T) { + t.Run("RWMutexSlice", func(t *testing.T) { + // RWMutexSlice should be zero-value safe (slice-backed) + var s RWMutexSlice[int] + + // Append on zero-value + s.Append(1, 2, 3) + assert.Equal(t, 3, s.Len()) + + // Peek should work + items := s.Peek() + assert.Equal(t, []int{1, 2, 3}, items) + + // Flush should work + flushed := s.Flush() + assert.Equal(t, []int{1, 2, 3}, flushed) + assert.Equal(t, 0, s.Len()) + + // Read operations on zero-value + var s2 RWMutexSlice[string] + assert.Equal(t, 0, s2.Len()) + assert.Empty(t, s2.Peek()) + }) + + t.Run("MutexSlice", func(t *testing.T) { + // MutexSlice should be zero-value safe (slice-backed) + var s MutexSlice[int] + + // Append on zero-value + s.Append(1, 2, 3) + assert.Equal(t, 3, s.Len()) + + // Peek should work + items := s.Peek() + assert.Equal(t, []int{1, 2, 3}, items) + + // Flush should work + flushed := s.Flush() + assert.Equal(t, []int{1, 2, 3}, flushed) + assert.Equal(t, 0, s.Len()) + + // Read operations on zero-value + var s2 MutexSlice[string] + assert.Equal(t, 0, s2.Len()) + assert.Empty(t, s2.Peek()) + }) + + t.Run("ShardedSlice", func(t *testing.T) { + // ShardedSlice is zero-value safe (defaults to single shard) + var s ShardedSlice[int] + + // Append on zero-value (will default to 1 shard) + s.Append(1, 2, 3) + assert.Equal(t, 3, s.Len()) + + // Peek should work + items := s.Peek() + assert.Equal(t, []int{1, 2, 3}, items) + + // Flush should work + flushed := s.Flush() + assert.Equal(t, []int{1, 2, 3}, flushed) + assert.Equal(t, 0, s.Len()) + + // Read operations on zero-value + var s2 ShardedSlice[string] + assert.Equal(t, 0, s2.Len()) + assert.Empty(t, s2.Peek()) + }) +} + // // BENCHMARKS // From c550d7974d892c1d7eaccb652d535a2cab4a6b71 Mon Sep 17 00:00:00 2001 From: Jakob Ersson <11717405+jakobilobi@users.noreply.github.com> Date: Mon, 1 Dec 2025 10:32:25 +0100 Subject: [PATCH 5/7] docs: remove impr doc --- improvement_proposals.md | 61 ---------------------------------------- 1 file changed, 61 deletions(-) delete mode 100644 improvement_proposals.md diff --git a/improvement_proposals.md b/improvement_proposals.md deleted file mode 100644 index 6dd2d03..0000000 --- a/improvement_proposals.md +++ /dev/null @@ -1,61 +0,0 @@ -Thread-safe Set and Queue Fixes (PRD) - -## 1) RWMutexSet zero-value panic - -- **Problem**: `RWMutexSet` embeds an uninitialized `map[T]struct{}`. Calling `Add` on a zero-value set (e.g., `var s threadsafe.RWMutexSet[int]`) writes to a nil map and panics. -- **Impact**: Violates the repository pattern where most types are safe at zero value (e.g., `RWMutexQueue`), surprises callers, and can crash services if a zero value escapes DI/init code. -- **Repro**: - ```go - var s threadsafe.RWMutexSet[int] - s.Add(1) // panic: assignment to entry in nil map - ``` -- **Proposed fix**: lazily initialize `items` on first mutation so the zero value is usable. No API change. - ```go - // Add stores an item in the set. - func (s *RWMutexSet[T]) Add(item T) (added bool) { - s.mu.Lock() - if s.items == nil { // allow zero-value usage - s.items = make(map[T]struct{}) - } - if _, exists := s.items[item]; !exists { - s.items[item] = struct{}{} - s.size++ - s.mu.Unlock() - return true - } - s.mu.Unlock() - return false - } - ``` -- **Notes**: No behavioral change for existing constructor users. Optional follow-up: add a doc comment stating zero-value is ready to use. - -## 2) RWMutexQueue Range concurrency race - -- **Problem**: `Range` takes a slice view under `RLock`, unlocks, then iterates. Concurrent `Push`/`Pop` under `Lock` mutate the same backing array, so `Range` reads without synchronization. -- **Impact**: Data race in concurrent scenarios; possible stale or corrupted reads. Violates the “All operations must be safe for concurrent use” contract of `Queue`. -- **Repro (conceptual)**: - ```go - q := &threadsafe.RWMutexQueue[int]{} - var wg sync.WaitGroup - wg.Add(2) - go func() { defer wg.Done(); q.Range(func(int) bool { time.Sleep(time.Microsecond); return true }) }() - go func() { defer wg.Done(); q.Push(1); q.Pop() }() - wg.Wait() // race detector flags unsynchronized access - ``` -- **Proposed fix**: Snapshot under lock (matching `All`) or keep the lock during iteration. Snapshot keeps callers’ callback isolated from locks. - ```go - // Range calls f sequentially for each item from front to back. - func (q *RWMutexQueue[T]) Range(f func(item T) bool) { - q.mu.RLock() - snapshot := make([]T, len(q.items)-q.head) - copy(snapshot, q.items[q.head:]) - q.mu.RUnlock() - - for _, it := range snapshot { - if !f(it) { - break - } - } - } - ``` -- **Notes**: This aligns `Range` with `All`, preserves lock-free callback execution, and removes the race. Performance impact is minimal because a copy already exists in `All` and the queue’s size is bounded by workload. From 925ae3c3f7c0e0fb48fd3e613b98e5520f2d2d8c Mon Sep 17 00:00:00 2001 From: Jakob Ersson <11717405+jakobilobi@users.noreply.github.com> Date: Mon, 1 Dec 2025 10:47:32 +0100 Subject: [PATCH 6/7] docs: improve delete documentation for Map and Set --- TODO.md | 3 --- map.go | 2 +- set.go | 3 ++- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/TODO.md b/TODO.md index e62efa5..d597706 100644 --- a/TODO.md +++ b/TODO.md @@ -1,7 +1,4 @@ # todo -- Better function docs for the interfaces, example: - - What happens if deleting a key that doesn't exist? Perhaps use similar wording to the `delete` built-in: The delete built-in function deletes the element with the specified key (m[key]) from the map. If m is nil or there is no such element, delete is a no-op. - - Consider `Delete` in `Set`. - Potentially - Add last-in-first-out queue? \ No newline at end of file diff --git a/map.go b/map.go index 236775c..a1fb228 100644 --- a/map.go +++ b/map.go @@ -13,7 +13,7 @@ type Map[K comparable, V any] interface { Get(key K) (value V, loaded bool) // Set stores a value for the given key. Set(key K, value V) - // Delete removes the key from the map. + // Delete removes the key from the map. If the key doesn't exist, Delete is a no-op. Delete(key K) // Len returns the number of items in the map. Len() int diff --git a/set.go b/set.go index e8512ee..a76744e 100644 --- a/set.go +++ b/set.go @@ -7,7 +7,8 @@ import "iter" type Set[T comparable] interface { // Add stores an item in the set. Add(item T) (added bool) - // Delete removes an item from the set. + // Delete removes an item from the set. Returns true if the item was present and removed, + // false if it was not in the set. If the item doesn't exist, Delete is a no-op. Delete(item T) (removed bool) // Has returns true if the item is in the set, otherwise false. Has(item T) bool From be3332a86e9bcd978e4877a7413f11dd1cc4e44d Mon Sep 17 00:00:00 2001 From: Jakob Ersson <11717405+jakobilobi@users.noreply.github.com> Date: Mon, 1 Dec 2025 10:58:19 +0100 Subject: [PATCH 7/7] fix(linter): wg.Go --- queue_test.go | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/queue_test.go b/queue_test.go index 24730d6..e988063 100644 --- a/queue_test.go +++ b/queue_test.go @@ -240,11 +240,8 @@ func TestQueueConcurrentRange(t *testing.T) { } var wg sync.WaitGroup - wg.Add(3) - // Goroutine 1: Concurrent Range calls - go func() { - defer wg.Done() + wg.Go(func() { for range 20 { count := 0 q.Range(func(int) bool { @@ -254,23 +251,21 @@ func TestQueueConcurrentRange(t *testing.T) { // Verify we got some items (exact count may vary due to concurrent mutations) assert.Greater(t, count, 0) } - }() + }) // Goroutine 2: Concurrent Push operations - go func() { - defer wg.Done() + wg.Go(func() { for i := range 100 { q.Push(i + 1000) } - }() + }) // Goroutine 3: Concurrent Pop operations - go func() { - defer wg.Done() + wg.Go(func() { for range 50 { q.Pop() } - }() + }) wg.Wait() // Test should complete without data races