From bc525606dff004e17ba1c8106857c638da3c7941 Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Tue, 11 Aug 2020 16:37:07 +1000 Subject: [PATCH 1/4] Call retrieve inside once asynchronously --- internal/source/gitlab/cache/entry.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/source/gitlab/cache/entry.go b/internal/source/gitlab/cache/entry.go index d33d2758c..4bc04a570 100644 --- a/internal/source/gitlab/cache/entry.go +++ b/internal/source/gitlab/cache/entry.go @@ -65,7 +65,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.setResponse(e.retriever.Retrieve(e.domain)) }) + go e.retrieve.Do(func() { e.setResponse(e.retriever.Retrieve(e.domain)) }) select { case <-ctx.Done(): -- GitLab From 366d185bc2ac26e7e70b3eb3d6a94267b1a09314 Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Wed, 12 Aug 2020 19:03:27 +1000 Subject: [PATCH 2/4] Add sleep to test config --- internal/source/gitlab/cache/cache_test.go | 9 ++++++--- internal/source/gitlab/cache/entry.go | 2 +- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/internal/source/gitlab/cache/cache_test.go b/internal/source/gitlab/cache/cache_test.go index 52a3a4895..bbc472c9e 100644 --- a/internal/source/gitlab/cache/cache_test.go +++ b/internal/source/gitlab/cache/cache_test.go @@ -23,6 +23,7 @@ type client struct { bootup chan uint64 domain chan string failure error + sleep time.Duration } func (s *stats) bumpStarted() uint64 { @@ -61,6 +62,7 @@ func (c *client) GetLookup(ctx context.Context, _ string) api.Lookup { lookup := api.Lookup{} if c.failure == nil { + time.Sleep(c.sleep) lookup.Name = <-c.domain } else { lookup.Error = c.failure @@ -83,6 +85,7 @@ func withTestCache(config resolverConfig, cacheConfig *cacheConfig, block func(* } resolver := &client{ + sleep: config.sleep, domain: make(chan string, chanSize), bootup: make(chan uint64, 100), failure: config.failure, @@ -114,6 +117,7 @@ func (cache *Cache) withTestEntry(config entryConfig, block func(*Entry)) { } type resolverConfig struct { + sleep time.Duration buffered bool failure error } @@ -249,15 +253,14 @@ func TestResolve(t *testing.T) { }) t.Run("when retrieval failed because of resolution context being canceled", func(t *testing.T) { - withTestCache(resolverConfig{}, nil, func(cache *Cache, resolver *client) { + withTestCache(resolverConfig{sleep: 20 * time.Millisecond}, nil, func(cache *Cache, resolver *client) { cache.withTestEntry(entryConfig{expired: false, retrieved: false}, func(entry *Entry) { ctx, cancel := context.WithCancel(context.Background()) + cancel() response := make(chan *api.Lookup, 1) go func() { response <- cache.Resolve(ctx, "my.gitlab.com") }() - cancel() - resolver.domain <- "my.gitlab.com" lookup := <-response diff --git a/internal/source/gitlab/cache/entry.go b/internal/source/gitlab/cache/entry.go index 4bc04a570..d33d2758c 100644 --- a/internal/source/gitlab/cache/entry.go +++ b/internal/source/gitlab/cache/entry.go @@ -65,7 +65,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) { - go e.retrieve.Do(func() { e.setResponse(e.retriever.Retrieve(e.domain)) }) + e.retrieve.Do(func() { go e.setResponse(e.retriever.Retrieve(e.domain)) }) select { case <-ctx.Done(): -- GitLab From 09f4630f44bea6431df166e53696c1e93706c6ce Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Wed, 12 Aug 2020 20:18:39 +1000 Subject: [PATCH 3/4] Remove sleep and reorg call to setResponse --- internal/source/gitlab/cache/cache_test.go | 9 +++------ internal/source/gitlab/cache/entry.go | 9 ++++++++- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/internal/source/gitlab/cache/cache_test.go b/internal/source/gitlab/cache/cache_test.go index bbc472c9e..52a3a4895 100644 --- a/internal/source/gitlab/cache/cache_test.go +++ b/internal/source/gitlab/cache/cache_test.go @@ -23,7 +23,6 @@ type client struct { bootup chan uint64 domain chan string failure error - sleep time.Duration } func (s *stats) bumpStarted() uint64 { @@ -62,7 +61,6 @@ func (c *client) GetLookup(ctx context.Context, _ string) api.Lookup { lookup := api.Lookup{} if c.failure == nil { - time.Sleep(c.sleep) lookup.Name = <-c.domain } else { lookup.Error = c.failure @@ -85,7 +83,6 @@ func withTestCache(config resolverConfig, cacheConfig *cacheConfig, block func(* } resolver := &client{ - sleep: config.sleep, domain: make(chan string, chanSize), bootup: make(chan uint64, 100), failure: config.failure, @@ -117,7 +114,6 @@ func (cache *Cache) withTestEntry(config entryConfig, block func(*Entry)) { } type resolverConfig struct { - sleep time.Duration buffered bool failure error } @@ -253,14 +249,15 @@ func TestResolve(t *testing.T) { }) t.Run("when retrieval failed because of resolution context being canceled", func(t *testing.T) { - withTestCache(resolverConfig{sleep: 20 * time.Millisecond}, nil, 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()) - cancel() response := make(chan *api.Lookup, 1) go func() { response <- cache.Resolve(ctx, "my.gitlab.com") }() + cancel() + resolver.domain <- "my.gitlab.com" lookup := <-response diff --git a/internal/source/gitlab/cache/entry.go b/internal/source/gitlab/cache/entry.go index d33d2758c..162d0d3b1 100644 --- a/internal/source/gitlab/cache/entry.go +++ b/internal/source/gitlab/cache/entry.go @@ -65,7 +65,14 @@ 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.setResponse(e.retriever.Retrieve(e.domain)) }) + e.retrieve.Do(func() { + go func() { + // `e.setResponse` closes the `e.retrieved` channel when done. + // allocating a new anonymous function gives the runtime enough time to process the context.Cancel + // for TestResolve/when_retrieval_failed_because_of_resolution_context_being_canceled + e.setResponse(e.retriever.Retrieve(e.domain)) + }() + }) select { case <-ctx.Done(): -- GitLab From 0ce022dacade8e7c442d85bd6db1399884a880aa Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Mon, 17 Aug 2020 17:46:57 +1000 Subject: [PATCH 4/4] Call to retrieve on a separate function --- internal/source/gitlab/cache/cache_test.go | 15 ++++++--------- internal/source/gitlab/cache/entry.go | 16 ++++++++-------- 2 files changed, 14 insertions(+), 17 deletions(-) diff --git a/internal/source/gitlab/cache/cache_test.go b/internal/source/gitlab/cache/cache_test.go index 52a3a4895..c6e7d9788 100644 --- a/internal/source/gitlab/cache/cache_test.go +++ b/internal/source/gitlab/cache/cache_test.go @@ -60,12 +60,13 @@ func (c *client) GetLookup(ctx context.Context, _ string) api.Lookup { defer c.stats.bumpLookups() lookup := api.Lookup{} - if c.failure == nil { - lookup.Name = <-c.domain - } else { + if c.failure != nil { lookup.Error = c.failure + return lookup } + lookup.Name = <-c.domain + return lookup } @@ -74,12 +75,9 @@ func (c *client) Status() error { } func withTestCache(config resolverConfig, cacheConfig *cacheConfig, block func(*Cache, *client)) { - var chanSize int - + chanSize := 0 if config.buffered { chanSize = 1 - } else { - chanSize = 0 } resolver := &client{ @@ -252,12 +250,11 @@ func TestResolve(t *testing.T) { withTestCache(resolverConfig{}, nil, func(cache *Cache, resolver *client) { cache.withTestEntry(entryConfig{expired: false, retrieved: false}, func(entry *Entry) { ctx, cancel := context.WithCancel(context.Background()) + cancel() // <- test purpose response := make(chan *api.Lookup, 1) go func() { response <- cache.Resolve(ctx, "my.gitlab.com") }() - cancel() - resolver.domain <- "my.gitlab.com" lookup := <-response diff --git a/internal/source/gitlab/cache/entry.go b/internal/source/gitlab/cache/entry.go index 162d0d3b1..ca8b9817c 100644 --- a/internal/source/gitlab/cache/entry.go +++ b/internal/source/gitlab/cache/entry.go @@ -65,14 +65,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 func() { - // `e.setResponse` closes the `e.retrieved` channel when done. - // allocating a new anonymous function gives the runtime enough time to process the context.Cancel - // for TestResolve/when_retrieval_failed_because_of_resolution_context_being_canceled - e.setResponse(e.retriever.Retrieve(e.domain)) - }() - }) + e.retrieve.Do(func() { go e.retrieveAndSetResponse() }) select { case <-ctx.Done(): @@ -84,6 +77,13 @@ func (e *Entry) Retrieve(ctx context.Context, client api.Client) (lookup *api.Lo return lookup } +// retrieveAndSetResponse done on a separate function to avoid some intermittent failures +// with TestResolve/when_retrieval_failed_because_of_resolution_context_being_canceled +// see https://gitlab.com/gitlab-org/gitlab-pages/-/merge_requests/323#note_394716942 +func (e *Entry) retrieveAndSetResponse() { + e.setResponse(e.retriever.Retrieve(e.domain)) +} + // Refresh will update the entry in the store only when it gets resolved. func (e *Entry) Refresh(client api.Client, store Store) { e.refresh.Do(func() { -- GitLab