diff --git a/coderd/util/syncmap/map.go b/coderd/util/syncmap/map.go index f35973ea426..c943bf0f767 100644 --- a/coderd/util/syncmap/map.go +++ b/coderd/util/syncmap/map.go @@ -15,42 +15,44 @@ func New[K, V any]() *Map[K, V] { } } +// cast converts a value returned by the underlying sync.Map to T. The +// map returns a nil `any` for a missing key, and for a present key whose +// interface-typed value is nil. Neither can be type-asserted, so both +// become the zero T, which is nil for interface types. +func cast[T any](v any) T { + if v == nil { + var empty T + return empty + } + //nolint:forcetypeassert // Only K and V values ever enter the map. + return v.(T) +} + func (m *Map[K, V]) Store(k K, v V) { m.m.Store(k, v) } -//nolint:forcetypeassert func (m *Map[K, V]) Load(key K) (value V, ok bool) { v, ok := m.m.Load(key) - if !ok { - var empty V - return empty, false - } - return v.(V), ok + return cast[V](v), ok } func (m *Map[K, V]) Delete(key K) { m.m.Delete(key) } -//nolint:forcetypeassert func (m *Map[K, V]) LoadAndDelete(key K) (actual V, loaded bool) { act, loaded := m.m.LoadAndDelete(key) - if !loaded { - var empty V - return empty, loaded - } - return act.(V), loaded + return cast[V](act), loaded } -//nolint:forcetypeassert +// LoadOrStore returns the existing value for the key if present. +// Otherwise, it stores and returns the given value. The loaded result +// is true if the value was loaded, false if stored. As with sync.Map, +// actual is usable in both cases. func (m *Map[K, V]) LoadOrStore(key K, value V) (actual V, loaded bool) { act, loaded := m.m.LoadOrStore(key, value) - if !loaded { - var empty V - return empty, loaded - } - return act.(V), loaded + return cast[V](act), loaded } func (m *Map[K, V]) CompareAndSwap(key K, old V, newVal V) bool { @@ -61,19 +63,16 @@ func (m *Map[K, V]) CompareAndDelete(key K, old V) (deleted bool) { return m.m.CompareAndDelete(key, old) } -//nolint:forcetypeassert -func (m *Map[K, V]) Swap(key K, value V) (previous any, loaded bool) { - previous, loaded = m.m.Swap(key, value) - if !loaded { - var empty V - return empty, loaded - } - return previous.(V), loaded +// Swap stores the given value for the key and returns the previous +// value if there was one. As with sync.Map, previous is the zero V when +// the key was absent. +func (m *Map[K, V]) Swap(key K, value V) (previous V, loaded bool) { + prev, loaded := m.m.Swap(key, value) + return cast[V](prev), loaded } -//nolint:forcetypeassert func (m *Map[K, V]) Range(f func(key K, value V) bool) { - m.m.Range(func(key, value interface{}) bool { - return f(key.(K), value.(V)) + m.m.Range(func(key, value any) bool { + return f(cast[K](key), cast[V](value)) }) } diff --git a/coderd/util/syncmap/map_test.go b/coderd/util/syncmap/map_test.go new file mode 100644 index 00000000000..d61bf01f453 --- /dev/null +++ b/coderd/util/syncmap/map_test.go @@ -0,0 +1,289 @@ +package syncmap_test + +import ( + "sync" + "sync/atomic" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/coder/coder/v2/coderd/util/syncmap" +) + +// The tests below pin Map to the sync.Map contract it wraps. Where the +// stdlib returns a value, Map must return that same value typed as V, +// and where the stdlib returns nil, Map must return the zero V. + +func TestStoreLoad(t *testing.T) { + t.Parallel() + + m := syncmap.New[string, int]() + + v, ok := m.Load("missing") + require.False(t, ok) + require.Zero(t, v) + + m.Store("key", 1) + v, ok = m.Load("key") + require.True(t, ok) + require.Equal(t, 1, v) + + m.Store("key", 2) + v, ok = m.Load("key") + require.True(t, ok) + require.Equal(t, 2, v) +} + +func TestLoadOrStore(t *testing.T) { + t.Parallel() + + m := syncmap.New[string, int]() + + actual, loaded := m.LoadOrStore("key", 1) + require.False(t, loaded) + require.Equal(t, 1, actual, "stored value must be returned, not the zero value") + + actual, loaded = m.LoadOrStore("key", 2) + require.True(t, loaded) + require.Equal(t, 1, actual, "existing value must win") + + v, ok := m.Load("key") + require.True(t, ok) + require.Equal(t, 1, v) +} + +// TestLoadOrStorePointer covers the load-or-create pattern, where a +// zero-value return is a nil pointer the caller then dereferences. +func TestLoadOrStorePointer(t *testing.T) { + t.Parallel() + + m := syncmap.New[string, *atomic.Int32]() + + for range 3 { + counter, _ := m.LoadOrStore("key", &atomic.Int32{}) + require.NotNil(t, counter) + counter.Add(1) + } + + counter, ok := m.Load("key") + require.True(t, ok) + require.Equal(t, int32(3), counter.Load(), "all callers must share one counter") +} + +func TestLoadOrStoreConcurrent(t *testing.T) { + t.Parallel() + + const goroutines = 16 + + m := syncmap.New[string, *atomic.Int32]() + + var start, done sync.WaitGroup + start.Add(1) + done.Add(goroutines) + winners := make([]*atomic.Int32, goroutines) + loadedFlags := make([]bool, goroutines) + for i := range goroutines { + go func() { + defer done.Done() + start.Wait() + winners[i], loadedFlags[i] = m.LoadOrStore("key", &atomic.Int32{}) + }() + } + start.Done() + done.Wait() + + stored, ok := m.Load("key") + require.True(t, ok) + stores := 0 + for i, winner := range winners { + require.Same(t, stored, winner, "goroutine %d observed a different value than the map holds", i) + if !loadedFlags[i] { + stores++ + } + } + require.Equal(t, 1, stores, "exactly one goroutine should store") +} + +func TestLoadAndDelete(t *testing.T) { + t.Parallel() + + m := syncmap.New[string, int]() + + actual, loaded := m.LoadAndDelete("missing") + require.False(t, loaded) + require.Zero(t, actual) + + m.Store("key", 1) + actual, loaded = m.LoadAndDelete("key") + require.True(t, loaded) + require.Equal(t, 1, actual) + + _, ok := m.Load("key") + require.False(t, ok) +} + +func TestDelete(t *testing.T) { + t.Parallel() + + m := syncmap.New[string, int]() + + m.Delete("missing") // No-op. + + m.Store("key", 1) + m.Delete("key") + _, ok := m.Load("key") + require.False(t, ok) +} + +func TestSwap(t *testing.T) { + t.Parallel() + + m := syncmap.New[string, int]() + + previous, loaded := m.Swap("key", 1) + require.False(t, loaded) + require.Zero(t, previous) + + previous, loaded = m.Swap("key", 2) + require.True(t, loaded) + require.Equal(t, 1, previous) + + v, ok := m.Load("key") + require.True(t, ok) + require.Equal(t, 2, v) +} + +// TestSwapTyped pins previous to V rather than any: dereferencing it +// only compiles if the wrapper returns the value type. +func TestSwapTyped(t *testing.T) { + t.Parallel() + + m := syncmap.New[string, *int]() + first, second := 1, 2 + + previous, loaded := m.Swap("key", &first) + require.False(t, loaded) + require.Nil(t, previous) + + previous, loaded = m.Swap("key", &second) + require.True(t, loaded) + require.Equal(t, 1, *previous) +} + +func TestCompareAndSwap(t *testing.T) { + t.Parallel() + + m := syncmap.New[string, int]() + + require.False(t, m.CompareAndSwap("missing", 1, 2)) + + m.Store("key", 1) + require.False(t, m.CompareAndSwap("key", 2, 3), "swap must not happen on mismatch") + require.True(t, m.CompareAndSwap("key", 1, 3)) + + v, ok := m.Load("key") + require.True(t, ok) + require.Equal(t, 3, v) +} + +func TestCompareAndDelete(t *testing.T) { + t.Parallel() + + m := syncmap.New[string, int]() + + require.False(t, m.CompareAndDelete("missing", 1)) + + m.Store("key", 1) + require.False(t, m.CompareAndDelete("key", 2), "delete must not happen on mismatch") + require.True(t, m.CompareAndDelete("key", 1)) + + _, ok := m.Load("key") + require.False(t, ok) +} + +func TestRange(t *testing.T) { + t.Parallel() + + m := syncmap.New[string, int]() + want := map[string]int{"a": 1, "b": 2, "c": 3} + for k, v := range want { + m.Store(k, v) + } + + got := make(map[string]int) + m.Range(func(key string, value int) bool { + got[key] = value + return true + }) + require.Equal(t, want, got) + + visited := 0 + m.Range(func(string, int) bool { + visited++ + return false + }) + require.Equal(t, 1, visited, "returning false must stop iteration") +} + +// TestNilInterfaceValue covers an interface value type holding nil. +// sync.Map stores it as a nil `any`, which cannot be type-asserted, so +// every read path has to return the zero V instead of panicking. +func TestNilInterfaceValue(t *testing.T) { + t.Parallel() + + var nilErr error + + t.Run("LoadOrStore", func(t *testing.T) { + t.Parallel() + + m := syncmap.New[string, error]() + actual, loaded := m.LoadOrStore("key", nilErr) + require.False(t, loaded) + require.NoError(t, actual) + }) + + t.Run("Load", func(t *testing.T) { + t.Parallel() + + m := syncmap.New[string, error]() + m.Store("key", nilErr) + v, ok := m.Load("key") + require.True(t, ok, "a stored nil is still a present key") + require.NoError(t, v) + }) + + t.Run("LoadAndDelete", func(t *testing.T) { + t.Parallel() + + m := syncmap.New[string, error]() + m.Store("key", nilErr) + v, loaded := m.LoadAndDelete("key") + require.True(t, loaded) + require.NoError(t, v) + }) + + t.Run("Swap", func(t *testing.T) { + t.Parallel() + + m := syncmap.New[string, error]() + m.Store("key", nilErr) + previous, loaded := m.Swap("key", nilErr) + require.True(t, loaded) + require.NoError(t, previous) + }) + + t.Run("Range", func(t *testing.T) { + t.Parallel() + + m := syncmap.New[string, error]() + m.Store("key", nilErr) + visited := 0 + m.Range(func(key string, value error) bool { + visited++ + require.Equal(t, "key", key) + require.NoError(t, value) + return true + }) + require.Equal(t, 1, visited) + }) +}