diff --git a/internal/mocks/mocks.go b/internal/mocks/mocks.go index e1f0f6d7ee08247b4ade38a0f62249dca9e8a5f7..2816205dfe42b46d2b30cb90db40d663c76d276c 100644 --- a/internal/mocks/mocks.go +++ b/internal/mocks/mocks.go @@ -5,10 +5,9 @@ package mocks import ( + gomock "github.com/golang/mock/gomock" http "net/http" reflect "reflect" - - gomock "github.com/golang/mock/gomock" ) // MockArtifact is a mock of Artifact interface diff --git a/internal/source/gitlab/cache/retriever.go b/internal/source/gitlab/cache/retriever.go index 865bf88be3677eb37a00fb9a4e58e79adabf6dd8..a3c060125e817b5ba3230625a5d85aa019afddb4 100644 --- a/internal/source/gitlab/cache/retriever.go +++ b/internal/source/gitlab/cache/retriever.go @@ -7,9 +7,7 @@ import ( log "github.com/sirupsen/logrus" - "gitlab.com/gitlab-org/gitlab-pages/internal/domain" "gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/api" - "gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/client" ) // Retriever is an utility type that performs an HTTP request with backoff in @@ -48,21 +46,22 @@ func (r *Retriever) Retrieve(domain string) (lookup api.Lookup) { return lookup } -func (r *Retriever) resolveWithBackoff(ctx context.Context, domainName string) <-chan api.Lookup { - response := make(chan api.Lookup) +func (r *Retriever) resolveWithBackoff(ctx context.Context, domain string) <-chan api.Lookup { + response := make(chan api.Lookup, 8) go func() { var lookup api.Lookup for i := 1; i <= r.maxRetrievalRetries; i++ { - lookup = r.client.GetLookup(ctx, domainName) - if lookup.Error == nil || errors.Is(lookup.Error, domain.ErrDomainDoesNotExist) || - errors.Is(lookup.Error, client.ErrUnauthorizedAPI) { - // do not retry if the domain does not exist or there is an auth error + lookup = r.client.GetLookup(ctx, domain) + + if lookup.Error != nil { + if !sleepWithContext(ctx, r.maxRetrievalInterval) { + break + } + } else { break } - - time.Sleep(r.maxRetrievalInterval) } response <- lookup @@ -71,3 +70,14 @@ func (r *Retriever) resolveWithBackoff(ctx context.Context, domainName string) < return response } + +func sleepWithContext(ctx context.Context, d time.Duration) bool { + t := time.NewTimer(d) + select { + case <-ctx.Done(): + t.Stop() + return false + case <-t.C: + return true + } +} diff --git a/internal/source/gitlab/cache/retriever_test.go b/internal/source/gitlab/cache/retriever_test.go new file mode 100644 index 0000000000000000000000000000000000000000..b77591dbce9cb68e131557af75892151b3fd4db6 --- /dev/null +++ b/internal/source/gitlab/cache/retriever_test.go @@ -0,0 +1,27 @@ +package cache + +import ( + "errors" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +func TestRetrieveTimerStopsWhenContextIsDone(t *testing.T) { + retrievalTimeout := time.Millisecond // quick timeout that cancels inner context + maxRetrievalInterval := time.Minute // long sleep inside resolveWithBackoff + + resolver := &client{ + domain: make(chan string), + lookups: make(chan uint64, 1), + failure: errors.New("500 error"), + } + + retriever := NewRetriever(resolver, retrievalTimeout, maxRetrievalInterval, 3) + // require.False(t, retriever.timer.hasStopped(), "timer has not been stopped yet") + + lookup := retriever.Retrieve("my.gitlab.com") + require.Empty(t, lookup.Name) + require.Eventually(t, retriever, time.Second, time.Millisecond, "timer must have been stopped") +}