diff options
author | Grzegorz Bizon <grzesiek.bizon@gmail.com> | 2019-11-14 13:50:43 +0300 |
---|---|---|
committer | Grzegorz Bizon <grzesiek.bizon@gmail.com> | 2019-11-14 13:50:43 +0300 |
commit | fd0e417eed8f73e6316482a2538074ecf31c3923 (patch) | |
tree | b74f63a8134b34edb7cb29ab5d964202ecec095d /internal/source/gitlab/cache | |
parent | 521a76e2348b3ada776d8031528bc81fe7693227 (diff) |
Simplify how we use gitlab source retrieval contexts
Diffstat (limited to 'internal/source/gitlab/cache')
-rw-r--r-- | internal/source/gitlab/cache/cache.go | 6 | ||||
-rw-r--r-- | internal/source/gitlab/cache/cache_test.go | 21 | ||||
-rw-r--r-- | internal/source/gitlab/cache/entry.go | 29 | ||||
-rw-r--r-- | internal/source/gitlab/cache/entry_test.go | 9 | ||||
-rw-r--r-- | internal/source/gitlab/cache/memstore.go | 19 | ||||
-rw-r--r-- | internal/source/gitlab/cache/retriever.go | 2 | ||||
-rw-r--r-- | internal/source/gitlab/cache/store.go | 6 |
7 files changed, 34 insertions, 58 deletions
diff --git a/internal/source/gitlab/cache/cache.go b/internal/source/gitlab/cache/cache.go index 03e6c3db..b3749450 100644 --- a/internal/source/gitlab/cache/cache.go +++ b/internal/source/gitlab/cache/cache.go @@ -20,19 +20,19 @@ func NewCache(client Resolver) *Cache { // Resolve is going to return a Lookup based on a domain name func (c *Cache) Resolve(ctx context.Context, domain string) *Lookup { - entry := c.store.LoadOrCreate(ctx, domain) + entry := c.store.LoadOrCreate(domain) if entry.IsUpToDate() { return entry.Lookup() } if entry.NeedsRefresh() { - entry.Refresh(ctx, c.client, c.store) + entry.Refresh(c.client, c.store) return entry.Lookup() } - <-entry.Retrieve(c.client) + <-entry.Retrieve(ctx, c.client) return entry.Lookup() } diff --git a/internal/source/gitlab/cache/cache_test.go b/internal/source/gitlab/cache/cache_test.go index 71212ef2..3a5a2e8f 100644 --- a/internal/source/gitlab/cache/cache_test.go +++ b/internal/source/gitlab/cache/cache_test.go @@ -64,7 +64,7 @@ func (cache *Cache) withTestEntry(config entryConfig, block func(*Entry)) { domain = config.domain } - entry := cache.store.LoadOrCreate(context.Background(), domain) + entry := cache.store.LoadOrCreate(domain) if config.retrieved { newResponse := make(chan Lookup, 1) @@ -210,7 +210,7 @@ func TestResolve(t *testing.T) { lookup := cache.Resolve(ctx, "my.gitlab.com") assert.Equal(t, uint64(0), resolver.resolutions) - assert.EqualError(t, lookup.Error, "context timeout") + assert.EqualError(t, lookup.Error, "context done") }) }) @@ -222,23 +222,24 @@ func TestResolve(t *testing.T) { lookup := cache.Resolve(context.Background(), "my.gitlab.com") assert.Equal(t, uint64(0), resolver.resolutions) - assert.EqualError(t, lookup.Error, "context timeout") + assert.EqualError(t, lookup.Error, "context done") }) }) - t.Run("when cache entry is evicted from cache", func(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) { cache.withTestEntry(entryConfig{expired: false, retrieved: false}, func(entry *Entry) { - ctx := context.Background() - lookup := make(chan *Lookup, 1) - go func() { lookup <- cache.Resolve(ctx, "my.gitlab.com") }() + ctx, cancel := context.WithCancel(context.Background()) + + response := make(chan *Lookup, 1) + go func() { response <- cache.Resolve(ctx, "my.gitlab.com") }() - cache.store.ReplaceOrCreate(ctx, "my.gitlab.com", newCacheEntry(ctx, "my.gitlab.com")) + cancel() resolver.domain <- "my.gitlab.com" - <-lookup + lookup := <-response - assert.EqualError(t, entry.ctx.Err(), "context canceled") + assert.EqualError(t, lookup.Error, "context done") }) }) }) diff --git a/internal/source/gitlab/cache/entry.go b/internal/source/gitlab/cache/entry.go index ad621963..01f44a47 100644 --- a/internal/source/gitlab/cache/entry.go +++ b/internal/source/gitlab/cache/entry.go @@ -17,8 +17,6 @@ var ( type Entry struct { domain string created time.Time - ctx context.Context - cancel context.CancelFunc fetch *sync.Once refresh *sync.Once mux *sync.RWMutex @@ -26,13 +24,9 @@ type Entry struct { response *Lookup } -func newCacheEntry(ctx context.Context, domain string) *Entry { - newctx, cancel := context.WithCancel(ctx) - +func newCacheEntry(domain string) *Entry { return &Entry{ domain: domain, - ctx: newctx, - cancel: cancel, created: time.Now(), fetch: &sync.Once{}, refresh: &sync.Once{}, @@ -69,11 +63,10 @@ func (e *Entry) Lookup() *Lookup { // Retrieve schedules a retrieval of a response. It returns a channel that is // going to be closed when retrieval is done, either successfully or not. -func (e *Entry) Retrieve(client Resolver) <-chan struct{} { +func (e *Entry) Retrieve(ctx context.Context, client Resolver) <-chan struct{} { + // TODO create context with timeout e.fetch.Do(func() { - retriever := Retriever{ - client: client, ctx: e.ctx, timeout: retrievalTimeout, - } + retriever := Retriever{client: client, ctx: ctx, timeout: retrievalTimeout} go e.setResponse(retriever.Retrieve(e.domain)) }) @@ -82,24 +75,18 @@ func (e *Entry) Retrieve(client Resolver) <-chan struct{} { } // Refresh will update the entry in the store only when it gets resolved. -func (e *Entry) Refresh(ctx context.Context, client Resolver, store Store) { +func (e *Entry) Refresh(client Resolver, store Store) { e.refresh.Do(func() { go func() { - entry := newCacheEntry(ctx, e.domain) + entry := newCacheEntry(e.domain) - <-entry.Retrieve(client) + <-entry.Retrieve(context.Background(), client) - store.ReplaceOrCreate(ctx, e.domain, entry) + store.ReplaceOrCreate(e.domain, entry) }() }) } -// CancelRetrieval cancels all cancelable contexts. Typically used when the -// entry is evicted from cache. -func (e *Entry) CancelRetrieval() { - e.cancel() -} - func (e *Entry) setResponse(response <-chan Lookup) { lookup := <-response diff --git a/internal/source/gitlab/cache/entry_test.go b/internal/source/gitlab/cache/entry_test.go index 98164cc3..503259de 100644 --- a/internal/source/gitlab/cache/entry_test.go +++ b/internal/source/gitlab/cache/entry_test.go @@ -1,7 +1,6 @@ package cache import ( - "context" "testing" "time" @@ -10,7 +9,7 @@ import ( func TestIsUpToDateAndNeedsRefresh(t *testing.T) { t.Run("when is resolved and not expired", func(t *testing.T) { - entry := newCacheEntry(context.Background(), "my.gitlab.com") + entry := newCacheEntry("my.gitlab.com") entry.response = &Lookup{} assert.True(t, entry.IsUpToDate()) @@ -18,7 +17,7 @@ func TestIsUpToDateAndNeedsRefresh(t *testing.T) { }) t.Run("when is resolved and is expired", func(t *testing.T) { - entry := newCacheEntry(context.Background(), "my.gitlab.com") + entry := newCacheEntry("my.gitlab.com") entry.response = &Lookup{} entry.created = time.Now().Add(-time.Hour) @@ -27,14 +26,14 @@ func TestIsUpToDateAndNeedsRefresh(t *testing.T) { }) t.Run("when is not resolved and not expired", func(t *testing.T) { - entry := newCacheEntry(context.Background(), "my.gitlab.com") + entry := newCacheEntry("my.gitlab.com") assert.False(t, entry.IsUpToDate()) assert.False(t, entry.NeedsRefresh()) }) t.Run("when is not resolved and is expired", func(t *testing.T) { - entry := newCacheEntry(context.Background(), "my.gitlab.com") + entry := newCacheEntry("my.gitlab.com") entry.created = time.Now().Add(-time.Hour) assert.False(t, entry.IsUpToDate()) diff --git a/internal/source/gitlab/cache/memstore.go b/internal/source/gitlab/cache/memstore.go index b5d4d868..f53f372f 100644 --- a/internal/source/gitlab/cache/memstore.go +++ b/internal/source/gitlab/cache/memstore.go @@ -1,7 +1,6 @@ package cache import ( - "context" "sync" "time" @@ -16,17 +15,13 @@ type memstore struct { var expiration = 10 * time.Minute func newMemStore() Store { - memStore := &memstore{ + return &memstore{ store: cache.New(expiration, time.Minute), mux: &sync.Mutex{}, } - - memStore.store.OnEvicted(memStore.OnEvicted) - - return memStore } -func (m *memstore) LoadOrCreate(ctx context.Context, domain string) *Entry { +func (m *memstore) LoadOrCreate(domain string) *Entry { m.mux.Lock() defer m.mux.Unlock() @@ -34,25 +29,21 @@ func (m *memstore) LoadOrCreate(ctx context.Context, domain string) *Entry { return entry.(*Entry) } - entry := newCacheEntry(ctx, domain) + entry := newCacheEntry(domain) m.store.SetDefault(domain, entry) return entry } -func (m *memstore) ReplaceOrCreate(ctx context.Context, domain string, entry *Entry) *Entry { +func (m *memstore) ReplaceOrCreate(domain string, entry *Entry) *Entry { m.mux.Lock() defer m.mux.Unlock() if _, exists := m.store.Get(domain); exists { - m.store.Delete(domain) // delete manually to trigger onEvicted + m.store.Delete(domain) } m.store.SetDefault(domain, entry) return entry } - -func (m *memstore) OnEvicted(key string, value interface{}) { - value.(*Entry).CancelRetrieval() -} diff --git a/internal/source/gitlab/cache/retriever.go b/internal/source/gitlab/cache/retriever.go index b604b9fa..de766aa4 100644 --- a/internal/source/gitlab/cache/retriever.go +++ b/internal/source/gitlab/cache/retriever.go @@ -35,7 +35,7 @@ func (r *Retriever) retrieveWithTimeout(domain string, response chan<- Lookup) { select { case <-newctx.Done(): - response <- Lookup{Status: 502, Error: errors.New("context timeout")} + response <- Lookup{Status: 502, Error: errors.New("context done")} fmt.Println("retrieval context done") // TODO logme case lookup = <-r.resolveWithBackoff(newctx, domain): response <- lookup diff --git a/internal/source/gitlab/cache/store.go b/internal/source/gitlab/cache/store.go index d2619c0e..3cf8aac1 100644 --- a/internal/source/gitlab/cache/store.go +++ b/internal/source/gitlab/cache/store.go @@ -1,9 +1,7 @@ package cache -import "context" - // Store defines an interface describing an abstract cache store type Store interface { - LoadOrCreate(ctx context.Context, domain string) *Entry - ReplaceOrCreate(ctx context.Context, domain string, entry *Entry) *Entry + LoadOrCreate(domain string) *Entry + ReplaceOrCreate(domain string, entry *Entry) *Entry } |