From b07f95ef9335d4fee673268b1371b1c281b224cd Mon Sep 17 00:00:00 2001 From: Dmitri Shuralyov Date: Fri, 22 Apr 2016 02:55:46 -0700 Subject: [PATCH] Predict *RateLimitError, return immediately without network call. When possible to do so reliably, i.e., the client knows that the rate limit is exceeded and reset time is in the future, immediately return *RateLimitError without making network calls to GitHub API. Add a unit test covering RateLimits method populating client.rateLimits. Remove commented out code. Helps #152 and #153. --- github/github.go | 35 +++++++++++++++++++++++- github/github_test.go | 62 ++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 95 insertions(+), 2 deletions(-) diff --git a/github/github.go b/github/github.go index e9404b5..b2de909 100644 --- a/github/github.go +++ b/github/github.go @@ -333,10 +333,16 @@ func (c *Client) Rate() Rate { // JSON decoded and stored in the value pointed to by v, or returned as an // error if an API error has occurred. If v implements the io.Writer // interface, the raw response body will be written to v, without attempting to -// first decode it. +// first decode it. If rate limit is exceeded and reset time is in the future, +// Do returns *RateLimitError immediately without making a network API call. func (c *Client) Do(req *http.Request, v interface{}) (*Response, error) { rateLimitCategory := category(req.URL.Path) + // If we've hit rate limit, don't make further requests before Reset time. + if err := c.checkRateLimitBeforeDo(req, rateLimitCategory); err != nil { + return nil, err + } + resp, err := c.client.Do(req) if err != nil { return nil, err @@ -376,6 +382,33 @@ func (c *Client) Do(req *http.Request, v interface{}) (*Response, error) { return response, err } +// checkRateLimitBeforeDo does not make any network calls, but uses existing knowledge from +// current client state in order to quickly check if *RateLimitError can be immediately returned +// from Client.Do, and if so, returns it so that Client.Do can skip making a network API call unneccessarily. +// Otherwise it returns nil, and Client.Do should proceed normally. +func (c *Client) checkRateLimitBeforeDo(req *http.Request, rateLimitCategory rateLimitCategory) error { + c.rateMu.Lock() + rate := c.rateLimits[rateLimitCategory] + c.rateMu.Unlock() + if !rate.Reset.Time.IsZero() && rate.Remaining == 0 && time.Now().Before(rate.Reset.Time) { + // Create a fake response. + resp := &http.Response{ + Status: http.StatusText(http.StatusForbidden), + StatusCode: http.StatusForbidden, + Request: req, + Header: make(http.Header), + Body: ioutil.NopCloser(strings.NewReader("")), + } + return &RateLimitError{ + Rate: rate, + Response: resp, + Message: fmt.Sprintf("API rate limit of %v still exceeded until %v, not making remote request.", rate.Limit, rate.Reset.Time), + } + } + + return nil +} + /* An ErrorResponse reports one or more errors caused by an API request. diff --git a/github/github_test.go b/github/github_test.go index 867d23a..521f12a 100644 --- a/github/github_test.go +++ b/github/github_test.go @@ -485,6 +485,60 @@ func TestDo_rateLimit_rateLimitError(t *testing.T) { } } +// Ensure a network call is not made when it's known that API rate limit is still exceeded. +func TestDo_rateLimit_noNetworkCall(t *testing.T) { + setup() + defer teardown() + + reset := time.Now().UTC().Round(time.Second).Add(time.Minute) // Rate reset is a minute from now, with 1 second precision. + + mux.HandleFunc("/first", func(w http.ResponseWriter, r *http.Request) { + w.Header().Add(headerRateLimit, "60") + w.Header().Add(headerRateRemaining, "0") + w.Header().Add(headerRateReset, fmt.Sprint(reset.Unix())) + w.Header().Set("Content-Type", "application/json; charset=utf-8") + w.WriteHeader(http.StatusForbidden) + fmt.Fprintln(w, `{ + "message": "API rate limit exceeded for xxx.xxx.xxx.xxx. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)", + "documentation_url": "https://developer.github.com/v3/#rate-limiting" +}`) + }) + + madeNetworkCall := false + mux.HandleFunc("/second", func(w http.ResponseWriter, r *http.Request) { + madeNetworkCall = true + }) + + // First request is made, and it makes the client aware of rate reset time being in the future. + req, _ := client.NewRequest("GET", "/first", nil) + client.Do(req, nil) + + // Second request should not cause a network call to be made, since client can predict a rate limit error. + req, _ = client.NewRequest("GET", "/second", nil) + _, err := client.Do(req, nil) + + if madeNetworkCall { + t.Fatal("Network call was made, even though rate limit is known to still be exceeded.") + } + + if err == nil { + t.Error("Expected error to be returned.") + } + rateLimitErr, ok := err.(*RateLimitError) + if !ok { + t.Fatalf("Expected a *RateLimitError error; got %#v.", err) + } + if got, want := rateLimitErr.Rate.Limit, 60; got != want { + t.Errorf("rateLimitErr rate limit = %v, want %v", got, want) + } + if got, want := rateLimitErr.Rate.Remaining, 0; got != want { + t.Errorf("rateLimitErr rate remaining = %v, want %v", got, want) + } + if rateLimitErr.Rate.Reset.UTC() != reset { + t.Errorf("rateLimitErr rate reset = %v, want %v", rateLimitErr.Rate.Reset.UTC(), reset) + } +} + func TestDo_noContent(t *testing.T) { setup() defer teardown() @@ -635,7 +689,6 @@ func TestRateLimit(t *testing.T) { if m := "GET"; m != r.Method { t.Errorf("Request method = %v, want %v", r.Method, m) } - //fmt.Fprint(w, `{"resources":{"core": {"limit":2,"remaining":1,"reset":1372700873}}}`) fmt.Fprint(w, `{"resources":{ "core": {"limit":2,"remaining":1,"reset":1372700873}, "search": {"limit":3,"remaining":2,"reset":1372700874} @@ -691,6 +744,13 @@ func TestRateLimits(t *testing.T) { if !reflect.DeepEqual(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) + } } func TestUnauthenticatedRateLimitedTransport(t *testing.T) {