From 3e0d895e24f014920ec83084e8b296feb3b73b74 Mon Sep 17 00:00:00 2001 From: Patrick Rice Date: Mon, 19 Aug 2024 14:10:53 +0000 Subject: [PATCH 1/4] Add an exponential backoff to the retry function --- gitlab.go | 6 +++++- gitlab_test.go | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/gitlab.go b/gitlab.go index db6041726..3c69d8069 100644 --- a/gitlab.go +++ b/gitlab.go @@ -24,6 +24,7 @@ import ( "errors" "fmt" "io" + "math" "math/rand" "mime/multipart" "net/http" @@ -509,7 +510,7 @@ func (c *Client) retryHTTPBackoff(min, max time.Duration, attemptNum int, resp * // min and max are mainly used for bounding the jitter that will be added to // the reset time retrieved from the headers. But if the final wait time is // less then min, min will be used instead. -func rateLimitBackoff(min, max time.Duration, _ int, resp *http.Response) time.Duration { +func rateLimitBackoff(min, max time.Duration, attemptNum int, resp *http.Response) time.Duration { // rnd is used to generate pseudo-random numbers. rnd := rand.New(rand.NewSource(time.Now().UnixNano())) @@ -527,6 +528,9 @@ func rateLimitBackoff(min, max time.Duration, _ int, resp *http.Response) time.D } } + // For each attempt, back off an additiona 100% exponentially. With the default milliseconds + // being set to 100 for `min`, this makes the 5th retry wait 3.2 seconds (3,200 ms) by default + min = time.Duration(float64(min) * math.Pow(2, float64(attemptNum))) return min + jitter } diff --git a/gitlab_test.go b/gitlab_test.go index d6e027321..02a69b18c 100644 --- a/gitlab_test.go +++ b/gitlab_test.go @@ -418,3 +418,36 @@ func TestPaginationPopulatePageValuesKeyset(t *testing.T) { } } } + +func TestExponentialBackoffLogic(t *testing.T) { + // Can't use the default `setup` because it disabled the backoff + mux := http.NewServeMux() + server := httptest.NewServer(mux) + t.Cleanup(server.Close) + client, err := NewClient("", + WithBaseURL(server.URL), + ) + if err != nil { + t.Fatalf("Failed to create client: %v", err) + } + + // Create a method that returns 429 + mux.HandleFunc("/api/v4/projects/1", func(w http.ResponseWriter, r *http.Request) { + testMethod(t, r, http.MethodGet) + w.WriteHeader(http.StatusTooManyRequests) + }) + + // Measure the time at the start of the test + start := time.Now() + + // Send a request (which will get a bunch of 429s) + // None of the responses matter, so ignore them all + _, _, _ = client.Projects.GetProject(1, nil) + end := time.Now() + + // The test should run for _at least_ 3,200 milliseconds + duration := float64(end.Sub(start)) + if duration < float64(3200*time.Millisecond) { + t.Fatal("Wait was shorter than expected. Expected a minimum of 5 retries taking 3200 milliseconds, got:", duration) + } +} From 07b7c25b5b87d0f358f985d386e0295e12ca3c19 Mon Sep 17 00:00:00 2001 From: Patrick Rice Date: Mon, 19 Aug 2024 14:22:05 +0000 Subject: [PATCH 2/4] Fix linting error --- gitlab_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/gitlab_test.go b/gitlab_test.go index 02a69b18c..b07631fd5 100644 --- a/gitlab_test.go +++ b/gitlab_test.go @@ -442,7 +442,7 @@ func TestExponentialBackoffLogic(t *testing.T) { // Send a request (which will get a bunch of 429s) // None of the responses matter, so ignore them all - _, _, _ = client.Projects.GetProject(1, nil) + _, resp, _ := client.Projects.GetProject(1, nil) end := time.Now() // The test should run for _at least_ 3,200 milliseconds @@ -450,4 +450,7 @@ func TestExponentialBackoffLogic(t *testing.T) { if duration < float64(3200*time.Millisecond) { t.Fatal("Wait was shorter than expected. Expected a minimum of 5 retries taking 3200 milliseconds, got:", duration) } + if resp.StatusCode != 429 { + t.Fatal("Expected to get a 429 code given the server is hard-coded to return this. Received instead:", resp.StatusCode) + } } From 65c324f4f2206cd129851d58fb386fb9159af39e Mon Sep 17 00:00:00 2001 From: Patrick Rice Date: Sat, 24 Aug 2024 16:17:16 +0000 Subject: [PATCH 3/4] Update backoff to only apply when the header isn't set --- gitlab.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/gitlab.go b/gitlab.go index 3c69d8069..a91c9b4f8 100644 --- a/gitlab.go +++ b/gitlab.go @@ -525,12 +525,13 @@ func rateLimitBackoff(min, max time.Duration, attemptNum int, resp *http.Respons min = wait } } + } else { + // For each attempt without the header, back off an additiona 100% exponentially. With the default milliseconds + // being set to 100 for `min`, this makes the 5th retry wait 3.2 seconds (3,200 ms) by default + min = time.Duration(float64(min) * math.Pow(2, float64(attemptNum))) } } - // For each attempt, back off an additiona 100% exponentially. With the default milliseconds - // being set to 100 for `min`, this makes the 5th retry wait 3.2 seconds (3,200 ms) by default - min = time.Duration(float64(min) * math.Pow(2, float64(attemptNum))) return min + jitter } From cd5f603fcb22819c6976cfa7862e7eb30f8717bf Mon Sep 17 00:00:00 2001 From: Sander van Harmelen Date: Mon, 26 Aug 2024 09:49:11 +0200 Subject: [PATCH 4/4] Update the comment --- gitlab.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/gitlab.go b/gitlab.go index a91c9b4f8..7d1a45739 100644 --- a/gitlab.go +++ b/gitlab.go @@ -526,8 +526,9 @@ func rateLimitBackoff(min, max time.Duration, attemptNum int, resp *http.Respons } } } else { - // For each attempt without the header, back off an additiona 100% exponentially. With the default milliseconds - // being set to 100 for `min`, this makes the 5th retry wait 3.2 seconds (3,200 ms) by default + // In case the RateLimit-Reset header is not set, back off an additional + // 100% exponentially. With the default milliseconds being set to 100 for + // `min`, this makes the 5th retry wait 3.2 seconds (3,200 ms) by default. min = time.Duration(float64(min) * math.Pow(2, float64(attemptNum))) } }