From e5099d75375212809d7ccedb61fbbcb2976cff0c Mon Sep 17 00:00:00 2001 From: Dmitri Shuralyov Date: Mon, 28 Dec 2015 20:38:37 -0800 Subject: [PATCH] Fix data race surrounding Client.Rate. Previously, concurrent calls to Client.Do would have data races writing Client.Rate. Protect it with a mutex. Resolves #253. --- github/github.go | 23 +++++++++++++++++------ github/github_test.go | 24 ++++++++++++------------ 2 files changed, 29 insertions(+), 18 deletions(-) diff --git a/github/github.go b/github/github.go index 30b8390..76939f8 100644 --- a/github/github.go +++ b/github/github.go @@ -17,6 +17,7 @@ import ( "reflect" "strconv" "strings" + "sync" "time" "github.com/google/go-querystring/query" @@ -64,11 +65,8 @@ type Client struct { // User agent used when communicating with the GitHub API. UserAgent string - // Rate specifies the current rate limit for the client as determined by the - // most recent API call. If the client is used in a multi-user application, - // this rate may not always be up-to-date. Call RateLimits() to check the - // current rate. - Rate Rate + rateMu sync.Mutex + rate Rate // Services used for talking to different parts of the GitHub API. Activity *ActivityService @@ -292,6 +290,17 @@ func (r *Response) populateRate() { } } +// Rate specifies the current rate limit for the client as determined by the +// most recent API call. If the client is used in a multi-user application, +// this rate may not always be up-to-date. Call RateLimits() to check the +// current rate. +func (c *Client) Rate() Rate { + c.rateMu.Lock() + rate := c.rate + c.rateMu.Unlock() + return rate +} + // Do sends an API request and returns the API response. The API response is // 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 @@ -307,7 +316,9 @@ func (c *Client) Do(req *http.Request, v interface{}) (*Response, error) { response := newResponse(resp) - c.Rate = response.Rate + c.rateMu.Lock() + c.rate = response.Rate + c.rateMu.Unlock() err = CheckResponse(resp) if err != nil { diff --git a/github/github_test.go b/github/github_test.go index f4626f4..7ec4617 100644 --- a/github/github_test.go +++ b/github/github_test.go @@ -378,28 +378,28 @@ func TestDo_rateLimit(t *testing.T) { w.Header().Add(headerRateReset, "1372700873") }) - if got, want := client.Rate.Limit, 0; got != want { + if got, want := client.Rate().Limit, 0; got != want { t.Errorf("Client rate limit = %v, want %v", got, want) } - if got, want := client.Rate.Remaining, 0; got != want { + if got, want := client.Rate().Remaining, 0; got != want { t.Errorf("Client rate remaining = %v, got %v", got, want) } - if !client.Rate.Reset.IsZero() { + if !client.Rate().Reset.IsZero() { t.Errorf("Client rate reset not initialized to zero value") } req, _ := client.NewRequest("GET", "/", nil) client.Do(req, nil) - if got, want := client.Rate.Limit, 60; got != want { + if got, want := client.Rate().Limit, 60; got != want { t.Errorf("Client rate limit = %v, want %v", got, want) } - if got, want := client.Rate.Remaining, 59; got != want { + if got, want := client.Rate().Remaining, 59; got != want { t.Errorf("Client rate remaining = %v, want %v", got, want) } reset := time.Date(2013, 7, 1, 17, 47, 53, 0, time.UTC) - if client.Rate.Reset.UTC() != reset { - t.Errorf("Client rate reset = %v, want %v", client.Rate.Reset, reset) + if client.Rate().Reset.UTC() != reset { + t.Errorf("Client rate reset = %v, want %v", client.Rate().Reset, reset) } } @@ -418,15 +418,15 @@ func TestDo_rateLimit_errorResponse(t *testing.T) { req, _ := client.NewRequest("GET", "/", nil) client.Do(req, nil) - if got, want := client.Rate.Limit, 60; got != want { + if got, want := client.Rate().Limit, 60; got != want { t.Errorf("Client rate limit = %v, want %v", got, want) } - if got, want := client.Rate.Remaining, 59; got != want { + if got, want := client.Rate().Remaining, 59; got != want { t.Errorf("Client rate remaining = %v, want %v", got, want) } reset := time.Date(2013, 7, 1, 17, 47, 53, 0, time.UTC) - if client.Rate.Reset.UTC() != reset { - t.Errorf("Client rate reset = %v, want %v", client.Rate.Reset, reset) + if client.Rate().Reset.UTC() != reset { + t.Errorf("Client rate reset = %v, want %v", client.Rate().Reset, reset) } } @@ -453,7 +453,7 @@ func TestCheckResponse(t *testing.T) { res := &http.Response{ Request: &http.Request{}, StatusCode: http.StatusBadRequest, - Body: ioutil.NopCloser(strings.NewReader(`{"message":"m", + Body: ioutil.NopCloser(strings.NewReader(`{"message":"m", "errors": [{"resource": "r", "field": "f", "code": "c"}]}`)), } err := CheckResponse(res).(*ErrorResponse)