diff options
author | Alessio Caiazza <acaiazza@gitlab.com> | 2020-03-03 17:50:04 +0300 |
---|---|---|
committer | Alessio Caiazza <acaiazza@gitlab.com> | 2020-03-03 17:50:04 +0300 |
commit | 369b1801000f079ffdafe06650ce933902c928cd (patch) | |
tree | 3c6691e7f821b6dc11f95c094d742cfafb92fd4f | |
parent | 3ae8b1c077bef83c480cba2695e3943ba152e00c (diff) | |
parent | 536bdde15bc09aab0bd1a15a5ac9f4b5133716bd (diff) |
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
-rw-r--r-- | internal/source/gitlab/cache/cache.go | 34 | ||||
-rw-r--r-- | internal/source/gitlab/cache/cache_test.go | 33 | ||||
-rw-r--r-- | internal/source/gitlab/cache/const.go | 11 | ||||
-rw-r--r-- | internal/source/gitlab/cache/entry.go | 45 | ||||
-rw-r--r-- | internal/source/gitlab/cache/entry_test.go | 78 | ||||
-rw-r--r-- | internal/source/gitlab/cache/memstore.go | 19 | ||||
-rw-r--r-- | internal/source/gitlab/cache/retriever.go | 21 | ||||
-rw-r--r-- | internal/source/gitlab/gitlab.go | 3 |
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 |