Fix data race on cache.client struct

1 files changed, 54 insertions, 28 deletions

diff --git a/internal/source/gitlab/cache/cache_test.go b/internal/source/gitlab/cache/cache_test.go
index 39f8cb73..5d1e153d 100644
--- a/internal/source/gitlab/cache/cache_test.go
+++ b/internal/source/gitlab/cache/cache_test.go
@@ -4,7 +4,6 @@ import (
 	"context"
 	"errors"
 	"sync"
-	"sync/atomic"
 	"testing"
 	"time"
 
@@ -13,27 +12,54 @@ import (
 	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/api"
 )
 
-type client struct {
+type stats struct {
+	m       sync.Mutex
 	started uint64
 	lookups uint64
+}
+
+type client struct {
+	stats   stats
 	bootup  chan uint64
 	domain  chan string
 	failure error
 }
 
-func (c *client) GetLookup(ctx context.Context, _ string) (lookup api.Lookup) {
-	// TODO This might not work on some architectures
-	//
-	// https://golang.org/pkg/sync/atomic/#pkg-note-BUG
-	//
-	// On ARM, x86-32, and 32-bit MIPS, it is the caller's responsibility to
-	// arrange for 64-bit alignment of 64-bit words accessed atomically. The first
-	// word in a variable or in an allocated struct, array, or slice can be relied
-	// upon to be 64-bit aligned.
+func (s *stats) bumpStarted() uint64 {
+	s.m.Lock()
+	defer s.m.Unlock()
+
+	s.started++
+	return s.started
+}
+
+func (s *stats) bumpLookups() uint64 {
+	s.m.Lock()
+	defer s.m.Unlock()
+
+	s.lookups++
+	return s.lookups
+}
+
+func (s *stats) getStarted() uint64 {
+	s.m.Lock()
+	defer s.m.Unlock()
+
+	return s.started
+}
+
+func (s *stats) getLookups() uint64 {
+	s.m.Lock()
+	defer s.m.Unlock()
+
+	return s.lookups
+}
 
-	c.bootup <- atomic.AddUint64(&c.started, 1)
-	defer atomic.AddUint64(&c.lookups, 1)
+func (c *client) GetLookup(ctx context.Context, _ string) api.Lookup {
+	c.bootup <- c.stats.bumpStarted()
+	defer c.stats.bumpLookups()
 
+	lookup := api.Lookup{}
 	if c.failure == nil {
 		lookup.Name = <-c.domain
 	} else {
@@ -103,7 +129,7 @@ func TestResolve(t *testing.T) {
 
 			require.NoError(t, lookup.Error)
 			require.Equal(t, "my.gitlab.com", lookup.Name)
-			require.Equal(t, uint64(1), resolver.lookups)
+			require.Equal(t, uint64(1), resolver.stats.getLookups())
 		})
 	})
 
@@ -122,12 +148,12 @@ func TestResolve(t *testing.T) {
 			go receiver()
 			go receiver()
 
-			require.Equal(t, uint64(0), resolver.lookups)
+			require.Equal(t, uint64(0), resolver.stats.getLookups())
 
 			resolver.domain <- "my.gitlab.com"
 			wg.Wait()
 
-			require.Equal(t, uint64(1), resolver.lookups)
+			require.Equal(t, uint64(1), resolver.stats.getLookups())
 		})
 	})
 
@@ -137,7 +163,7 @@ func TestResolve(t *testing.T) {
 				lookup := cache.Resolve(context.Background(), "my.gitlab.com")
 
 				require.Equal(t, "my.gitlab.com", lookup.Name)
-				require.Equal(t, uint64(0), resolver.lookups)
+				require.Equal(t, uint64(0), resolver.stats.getLookups())
 			})
 		})
 	})
@@ -153,14 +179,14 @@ func TestResolve(t *testing.T) {
 
 				<-resolver.bootup
 
-				require.Equal(t, uint64(1), resolver.started)
-				require.Equal(t, uint64(0), resolver.lookups)
+				require.Equal(t, uint64(1), resolver.stats.getStarted())
+				require.Equal(t, uint64(0), resolver.stats.getLookups())
 
 				resolver.domain <- "my.gitlab.com"
 				<-lookup
 
-				require.Equal(t, uint64(1), resolver.started)
-				require.Equal(t, uint64(1), resolver.lookups)
+				require.Equal(t, uint64(1), resolver.stats.getStarted())
+				require.Equal(t, uint64(1), resolver.stats.getLookups())
 			})
 		})
 	})
@@ -171,10 +197,10 @@ func TestResolve(t *testing.T) {
 				lookup := cache.Resolve(context.Background(), "my.gitlab.com")
 
 				require.Equal(t, "my.gitlab.com", lookup.Name)
-				require.Equal(t, uint64(0), resolver.lookups)
+				require.Equal(t, uint64(0), resolver.stats.getLookups())
 
 				resolver.domain <- "my.gitlab.com"
-				require.Equal(t, uint64(1), resolver.lookups)
+				require.Equal(t, uint64(1), resolver.stats.getLookups())
 			})
 		})
 	})
@@ -186,10 +212,10 @@ func TestResolve(t *testing.T) {
 				cache.Resolve(context.Background(), "my.gitlab.com")
 				cache.Resolve(context.Background(), "my.gitlab.com")
 
-				require.Equal(t, uint64(0), resolver.lookups)
+				require.Equal(t, uint64(0), resolver.stats.getLookups())
 
 				resolver.domain <- "my.gitlab.com"
-				require.Equal(t, uint64(1), resolver.lookups)
+				require.Equal(t, uint64(1), resolver.stats.getLookups())
 			})
 		})
 	})
@@ -200,7 +226,7 @@ func TestResolve(t *testing.T) {
 
 			lookup := cache.Resolve(context.Background(), "my.gitlab.com")
 
-			require.Equal(t, uint64(3), resolver.lookups)
+			require.Equal(t, uint64(3), resolver.stats.getLookups())
 			require.EqualError(t, lookup.Error, "500 err")
 		})
 	})
@@ -212,7 +238,7 @@ func TestResolve(t *testing.T) {
 		withTestCache(resolverConfig{}, func(cache *Cache, resolver *client) {
 			lookup := cache.Resolve(ctx, "my.gitlab.com")
 
-			require.Equal(t, uint64(0), resolver.lookups)
+			require.Equal(t, uint64(0), resolver.stats.getLookups())
 			require.EqualError(t, lookup.Error, "context done")
 		})
 	})
@@ -224,7 +250,7 @@ func TestResolve(t *testing.T) {
 		withTestCache(resolverConfig{}, func(cache *Cache, resolver *client) {
 			lookup := cache.Resolve(context.Background(), "my.gitlab.com")
 
-			require.Equal(t, uint64(0), resolver.lookups)
+			require.Equal(t, uint64(0), resolver.stats.getLookups())
 			require.EqualError(t, lookup.Error, "retrieval context done")
 		})
 	})