Merge branch '356-fix-cache-race' into 'master'

Fix data race in GitLab source cache package

Closes #356

See merge request gitlab-org/gitlab-pages!249

8 files changed, 147 insertions, 97 deletions

diff --git a/internal/source/gitlab/cache/cache.go b/internal/source/gitlab/cache/cache.go
index 9a4ab753..bb3567b4 100644
--- a/internal/source/gitlab/cache/cache.go
+++ b/internal/source/gitlab/cache/cache.go
@@ -2,22 +2,44 @@ package cache
 
 import (
 	"context"
+	"time"
 
 	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/api"
 	"gitlab.com/gitlab-org/gitlab-pages/metrics"
 )
 
+var defaultCacheConfig = cacheConfig{
+	cacheExpiry:          10 * time.Minute,
+	entryRefreshTimeout:  30 * time.Second,
+	retrievalTimeout:     5 * time.Second,
+	maxRetrievalInterval: time.Second,
+	maxRetrievalRetries:  3,
+}
+
 // Cache is a short and long caching mechanism for GitLab source
 type Cache struct {
-	client api.Client
-	store  Store
+	client      api.Client
+	store       Store
+	cacheConfig *cacheConfig
+}
+
+type cacheConfig struct {
+	cacheExpiry          time.Duration
+	entryRefreshTimeout  time.Duration
+	retrievalTimeout     time.Duration
+	maxRetrievalInterval time.Duration
+	maxRetrievalRetries  int
 }
 
 // NewCache creates a new instance of Cache.
-func NewCache(client api.Client) *Cache {
+func NewCache(client api.Client, cc *cacheConfig) *Cache {
+	if cc == nil {
+		cc = &defaultCacheConfig
+	}
+
 	return &Cache{
 		client: client,
-		store:  newMemStore(),
+		store:  newMemStore(client, cc),
 	}
 }
 
@@ -25,8 +47,8 @@ func NewCache(client api.Client) *Cache {
 // algorithm works as follows:
 // - We first check if the cache entry exists, and if it is up-to-date. If it
 //   is fresh we return the lookup entry from cache and it is a cache hit.
-// - If entry is not up-to-date, what means that it has been created in a cache
-//   more than `shortCacheExpiry` duration ago,  we schedule an asynchronous
+// - If entry is not up-to-date, that means it has been created in a cache
+//   more than `entryRefreshTimeout` duration ago,  we schedule an asynchronous
 //   retrieval of the latest configuration we are going to obtain through the
 //   API, and we immediately return an old value, to avoid blocking clients. In
 //   this case it is also a cache hit.
diff --git a/internal/source/gitlab/cache/cache_test.go b/internal/source/gitlab/cache/cache_test.go
index a0ee507d..c3e03dec 100644
--- a/internal/source/gitlab/cache/cache_test.go
+++ b/internal/source/gitlab/cache/cache_test.go
@@ -69,7 +69,7 @@ func (c *client) GetLookup(ctx context.Context, _ string) api.Lookup {
 	return lookup
 }
 
-func withTestCache(config resolverConfig, block func(*Cache, *client)) {
+func withTestCache(config resolverConfig, cacheConfig *cacheConfig, block func(*Cache, *client)) {
 	var chanSize int
 
 	if config.buffered {
@@ -84,7 +84,7 @@ func withTestCache(config resolverConfig, block func(*Cache, *client)) {
 		failure: config.failure,
 	}
 
-	cache := NewCache(resolver)
+	cache := NewCache(resolver, cacheConfig)
 
 	block(cache, resolver)
 }
@@ -122,7 +122,7 @@ type entryConfig struct {
 
 func TestResolve(t *testing.T) {
 	t.Run("when item is not cached", func(t *testing.T) {
-		withTestCache(resolverConfig{buffered: true}, func(cache *Cache, resolver *client) {
+		withTestCache(resolverConfig{buffered: true}, nil, func(cache *Cache, resolver *client) {
 			resolver.domain <- "my.gitlab.com"
 
 			lookup := cache.Resolve(context.Background(), "my.gitlab.com")
@@ -134,7 +134,7 @@ func TestResolve(t *testing.T) {
 	})
 
 	t.Run("when item is not cached and accessed multiple times", func(t *testing.T) {
-		withTestCache(resolverConfig{}, func(cache *Cache, resolver *client) {
+		withTestCache(resolverConfig{}, nil, func(cache *Cache, resolver *client) {
 			wg := &sync.WaitGroup{}
 			ctx := context.Background()
 
@@ -158,7 +158,7 @@ func TestResolve(t *testing.T) {
 	})
 
 	t.Run("when item is in short cache", func(t *testing.T) {
-		withTestCache(resolverConfig{}, func(cache *Cache, resolver *client) {
+		withTestCache(resolverConfig{}, nil, func(cache *Cache, resolver *client) {
 			cache.withTestEntry(entryConfig{expired: false, retrieved: true}, func(*Entry) {
 				lookup := cache.Resolve(context.Background(), "my.gitlab.com")
 
@@ -169,7 +169,7 @@ func TestResolve(t *testing.T) {
 	})
 
 	t.Run("when a non-retrieved new item is in short cache", func(t *testing.T) {
-		withTestCache(resolverConfig{}, func(cache *Cache, resolver *client) {
+		withTestCache(resolverConfig{}, nil, func(cache *Cache, resolver *client) {
 			cache.withTestEntry(entryConfig{expired: false, retrieved: false}, func(*Entry) {
 				lookup := make(chan *api.Lookup, 1)
 
@@ -192,7 +192,7 @@ func TestResolve(t *testing.T) {
 	})
 
 	t.Run("when item is in long cache only", func(t *testing.T) {
-		withTestCache(resolverConfig{}, func(cache *Cache, resolver *client) {
+		withTestCache(resolverConfig{}, nil, func(cache *Cache, resolver *client) {
 			cache.withTestEntry(entryConfig{expired: true, retrieved: true}, func(*Entry) {
 				lookup := cache.Resolve(context.Background(), "my.gitlab.com")
 
@@ -206,7 +206,7 @@ func TestResolve(t *testing.T) {
 	})
 
 	t.Run("when item in long cache is requested multiple times", func(t *testing.T) {
-		withTestCache(resolverConfig{}, func(cache *Cache, resolver *client) {
+		withTestCache(resolverConfig{}, nil, func(cache *Cache, resolver *client) {
 			cache.withTestEntry(entryConfig{expired: true, retrieved: true}, func(*Entry) {
 				cache.Resolve(context.Background(), "my.gitlab.com")
 				cache.Resolve(context.Background(), "my.gitlab.com")
@@ -221,9 +221,10 @@ func TestResolve(t *testing.T) {
 	})
 
 	t.Run("when retrieval failed with an error", func(t *testing.T) {
-		withTestCache(resolverConfig{failure: errors.New("500 err")}, func(cache *Cache, resolver *client) {
-			maxRetrievalInterval = 0
+		cc := defaultCacheConfig
+		cc.maxRetrievalInterval = 0
 
+		withTestCache(resolverConfig{failure: errors.New("500 err")}, &cc, func(cache *Cache, resolver *client) {
 			lookup := cache.Resolve(context.Background(), "my.gitlab.com")
 
 			require.Equal(t, uint64(3), resolver.stats.getLookups())
@@ -235,7 +236,7 @@ func TestResolve(t *testing.T) {
 		ctx, cancel := context.WithCancel(context.Background())
 		cancel()
 
-		withTestCache(resolverConfig{}, func(cache *Cache, resolver *client) {
+		withTestCache(resolverConfig{}, nil, func(cache *Cache, resolver *client) {
 			lookup := cache.Resolve(ctx, "my.gitlab.com")
 
 			require.Equal(t, uint64(0), resolver.stats.getLookups())
@@ -244,12 +245,10 @@ func TestResolve(t *testing.T) {
 	})
 
 	t.Run("when retrieval failed because of an internal retriever context timeout", func(t *testing.T) {
-		t.Skip("Data race")
+		cc := defaultCacheConfig
+		cc.retrievalTimeout = 0
 
-		retrievalTimeout = 0
-		defer func() { retrievalTimeout = 5 * time.Second }()
-
-		withTestCache(resolverConfig{}, func(cache *Cache, resolver *client) {
+		withTestCache(resolverConfig{}, &cc, func(cache *Cache, resolver *client) {
 			lookup := cache.Resolve(context.Background(), "my.gitlab.com")
 
 			require.Equal(t, uint64(0), resolver.stats.getLookups())
@@ -258,7 +257,7 @@ func TestResolve(t *testing.T) {
 	})
 
 	t.Run("when retrieval failed because of resolution context being canceled", func(t *testing.T) {
-		withTestCache(resolverConfig{}, func(cache *Cache, resolver *client) {
+		withTestCache(resolverConfig{}, nil, func(cache *Cache, resolver *client) {
 			cache.withTestEntry(entryConfig{expired: false, retrieved: false}, func(entry *Entry) {
 				ctx, cancel := context.WithCancel(context.Background())
 
diff --git a/internal/source/gitlab/cache/const.go b/internal/source/gitlab/cache/const.go
deleted file mode 100644
index 90c5bddc..00000000
--- a/internal/source/gitlab/cache/const.go
+++ /dev/null
@@ -1,11 +0,0 @@
-package cache
-
-import "time"
-
-var (
-	shortCacheExpiry     = 30 * time.Second
-	longCacheExpiry      = 10 * time.Minute
-	retrievalTimeout     = 5 * time.Second
-	maxRetrievalInterval = time.Second
-	maxRetrievalRetries  = 3
-)
diff --git a/internal/source/gitlab/cache/entry.go b/internal/source/gitlab/cache/entry.go
index d91bb331..191ef789 100644
--- a/internal/source/gitlab/cache/entry.go
+++ b/internal/source/gitlab/cache/entry.go
@@ -13,23 +13,28 @@ import (
 // holds a pointer to *api.Lookup when the domain lookup has been retrieved
 // successfully
 type Entry struct {
-	domain    string
-	created   time.Time
-	retrieve  *sync.Once
-	refresh   *sync.Once
-	mux       *sync.RWMutex
-	retrieved chan struct{}
-	response  *api.Lookup
+	domain         string
+	created        time.Time
+	retrieve       *sync.Once
+	refresh        *sync.Once
+	mux            *sync.RWMutex
+	retrieved      chan struct{}
+	response       *api.Lookup
+	refreshTimeout time.Duration
+	retriever      *Retriever
 }
 
-func newCacheEntry(domain string) *Entry {
+func newCacheEntry(domain string, refreshTimeout time.Duration, retriever *Retriever) *Entry {
+
 	return &Entry{
-		domain:    domain,
-		created:   time.Now(),
-		retrieve:  &sync.Once{},
-		refresh:   &sync.Once{},
-		mux:       &sync.RWMutex{},
-		retrieved: make(chan struct{}),
+		domain:         domain,
+		created:        time.Now(),
+		retrieve:       &sync.Once{},
+		refresh:        &sync.Once{},
+		mux:            &sync.RWMutex{},
+		retrieved:      make(chan struct{}),
+		refreshTimeout: refreshTimeout,
+		retriever:      retriever,
 	}
 }
 
@@ -61,7 +66,7 @@ func (e *Entry) Lookup() *api.Lookup {
 
 // Retrieve perform a blocking retrieval of the cache entry response.
 func (e *Entry) Retrieve(ctx context.Context, client api.Client) (lookup *api.Lookup) {
-	e.retrieve.Do(func() { go e.retrieveWithClient(client) })
+	e.retrieve.Do(func() { go e.setResponse(e.retriever.Retrieve(e.domain)) })
 
 	select {
 	case <-ctx.Done():
@@ -77,7 +82,7 @@ func (e *Entry) Retrieve(ctx context.Context, client api.Client) (lookup *api.Lo
 func (e *Entry) Refresh(client api.Client, store Store) {
 	e.refresh.Do(func() {
 		go func() {
-			entry := newCacheEntry(e.domain)
+			entry := newCacheEntry(e.domain, e.refreshTimeout, e.retriever)
 
 			entry.Retrieve(context.Background(), client)
 
@@ -86,12 +91,6 @@ func (e *Entry) Refresh(client api.Client, store Store) {
 	})
 }
 
-func (e *Entry) retrieveWithClient(client api.Client) {
-	retriever := Retriever{client: client}
-
-	e.setResponse(retriever.Retrieve(e.domain))
-}
-
 func (e *Entry) setResponse(lookup api.Lookup) {
 	e.mux.Lock()
 	defer e.mux.Unlock()
@@ -101,7 +100,7 @@ func (e *Entry) setResponse(lookup api.Lookup) {
 }
 
 func (e *Entry) isExpired() bool {
-	return time.Since(e.created) > shortCacheExpiry
+	return time.Since(e.created) > e.refreshTimeout
 }
 
 func (e *Entry) isResolved() bool {
diff --git a/internal/source/gitlab/cache/entry_test.go b/internal/source/gitlab/cache/entry_test.go
index 118981d0..e6a86557 100644
--- a/internal/source/gitlab/cache/entry_test.go
+++ b/internal/source/gitlab/cache/entry_test.go
@@ -10,35 +10,55 @@ import (
 )
 
 func TestIsUpToDateAndNeedsRefresh(t *testing.T) {
-	t.Run("when is resolved and not expired", func(t *testing.T) {
-		entry := newCacheEntry("my.gitlab.com")
-		entry.response = &api.Lookup{}
+	tests := []struct {
+		name                string
+		resolved            bool
+		expired             bool
+		expectedIsUpToDate  bool
+		expectedNeedRefresh bool
+	}{
+		{
+			name:                "resolved_and_not_expired",
+			resolved:            true,
+			expired:             false,
+			expectedIsUpToDate:  true,
+			expectedNeedRefresh: false,
+		},
+		{
+			name:                "resolved_and_expired",
+			resolved:            true,
+			expired:             true,
+			expectedIsUpToDate:  false,
+			expectedNeedRefresh: true,
+		},
+		{
+			name:                "not_resolved_and_not_expired",
+			resolved:            false,
+			expired:             false,
+			expectedIsUpToDate:  false,
+			expectedNeedRefresh: false,
+		},
+		{
+			name:                "not_resolved_and_expired",
+			resolved:            false,
+			expired:             true,
+			expectedIsUpToDate:  false,
+			expectedNeedRefresh: false,
+		},
+	}
 
-		require.True(t, entry.IsUpToDate())
-		require.False(t, entry.NeedsRefresh())
-	})
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			entry := newCacheEntry("my.gitlab.com", defaultCacheConfig.entryRefreshTimeout, nil)
+			if tt.resolved {
+				entry.response = &api.Lookup{}
+			}
+			if tt.expired {
+				entry.created = time.Now().Add(-time.Hour)
+			}
 
-	t.Run("when is resolved and is expired", func(t *testing.T) {
-		entry := newCacheEntry("my.gitlab.com")
-		entry.response = &api.Lookup{}
-		entry.created = time.Now().Add(-time.Hour)
-
-		require.False(t, entry.IsUpToDate())
-		require.True(t, entry.NeedsRefresh())
-	})
-
-	t.Run("when is not resolved and not expired", func(t *testing.T) {
-		entry := newCacheEntry("my.gitlab.com")
-
-		require.False(t, entry.IsUpToDate())
-		require.False(t, entry.NeedsRefresh())
-	})
-
-	t.Run("when is not resolved and is expired", func(t *testing.T) {
-		entry := newCacheEntry("my.gitlab.com")
-		entry.created = time.Now().Add(-time.Hour)
-
-		require.False(t, entry.IsUpToDate())
-		require.False(t, entry.NeedsRefresh())
-	})
+			require.Equal(t, tt.expectedIsUpToDate, entry.IsUpToDate())
+			require.Equal(t, tt.expectedNeedRefresh, entry.NeedsRefresh())
+		})
+	}
 }
diff --git a/internal/source/gitlab/cache/memstore.go b/internal/source/gitlab/cache/memstore.go
index 08eccd94..1d7c678d 100644
--- a/internal/source/gitlab/cache/memstore.go
+++ b/internal/source/gitlab/cache/memstore.go
@@ -5,17 +5,24 @@ import (
 	"time"
 
 	cache "github.com/patrickmn/go-cache"
+
+	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/api"
 )
 
 type memstore struct {
-	store *cache.Cache
-	mux   *sync.RWMutex
+	store               *cache.Cache
+	mux                 *sync.RWMutex
+	retriever           *Retriever
+	entryRefreshTimeout time.Duration
 }
 
-func newMemStore() Store {
+func newMemStore(client api.Client, cc *cacheConfig) Store {
+	retriever := NewRetriever(client, cc.retrievalTimeout, cc.maxRetrievalInterval, cc.maxRetrievalRetries)
 	return &memstore{
-		store: cache.New(longCacheExpiry, time.Minute),
-		mux:   &sync.RWMutex{},
+		store:               cache.New(cc.cacheExpiry, time.Minute),
+		mux:                 &sync.RWMutex{},
+		retriever:           retriever,
+		entryRefreshTimeout: cc.entryRefreshTimeout,
 	}
 }
 
@@ -37,7 +44,7 @@ func (m *memstore) LoadOrCreate(domain string) *Entry {
 		return entry.(*Entry)
 	}
 
-	newEntry := newCacheEntry(domain)
+	newEntry := newCacheEntry(domain, m.entryRefreshTimeout, m.retriever)
 	m.store.SetDefault(domain, newEntry)
 
 	return newEntry
diff --git a/internal/source/gitlab/cache/retriever.go b/internal/source/gitlab/cache/retriever.go
index cf0541b1..de37c231 100644
--- a/internal/source/gitlab/cache/retriever.go
+++ b/internal/source/gitlab/cache/retriever.go
@@ -13,13 +13,26 @@ import (
 // Retriever is an utility type that performs an HTTP request with backoff in
 // case of errors
 type Retriever struct {
-	client api.Client
+	client               api.Client
+	retrievalTimeout     time.Duration
+	maxRetrievalInterval time.Duration
+	maxRetrievalRetries  int
+}
+
+// NewRetriever creates a Retriever with a client
+func NewRetriever(client api.Client, retrievalTimeout, maxRetrievalInterval time.Duration, maxRetrievalRetries int) *Retriever {
+	return &Retriever{
+		client:               client,
+		retrievalTimeout:     retrievalTimeout,
+		maxRetrievalInterval: maxRetrievalInterval,
+		maxRetrievalRetries:  maxRetrievalRetries,
+	}
 }
 
 // Retrieve retrieves a lookup response from external source with timeout and
 // backoff. It has its own context with timeout.
 func (r *Retriever) Retrieve(domain string) (lookup api.Lookup) {
-	ctx, cancel := context.WithTimeout(context.Background(), retrievalTimeout)
+	ctx, cancel := context.WithTimeout(context.Background(), r.retrievalTimeout)
 	defer cancel()
 
 	select {
@@ -39,11 +52,11 @@ func (r *Retriever) resolveWithBackoff(ctx context.Context, domain string) <-cha
 	go func() {
 		var lookup api.Lookup
 
-		for i := 1; i <= maxRetrievalRetries; i++ {
+		for i := 1; i <= r.maxRetrievalRetries; i++ {
 			lookup = r.client.GetLookup(ctx, domain)
 
 			if lookup.Error != nil {
-				time.Sleep(maxRetrievalInterval)
+				time.Sleep(r.maxRetrievalInterval)
 			} else {
 				break
 			}
diff --git a/internal/source/gitlab/gitlab.go b/internal/source/gitlab/gitlab.go
index 6260200a..12da9af1 100644
--- a/internal/source/gitlab/gitlab.go
+++ b/internal/source/gitlab/gitlab.go
@@ -28,7 +28,8 @@ func New(config client.Config) (*Gitlab, error) {
 		return nil, err
 	}
 
-	return &Gitlab{client: cache.NewCache(client)}, nil
+	// using nil for cache config will use the default values specified in internal/source/gitlab/cache/cache.go#12
+	return &Gitlab{client: cache.NewCache(client, nil)}, nil
 }
 
 // GetDomain return a representation of a domain that we have fetched from