Implementing HTTP Retries (#215)

* Implementing HTTP retries * Added unit tests for retry capability * Some code cleanup * More code clean up * More readability and test improvements * Testing retries in RTDB * Fixed the low-level network error test * Added MaxDelay support * Added a attemptResult type for cleanup * Reordered the code * Addressing code review feedback * Retry only 5xx responses by default * Updated changelog
firebase · Jan 23, 2019 · 6ea9a02 · 6ea9a02
1 parent f1dcecc
commit 6ea9a02
Show file tree

Hide file tree

Showing 7 changed files with 663 additions and 40 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,9 @@
 # Unreleased
 
+- [added] Implemented HTTP retries for the `db` package. This package
+ now retries HTTP calls on low-level connection and socket read errors, as
+ well as HTTP 500 and 503 errors.
+
 # v3.6.0
 
 - [added] `messaging.Aps` type now supports critical sound in its payload.

diff --git a/db/db.go b/db/db.go
@@ -25,7 +25,6 @@ import (
 
  "firebase.google.com/go/internal"
  "google.golang.org/api/option"
- "google.golang.org/api/transport"
 )
 
 const userAgentFormat = "Firebase/HTTP/%s/%s/AdminGo"
@@ -44,14 +43,6 @@ type Client struct {
 // This function can only be invoked from within the SDK. Client applications should access the
 // Database service through firebase.App.
 func NewClient(ctx context.Context, c *internal.DatabaseConfig) (*Client, error) {
- opts := append([]option.ClientOption{}, c.Opts...)
- ua := fmt.Sprintf(userAgentFormat, c.Version, runtime.Version())
- opts = append(opts, option.WithUserAgent(ua))
- hc, _, err := transport.NewHTTPClient(ctx, opts...)
- if err != nil {
- return nil, err
- }
-
  p, err := url.ParseRequestURI(c.URL)
  if err != nil {
  return nil, err
@@ -69,6 +60,14 @@ func NewClient(ctx context.Context, c *internal.DatabaseConfig) (*Client, error)
  }
  }
 
+ opts := append([]option.ClientOption{}, c.Opts...)
+ ua := fmt.Sprintf(userAgentFormat, c.Version, runtime.Version())
+ opts = append(opts, option.WithUserAgent(ua))
+ hc, _, err := internal.NewHTTPClient(ctx, opts...)
+ if err != nil {
+ return nil, err
+ }
+
  ep := func(b []byte) string {
  var p struct {
  Error string `json:"error"`
@@ -78,8 +77,10 @@ func NewClient(ctx context.Context, c *internal.DatabaseConfig) (*Client, error)
  }
  return p.Error
  }
+ hc.ErrParser = ep
+
  return &Client{
- hc: &internal.HTTPClient{Client: hc, ErrParser: ep},
+ hc: hc,
  url: fmt.Sprintf("https://%s", p.Host),
  authOverride: string(ao),
  }, nil

diff --git a/db/db_test.go b/db/db_test.go
@@ -33,7 +33,10 @@ import (
  "google.golang.org/api/option"
 )
 
-const testURL = "https://test-db.firebaseio.com"
+const (
+ testURL = "https://test-db.firebaseio.com"
+ defaultMaxRetries = 1
+)
 
 var testUserAgent string
 var testAuthOverrides string
@@ -56,6 +59,9 @@ func TestMain(m *testing.M) {
  if err != nil {
  log.Fatalln(err)
  }
+ retryConfig := client.hc.RetryConfig
+ retryConfig.MaxRetries = defaultMaxRetries
+ retryConfig.ExpBackoffFactor = 0
 
  ao := map[string]interface{}{"uid": "user1"}
  aoClient, err = NewClient(context.Background(), &internal.DatabaseConfig{

diff --git a/db/ref_test.go b/db/ref_test.go
@@ -292,8 +292,9 @@ func TestWelformedHttpError(t *testing.T) {
  })
  }
 
- if len(mock.Reqs) != len(testOps) {
- t.Errorf("Requests = %d; want = %d", len(mock.Reqs), len(testOps))
+ wantReqs := len(testOps) * (1 + defaultMaxRetries)
+ if len(mock.Reqs) != wantReqs {
+ t.Errorf("Requests = %d; want = %d", len(mock.Reqs), wantReqs)
  }
 }
 
@@ -312,8 +313,9 @@ func TestUnexpectedHttpError(t *testing.T) {
  })
  }
 
- if len(mock.Reqs) != len(testOps) {
- t.Errorf("Requests = %d; want = %d", len(mock.Reqs), len(testOps))
+ wantReqs := len(testOps) * (1 + defaultMaxRetries)
+ if len(mock.Reqs) != wantReqs {
+ t.Errorf("Requests = %d; want = %d", len(mock.Reqs), wantReqs)
  }
 }
 

diff --git a/internal/http_client.go b/internal/http_client.go
@@ -21,44 +21,131 @@ import (
  "fmt"
  "io"
  "io/ioutil"
+ "math"
  "net/http"
+ "strconv"
+ "time"
+
+ "google.golang.org/api/option"
+ "google.golang.org/api/transport"
 )
 
 // HTTPClient is a convenient API to make HTTP calls.
 //
-// This API handles some of the repetitive tasks such as entity serialization and deserialization
-// involved in making HTTP calls. It provides a convenient mechanism to set headers and query
+// This API handles repetitive tasks such as entity serialization and deserialization
+// when making HTTP calls. It provides a convenient mechanism to set headers and query
 // parameters on outgoing requests, while enforcing that an explicit context is used per request.
-// Responses returned by HTTPClient can be easily parsed as JSON, and provide a simple mechanism to
-// extract error details.
+// Responses returned by HTTPClient can be easily unmarshalled as JSON.
+//
+// HTTPClient also handles automatically retrying failed HTTP requests.
 type HTTPClient struct {
- Client *http.Client
- ErrParser ErrorParser
+ Client *http.Client
+ RetryConfig *RetryConfig
+ ErrParser ErrorParser
 }
 
-// Do executes the given Request, and returns a Response.
-func (c *HTTPClient) Do(ctx context.Context, r *Request) (*Response, error) {
- req, err := r.buildHTTPRequest()
+// NewHTTPClient creates a new HTTPClient using the provided client options and the default
+// RetryConfig.
+//
+// The default RetryConfig retries requests on all low-level network errors as well as on HTTP
+// InternalServerError (500) and ServiceUnavailable (503) errors. Repeatedly failing requests are
+// retried up to 4 times with exponential backoff. Retry delay is never longer than 2 minutes.
+//
+// NewHTTPClient returns the created HTTPClient along with the target endpoint URL. The endpoint
+// is obtained from the client options passed into the function.
+func NewHTTPClient(ctx context.Context, opts ...option.ClientOption) (*HTTPClient, string, error) {
+ hc, endpoint, err := transport.NewHTTPClient(ctx, opts...)
  if err != nil {
- return nil, err
+ return nil, "", err
  }
+ twoMinutes := time.Duration(2) * time.Minute
+ client := &HTTPClient{
+ Client: hc,
+ RetryConfig: &RetryConfig{
+ MaxRetries: 4,
+ CheckForRetry: retryNetworkAndHTTPErrors(
+ http.StatusInternalServerError,
+ http.StatusServiceUnavailable,
+ ),
+ ExpBackoffFactor: 0.5,
+ MaxDelay: &twoMinutes,
+ },
+ }
+ return client, endpoint, nil
+}
 
- resp, err := c.Client.Do(req.WithContext(ctx))
- if err != nil {
- return nil, err
+// Do executes the given Request, and returns a Response.
+//
+// If a RetryConfig is specified on the client, Do attempts to retry failing requests.
+func (c *HTTPClient) Do(ctx context.Context, req *Request) (*Response, error) {
+ var result *attemptResult
+ var err error
+
+ for retries := 0; ; retries++ {
+ result, err = c.attempt(ctx, req, retries)
+ if err != nil {
+ return nil, err
+ }
+ if !result.Retry {
+ break
+ }
+ if err = result.waitForRetry(ctx); err != nil {
+ return nil, err
+ }
  }
- defer resp.Body.Close()
+ return result.handleResponse()
+}
 
- b, err := ioutil.ReadAll(resp.Body)
+func (c *HTTPClient) attempt(ctx context.Context, req *Request, retries int) (*attemptResult, error) {
+ hr, err := req.buildHTTPRequest()
  if err != nil {
  return nil, err
  }
- return &Response{
- Status: resp.StatusCode,
- Body: b,
- Header: resp.Header,
- errParser: c.ErrParser,
- }, nil
+
+ resp, err := c.Client.Do(hr.WithContext(ctx))
+ result := &attemptResult{
+ Resp: resp,
+ Err: err,
+ ErrParser: c.ErrParser,
+ }
+
+ // If a RetryConfig is available, always consult it to determine if the request should be retried
+ // or not. Even if there was a network error, we may not want to retry the request based on the
+ // RetryConfig that is in effect.
+ if c.RetryConfig != nil {
+ delay, retry := c.RetryConfig.retryDelay(retries, resp, err)
+ result.RetryAfter = delay
+ result.Retry = retry
+ if retry && resp != nil {
+ defer resp.Body.Close()
+ }
+ }
+ return result, nil
+}
+
+type attemptResult struct {
+ Resp *http.Response
+ Err error
+ Retry bool
+ RetryAfter time.Duration
+ ErrParser ErrorParser
+}
+
+func (r *attemptResult) waitForRetry(ctx context.Context) error {
+ if r.RetryAfter > 0 {
+ select {
+ case <-ctx.Done():
+ case <-time.After(r.RetryAfter):
+ }
+ }
+ return ctx.Err()
+}
+
+func (r *attemptResult) handleResponse() (*Response, error) {
+ if r.Err != nil {
+ return nil, r.Err
+ }
+ return newResponse(r.Resp, r.ErrParser)
 }
 
 // Request contains all the parameters required to construct an outgoing HTTP request.
@@ -124,9 +211,23 @@ type Response struct {
  errParser ErrorParser
 }
 
+func newResponse(resp *http.Response, errParser ErrorParser) (*Response, error) {
+ defer resp.Body.Close()
+ b, err := ioutil.ReadAll(resp.Body)
+ if err != nil {
+ return nil, err
+ }
+ return &Response{
+ Status: resp.StatusCode,
+ Body: b,
+ Header: resp.Header,
+ errParser: errParser,
+ }, nil
+}
+
 // CheckStatus checks whether the Response status code has the given HTTP status code.
 //
-// Returns an error if the status code does not match. If an ErroParser is specified, uses that to
+// Returns an error if the status code does not match. If an ErrorParser is specified, uses that to
 // construct the returned error message. Otherwise includes the full response body in the error.
 func (r *Response) CheckStatus(want int) error {
  if r.Status == want {
@@ -188,3 +289,93 @@ func WithQueryParams(qp map[string]string) HTTPOption {
  r.URL.RawQuery = q.Encode()
  }
 }
+
+// RetryConfig specifies how the HTTPClient should retry failing HTTP requests.
+//
+// A request is never retried more than MaxRetries times. If CheckForRetry is nil, all network
+// errors, and all 400+ HTTP status codes are retried. If an HTTP error response contains the
+// Retry-After header, it is always respected. Otherwise retries are delayed with exponential
+// backoff. Set ExpBackoffFactor to 0 to disable exponential backoff, and retry immediately
+// after each error.
+//
+// If MaxDelay is set, retries delay gets capped by that value. If the Retry-After header
+// requires a longer delay than MaxDelay, retries are not attempted.
+type RetryConfig struct {
+ MaxRetries int
+ CheckForRetry RetryCondition
+ ExpBackoffFactor float64
+ MaxDelay *time.Duration
+}
+
+// RetryCondition determines if an HTTP request should be retried depending on its last outcome.
+type RetryCondition func(resp *http.Response, networkErr error) bool
+
+func (rc *RetryConfig) retryDelay(retries int, resp *http.Response, err error) (time.Duration, bool) {
+ if !rc.retryEligible(retries, resp, err) {
+ return 0, false
+ }
+ estimatedDelay := rc.estimateDelayBeforeNextRetry(retries)
+ serverRecommendedDelay := parseRetryAfterHeader(resp)
+ if serverRecommendedDelay > estimatedDelay {
+ estimatedDelay = serverRecommendedDelay
+ }
+ if rc.MaxDelay != nil && estimatedDelay > *rc.MaxDelay {
+ return 0, false
+ }
+ return estimatedDelay, true
+}
+
+func (rc *RetryConfig) retryEligible(retries int, resp *http.Response, err error) bool {
+ if retries >= rc.MaxRetries {
+ return false
+ }
+ if rc.CheckForRetry == nil {
+ return err != nil || resp.StatusCode >= 500
+ }
+ return rc.CheckForRetry(resp, err)
+}
+
+func (rc *RetryConfig) estimateDelayBeforeNextRetry(retries int) time.Duration {
+ if retries == 0 {
+ return 0
+ }
+ delayInSeconds := int64(math.Pow(2, float64(retries)) * rc.ExpBackoffFactor)
+ estimatedDelay := time.Duration(delayInSeconds) * time.Second
+ if rc.MaxDelay != nil && estimatedDelay > *rc.MaxDelay {
+ estimatedDelay = *rc.MaxDelay
+ }
+ return estimatedDelay
+}
+
+var retryTimeClock Clock = &SystemClock{}
+
+func parseRetryAfterHeader(resp *http.Response) time.Duration {
+ if resp == nil {
+ return 0
+ }
+ retryAfterHeader := resp.Header.Get("retry-after")
+ if retryAfterHeader == "" {
+ return 0
+ }
+ if delayInSeconds, err := strconv.ParseInt(retryAfterHeader, 10, 64); err == nil {
+ return time.Duration(delayInSeconds) * time.Second
+ }
+ if timestamp, err := http.ParseTime(retryAfterHeader); err == nil {
+ return timestamp.Sub(retryTimeClock.Now())
+ }
+ return 0
+}
+
+func retryNetworkAndHTTPErrors(statusCodes ...int) RetryCondition {
+ return func(resp *http.Response, networkErr error) bool {
+ if networkErr != nil {
+ return true
+ }
+ for _, retryOnStatus := range statusCodes {
+ if resp.StatusCode == retryOnStatus {
+ return true
+ }
+ }
+ return false
+ }
+}