diff --git a/README.md b/README.md index 0e76752c82b..461ae4d02da 100644 --- a/README.md +++ b/README.md @@ -186,6 +186,24 @@ if _, ok := err.(*github.RateLimitError); ok { Learn more about GitHub rate limiting at https://docs.github.com/en/rest/rate-limit . +In addition to these rate limits, GitHub imposes a secondary rate limit on all API clients. +This rate limit prevents clients from making too many concurrent requests. + +To detect an API secondary rate limit error, you can check if its type is `*github.AbuseRateLimitError`: + +```go +repos, _, err := client.Repositories.List(ctx, "", nil) +if _, ok := err.(*github.AbuseRateLimitError); ok { + log.Println("hit secondary rate limit") +} +``` + +You can use [go-github-ratelimit](https://github.com/gofri/go-github-ratelimit) to handle +secondary rate limit sleep-and-retry for you. + +Learn more about GitHub secondary rate limiting at +https://docs.github.com/en/rest/overview/resources-in-the-rest-api#secondary-rate-limits . + ### Accepted Status ### Some endpoints may return a 202 Accepted status code, meaning that the diff --git a/example/go.mod b/example/go.mod index 85db0c81f3b..e268c7ad626 100644 --- a/example/go.mod +++ b/example/go.mod @@ -4,6 +4,7 @@ go 1.17 require ( github.com/bradleyfalzon/ghinstallation/v2 v2.0.4 + github.com/gofri/go-github-ratelimit v1.0.1 github.com/google/go-github/v50 v50.0.0 golang.org/x/crypto v0.0.0-20210817164053-32db794688a5 golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be diff --git a/example/go.sum b/example/go.sum index 663dff29860..1043059ffa2 100644 --- a/example/go.sum +++ b/example/go.sum @@ -1,5 +1,7 @@ github.com/bradleyfalzon/ghinstallation/v2 v2.0.4 h1:tXKVfhE7FcSkhkv0UwkLvPDeZ4kz6OXd0PKPlFqf81M= github.com/bradleyfalzon/ghinstallation/v2 v2.0.4/go.mod h1:B40qPqJxWE0jDZgOR1JmaMy+4AY1eBP+IByOvqyAKp0= +github.com/gofri/go-github-ratelimit v1.0.1 h1:sgefSzxhnvwZ+wR9uZ4l9TnjgLuNiwipJVzJL4YLj9A= +github.com/gofri/go-github-ratelimit v1.0.1/go.mod h1:OnCi5gV+hAG/LMR7llGhU7yHt44se9sYgKPnafoL7RY= github.com/golang-jwt/jwt/v4 v4.0.0 h1:RAqyYixv1p7uEnocuy8P1nru5wprCh/MH2BIlW5z5/o= github.com/golang-jwt/jwt/v4 v4.0.0/go.mod h1:/xlHOz8bRuivTWchD4jCa+NbatV+wEUSzwAxVc6locg= github.com/golang/protobuf v1.3.1/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U= diff --git a/example/ratelimit/main.go b/example/ratelimit/main.go new file mode 100644 index 00000000000..228099faa9f --- /dev/null +++ b/example/ratelimit/main.go @@ -0,0 +1,42 @@ +// Copyright 2023 The go-github AUTHORS. All rights reserved. +// +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// The ratelimit command demonstrates using the github_ratelimit.SecondaryRateLimitWaiter. +// By using the waiter, the client automatically sleeps and retry requests +// when it hits secondary rate limits. +package main + +import ( + "context" + "fmt" + + "github.com/gofri/go-github-ratelimit/github_ratelimit" + "github.com/google/go-github/v50/github" +) + +func main() { + var username string + fmt.Print("Enter GitHub username: ") + fmt.Scanf("%s", &username) + + rateLimiter, err := github_ratelimit.NewRateLimitWaiterClient(nil) + if err != nil { + fmt.Printf("Error: %v\n", err) + return + } + + client := github.NewClient(rateLimiter) + + // arbitrary usage of the client + organizations, _, err := client.Organizations.List(context.Background(), username, nil) + if err != nil { + fmt.Printf("Error: %v\n", err) + return + } + + for i, organization := range organizations { + fmt.Printf("%v. %v\n", i+1, organization.GetLogin()) + } +} diff --git a/github/code-scanning_test.go b/github/code-scanning_test.go index 1ae05096d3f..7911fca6360 100644 --- a/github/code-scanning_test.go +++ b/github/code-scanning_test.go @@ -83,7 +83,7 @@ func TestCodeScanningService_UploadSarif(t *testing.T) { return err }) - testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) { + testNewRequestAndDoFailureCategory(t, methodName, client, codeScanningUploadCategory, func() (*Response, error) { _, resp, err := client.CodeScanning.UploadSarif(ctx, "o", "r", sarifAnalysis) return resp, err }) diff --git a/github/github.go b/github/github.go index 629218165c7..6b37f082cf6 100644 --- a/github/github.go +++ b/github/github.go @@ -171,8 +171,9 @@ type Client struct { // User agent used when communicating with the GitHub API. UserAgent string - rateMu sync.Mutex - rateLimits [categories]Rate // Rate limits for the client as determined by the most recent API calls. + rateMu sync.Mutex + rateLimits [categories]Rate // Rate limits for the client as determined by the most recent API calls. + secondaryRateLimitReset time.Time // Secondary rate limit reset for the client as determined by the most recent API calls. common service // Reuse a single struct instead of allocating one for each service on the heap. @@ -702,7 +703,7 @@ func (c *Client) BareDo(ctx context.Context, req *http.Request) (*Response, erro req = withContext(ctx, req) - rateLimitCategory := category(req.URL.Path) + rateLimitCategory := category(req.Method, req.URL.Path) if bypass := ctx.Value(bypassRateLimitCheck); bypass == nil { // If we've hit rate limit, don't make further requests before Reset time. @@ -712,6 +713,12 @@ func (c *Client) BareDo(ctx context.Context, req *http.Request) (*Response, erro Rate: err.Rate, }, err } + // If we've hit a secondary rate limit, don't make further requests before Retry After. + if err := c.checkSecondaryRateLimitBeforeDo(ctx, req); err != nil { + return &Response{ + Response: err.Response, + }, err + } } resp, err := c.client.Do(req) @@ -763,6 +770,14 @@ func (c *Client) BareDo(ctx context.Context, req *http.Request) (*Response, erro aerr.Raw = b err = aerr } + + // Update the secondary rate limit if we hit it. + rerr, ok := err.(*AbuseRateLimitError) + if ok && rerr.RetryAfter != nil { + c.rateMu.Lock() + c.secondaryRateLimitReset = time.Now().Add(*rerr.RetryAfter) + c.rateMu.Unlock() + } } return response, err } @@ -827,6 +842,35 @@ func (c *Client) checkRateLimitBeforeDo(req *http.Request, rateLimitCategory rat return nil } +// checkSecondaryRateLimitBeforeDo does not make any network calls, but uses existing knowledge from +// current client state in order to quickly check if *AbuseRateLimitError can be immediately returned +// from Client.Do, and if so, returns it so that Client.Do can skip making a network API call unnecessarily. +// Otherwise it returns nil, and Client.Do should proceed normally. +func (c *Client) checkSecondaryRateLimitBeforeDo(ctx context.Context, req *http.Request) *AbuseRateLimitError { + c.rateMu.Lock() + secondary := c.secondaryRateLimitReset + c.rateMu.Unlock() + if !secondary.IsZero() && time.Now().Before(secondary) { + // Create a fake response. + resp := &http.Response{ + Status: http.StatusText(http.StatusForbidden), + StatusCode: http.StatusForbidden, + Request: req, + Header: make(http.Header), + Body: io.NopCloser(strings.NewReader("")), + } + + retryAfter := time.Until(secondary) + return &AbuseRateLimitError{ + Response: resp, + Message: fmt.Sprintf("API secondary rate limit exceeded until %v, not making remote request.", secondary), + RetryAfter: &retryAfter, + } + } + + return nil +} + // compareHTTPResponse returns whether two http.Response objects are equal or not. // Currently, only StatusCode is checked. This function is used when implementing the // Is(error) bool interface for the custom error types in this package. @@ -1197,13 +1241,36 @@ const ( categories // An array of this length will be able to contain all rate limit categories. ) -// category returns the rate limit category of the endpoint, determined by Request.URL.Path. -func category(path string) rateLimitCategory { +// category returns the rate limit category of the endpoint, determined by HTTP method and Request.URL.Path. +func category(method, path string) rateLimitCategory { switch { + // https://docs.github.com/en/rest/rate-limit#about-rate-limits default: + // NOTE: coreCategory is returned for actionsRunnerRegistrationCategory too, + // because no API found for this category. return coreCategory case strings.HasPrefix(path, "/search/"): return searchCategory + case path == "/graphql": + return graphqlCategory + case strings.HasPrefix(path, "/app-manifests/") && + strings.HasSuffix(path, "/conversions") && + method == http.MethodPost: + return integrationManifestCategory + + // https://docs.github.com/en/rest/migrations/source-imports#start-an-import + case strings.HasPrefix(path, "/repos/") && + strings.HasSuffix(path, "/import") && + method == http.MethodPut: + return sourceImportCategory + + // https://docs.github.com/en/rest/code-scanning#upload-an-analysis-as-sarif-data + case strings.HasSuffix(path, "/code-scanning/sarifs"): + return codeScanningUploadCategory + + // https://docs.github.com/en/enterprise-cloud@latest/rest/scim + case strings.HasPrefix(path, "/scim/"): + return scimCategory } } diff --git a/github/github_test.go b/github/github_test.go index 50ecd5435bc..7342e2dfd79 100644 --- a/github/github_test.go +++ b/github/github_test.go @@ -185,6 +185,11 @@ func testBadOptions(t *testing.T, methodName string, f func() error) { // Method f should be a regular call that would normally succeed, but // should return an error when NewRequest or s.client.Do fails. func testNewRequestAndDoFailure(t *testing.T, methodName string, client *Client, f func() (*Response, error)) { + testNewRequestAndDoFailureCategory(t, methodName, client, coreCategory, f) +} + +// testNewRequestAndDoFailureCategory works Like testNewRequestAndDoFailure, but allows setting the category +func testNewRequestAndDoFailureCategory(t *testing.T, methodName string, client *Client, category rateLimitCategory, f func() (*Response, error)) { t.Helper() if methodName == "" { t.Error("testNewRequestAndDoFailure: must supply method methodName") @@ -200,7 +205,7 @@ func testNewRequestAndDoFailure(t *testing.T, methodName string, client *Client, } client.BaseURL.Path = "/api-v3/" - client.rateLimits[0].Reset.Time = time.Now().Add(10 * time.Minute) + client.rateLimits[category].Reset.Time = time.Now().Add(10 * time.Minute) resp, err = f() if bypass := resp.Request.Context().Value(bypassRateLimitCheck); bypass != nil { return @@ -1136,6 +1141,67 @@ func TestDo_rateLimit(t *testing.T) { } } +func TestDo_rateLimitCategory(t *testing.T) { + tests := []struct { + method string + url string + category rateLimitCategory + }{ + { + method: http.MethodGet, + url: "/", + category: coreCategory, + }, + { + method: http.MethodGet, + url: "/search/issues?q=rate", + category: searchCategory, + }, + { + method: http.MethodGet, + url: "/graphql", + category: graphqlCategory, + }, + { + method: http.MethodPost, + url: "/app-manifests/code/conversions", + category: integrationManifestCategory, + }, + { + method: http.MethodGet, + url: "/app-manifests/code/conversions", + category: coreCategory, // only POST requests are in the integration manifest category + }, + { + method: http.MethodPut, + url: "/repos/google/go-github/import", + category: sourceImportCategory, + }, + { + method: http.MethodGet, + url: "/repos/google/go-github/import", + category: coreCategory, // only PUT requests are in the source import category + }, + { + method: http.MethodPost, + url: "/repos/google/go-github/code-scanning/sarifs", + category: codeScanningUploadCategory, + }, + { + method: http.MethodGet, + url: "/scim/v2/organizations/ORG/Users", + category: scimCategory, + }, + // missing a check for actionsRunnerRegistrationCategory: API not found + } + + for _, tt := range tests { + if got, want := category(tt.method, tt.url), tt.category; got != want { + t.Errorf("expecting category %v, found %v", got, want) + } + } +} + // ensure rate limit is still parsed, even for error responses func TestDo_rateLimit_errorResponse(t *testing.T) { client, mux, _, teardown := setup() @@ -1407,6 +1473,25 @@ func TestDo_rateLimit_abuseRateLimitError_retryAfter(t *testing.T) { if got, want := *abuseRateLimitErr.RetryAfter, 123*time.Second; got != want { t.Errorf("abuseRateLimitErr RetryAfter = %v, want %v", got, want) } + + // expect prevention of a following request + if _, err = client.Do(ctx, req, nil); err == nil { + t.Error("Expected error to be returned.") + } + abuseRateLimitErr, ok = err.(*AbuseRateLimitError) + if !ok { + t.Fatalf("Expected a *AbuseRateLimitError error; got %#v.", err) + } + if abuseRateLimitErr.RetryAfter == nil { + t.Fatalf("abuseRateLimitErr RetryAfter is nil, expected not-nil") + } + // the saved duration might be a bit smaller than Retry-After because the duration is calculated from the expected end-of-cooldown time + if got, want := *abuseRateLimitErr.RetryAfter, 123*time.Second; want-got > 1*time.Second { + t.Errorf("abuseRateLimitErr RetryAfter = %v, want %v", got, want) + } + if got, wantSuffix := abuseRateLimitErr.Message, "not making remote request."; !strings.HasSuffix(got, wantSuffix) { + t.Errorf("Expected request to be prevented because of secondary rate limit, got: %v.", got) + } } func TestDo_noContent(t *testing.T) { @@ -2065,30 +2150,48 @@ func TestRateLimits(t *testing.T) { if !cmp.Equal(rate, want) { t.Errorf("RateLimits returned %+v, want %+v", rate, want) } - - if got, want := client.rateLimits[coreCategory], *want.Core; got != want { - t.Errorf("client.rateLimits[coreCategory] is %+v, want %+v", got, want) - } - if got, want := client.rateLimits[searchCategory], *want.Search; got != want { - t.Errorf("client.rateLimits[searchCategory] is %+v, want %+v", got, want) - } - if got, want := client.rateLimits[graphqlCategory], *want.GraphQL; got != want { - t.Errorf("client.rateLimits[graphqlCategory] is %+v, want %+v", got, want) - } - if got, want := client.rateLimits[integrationManifestCategory], *want.IntegrationManifest; got != want { - t.Errorf("client.rateLimits[integrationManifestCategory] is %+v, want %+v", got, want) - } - if got, want := client.rateLimits[sourceImportCategory], *want.SourceImport; got != want { - t.Errorf("client.rateLimits[sourceImportCategory] is %+v, want %+v", got, want) - } - if got, want := client.rateLimits[codeScanningUploadCategory], *want.CodeScanningUpload; got != want { - t.Errorf("client.rateLimits[codeScanningUploadCategory] is %+v, want %+v", got, want) - } - if got, want := client.rateLimits[actionsRunnerRegistrationCategory], *want.ActionsRunnerRegistration; got != want { - t.Errorf("client.rateLimits[actionsRunnerRegistrationCategory] is %+v, want %+v", got, want) + tests := []struct { + category rateLimitCategory + rate *Rate + }{ + { + category: coreCategory, + rate: want.Core, + }, + { + category: searchCategory, + rate: want.Search, + }, + { + category: graphqlCategory, + rate: want.GraphQL, + }, + { + category: integrationManifestCategory, + rate: want.IntegrationManifest, + }, + { + category: sourceImportCategory, + rate: want.SourceImport, + }, + { + category: codeScanningUploadCategory, + rate: want.CodeScanningUpload, + }, + { + category: actionsRunnerRegistrationCategory, + rate: want.ActionsRunnerRegistration, + }, + { + category: scimCategory, + rate: want.SCIM, + }, } - if got, want := client.rateLimits[scimCategory], *want.SCIM; got != want { - t.Errorf("client.rateLimits[scimCategory] is %+v, want %+v", got, want) + + for _, tt := range tests { + if got, want := client.rateLimits[tt.category], *tt.rate; got != want { + t.Errorf("client.rateLimits[%v] is %+v, want %+v", tt.category, got, want) + } } } @@ -2117,7 +2220,13 @@ func TestRateLimits_overQuota(t *testing.T) { mux.HandleFunc("/rate_limit", func(w http.ResponseWriter, r *http.Request) { fmt.Fprint(w, `{"resources":{ "core": {"limit":2,"remaining":1,"reset":1372700873}, - "search": {"limit":3,"remaining":2,"reset":1372700874} + "search": {"limit":3,"remaining":2,"reset":1372700874}, + "graphql": {"limit":4,"remaining":3,"reset":1372700875}, + "integration_manifest": {"limit":5,"remaining":4,"reset":1372700876}, + "source_import": {"limit":6,"remaining":5,"reset":1372700877}, + "code_scanning_upload": {"limit":7,"remaining":6,"reset":1372700878}, + "actions_runner_registration": {"limit":8,"remaining":7,"reset":1372700879}, + "scim": {"limit":9,"remaining":8,"reset":1372700880} }}`) }) @@ -2138,16 +2247,82 @@ func TestRateLimits_overQuota(t *testing.T) { Remaining: 2, Reset: Timestamp{time.Date(2013, time.July, 1, 17, 47, 54, 0, time.UTC).Local()}, }, + GraphQL: &Rate{ + Limit: 4, + Remaining: 3, + Reset: Timestamp{time.Date(2013, time.July, 1, 17, 47, 55, 0, time.UTC).Local()}, + }, + IntegrationManifest: &Rate{ + Limit: 5, + Remaining: 4, + Reset: Timestamp{time.Date(2013, time.July, 1, 17, 47, 56, 0, time.UTC).Local()}, + }, + SourceImport: &Rate{ + Limit: 6, + Remaining: 5, + Reset: Timestamp{time.Date(2013, time.July, 1, 17, 47, 57, 0, time.UTC).Local()}, + }, + CodeScanningUpload: &Rate{ + Limit: 7, + Remaining: 6, + Reset: Timestamp{time.Date(2013, time.July, 1, 17, 47, 58, 0, time.UTC).Local()}, + }, + ActionsRunnerRegistration: &Rate{ + Limit: 8, + Remaining: 7, + Reset: Timestamp{time.Date(2013, time.July, 1, 17, 47, 59, 0, time.UTC).Local()}, + }, + SCIM: &Rate{ + Limit: 9, + Remaining: 8, + Reset: Timestamp{time.Date(2013, time.July, 1, 17, 48, 00, 0, time.UTC).Local()}, + }, } if !cmp.Equal(rate, want) { t.Errorf("RateLimits returned %+v, want %+v", rate, want) } - if got, want := client.rateLimits[coreCategory], *want.Core; got != want { - t.Errorf("client.rateLimits[coreCategory] is %+v, want %+v", got, want) + tests := []struct { + category rateLimitCategory + rate *Rate + }{ + { + category: coreCategory, + rate: want.Core, + }, + { + category: searchCategory, + rate: want.Search, + }, + { + category: graphqlCategory, + rate: want.GraphQL, + }, + { + category: integrationManifestCategory, + rate: want.IntegrationManifest, + }, + { + category: sourceImportCategory, + rate: want.SourceImport, + }, + { + category: codeScanningUploadCategory, + rate: want.CodeScanningUpload, + }, + { + category: actionsRunnerRegistrationCategory, + rate: want.ActionsRunnerRegistration, + }, + { + category: scimCategory, + rate: want.SCIM, + }, } - if got, want := client.rateLimits[searchCategory], *want.Search; got != want { - t.Errorf("client.rateLimits[searchCategory] is %+v, want %+v", got, want) + for _, tt := range tests { + if got, want := client.rateLimits[tt.category], *tt.rate; got != want { + t.Errorf("client.rateLimits[%v] is %+v, want %+v", tt.category, got, want) + } } }