aws : Adds configurations to the default retryer (#375)

Provides more retryer customization options by adding a constructor for default Retryer which accepts functional options. Adds NoOpRetryer to support no retry behavior. Exposes members of default retryer.

Updates the underlying logic used by the default retryer to calculate jittered delay for retry. Also updates the custom retry logic for service/ec2. Handles int overflow for retry delay.

Includes changes for PR #373 . The changes will be squashed once the PR #373 is merged in.

Fixes #370

Author: skotambkar

CHANGELOG_PENDING.md

@@ -10,11 +10,16 @@
 * `aws/ec2metadata`: Add marketplaceProductCodes to EC2 Instance Identity Document ([#374](https://github.com/aws/aws-sdk-go-v2/pull/374))
   * Adds `MarketplaceProductCodes` to the EC2 Instance Metadata's Identity Document. The ec2metadata client will now retrieve these values if they are available.
   * Related to: [aws/aws-sdk-go#2781](https://github.com/aws/aws-sdk-go/issues/2781)
-
+* `aws`: Adds configurations to the default retryer ([#375](https://github.com/aws/aws-sdk-go-v2/pull/375))
+  * Provides more customization options for retryer by adding a constructor for default Retryer which accepts functional options. Adds NoOpRetryer to support no retry behavior. Exposes members of default retryer.
+  * Updates the underlying logic used by the default retryer to calculate jittered delay for retry. Handles int overflow for retry delay. 
+  * Fixes [#370](https://github.com/aws/aws-sdk-go-v2/issues/370)
+  
 ### SDK Bugs
 * `aws`: Fixes bug in calculating throttled retry delay ([#373](https://github.com/aws/aws-sdk-go-v2/pull/373))
   * The `Retry-After` duration specified in the request is now added to the Retry delay for throttled exception. Adds test for retry delays for throttled exceptions. Fixes bug where the throttled retry's math was off.
   * Fixes [#45](https://github.com/aws/aws-sdk-go-v2/issues/45)
-* `aws` : Adds missing sdk error checking when seeking readers [#379](https://github.com/aws/aws-sdk-go-v2/pull/379).
+* `aws` : Adds missing sdk error checking when seeking readers ([#379](https://github.com/aws/aws-sdk-go-v2/pull/379))
   * Adds support for nonseekable io.Reader. Adds support for streamed payloads for unsigned body request. 
   * Fixes [#371](https://github.com/aws/aws-sdk-go-v2/issues/371)
+  

aws/client.go

@@ -63,13 +63,10 @@ func NewClient(cfg Config, metadata Metadata) *Client {
 
 	retryer := cfg.Retryer
 	if retryer == nil {
-		// TODO need better way of specifing default num retries
-		retryer = DefaultRetryer{NumMaxRetries: 3}
+		retryer = NewDefaultRetryer()
 	}
 	svc.Retryer = retryer
-
 	svc.AddDebugHandlers()
-
 	return svc
 }
 

aws/default_retryer.go

@@ -1,28 +1,41 @@
 package aws
 
 import (
+	"math"
 	"math/rand"
 	"strconv"
 	"sync"
 	"time"
 )
 
 // DefaultRetryer implements basic retry logic using exponential backoff for
-// most services. If you want to implement custom retry logic, implement the
-// Retryer interface or create a structure type that composes this
-// struct and override the specific methods. For example, to override only
-// the MaxRetries method:
-//
-//		type retryer struct {
-//      client.DefaultRetryer
-//    }
-//
-//    // This implementation always has 100 max retries
-//    func (d retryer) MaxRetries() int { return 100 }
+// most services. You can implement your own custom retryer by implementing
+// retryer interface.
 type DefaultRetryer struct {
-	NumMaxRetries int
+	NumMaxRetries    int
+	MinRetryDelay    time.Duration
+	MinThrottleDelay time.Duration
+	MaxRetryDelay    time.Duration
+	MaxThrottleDelay time.Duration
 }
 
+const (
+	// DefaultRetryerMaxNumRetries sets maximum number of retries
+	DefaultRetryerMaxNumRetries = 3
+
+	// DefaultRetryerMinRetryDelay sets minimum retry delay
+	DefaultRetryerMinRetryDelay = 30 * time.Millisecond
+
+	// DefaultRetryerMinThrottleDelay sets minimum delay when throttled
+	DefaultRetryerMinThrottleDelay = 500 * time.Millisecond
+
+	// DefaultRetryerMaxRetryDelay sets maximum retry delay
+	DefaultRetryerMaxRetryDelay = 300 * time.Second
+
+	// DefaultRetryerMaxThrottleDelay sets maximum delay when throttled
+	DefaultRetryerMaxThrottleDelay = 300 * time.Second
+)
+
 // MaxRetries returns the number of maximum returns the service will use to make
 // an individual API
 func (d DefaultRetryer) MaxRetries() int {
@@ -31,30 +44,63 @@ func (d DefaultRetryer) MaxRetries() int {
 
 var seededRand = rand.New(&lockedSource{src: rand.NewSource(time.Now().UnixNano())})
 
+// NewDefaultRetryer returns a retryer initialized with default values and optionally takes function
+// to override values for default retryer.
+func NewDefaultRetryer(opts ...func(d *DefaultRetryer)) DefaultRetryer {
+	d := DefaultRetryer{
+		NumMaxRetries:    DefaultRetryerMaxNumRetries,
+		MinRetryDelay:    DefaultRetryerMinRetryDelay,
+		MinThrottleDelay: DefaultRetryerMinThrottleDelay,
+		MaxRetryDelay:    DefaultRetryerMaxRetryDelay,
+		MaxThrottleDelay: DefaultRetryerMaxThrottleDelay,
+	}
+
+	for _, opt := range opts {
+		opt(&d)
+	}
+	return d
+}
+
 // RetryRules returns the delay duration before retrying this request again
+//
+// Note: RetryRules method must be a value receiver so that the
+// defaultRetryer is safe.
 func (d DefaultRetryer) RetryRules(r *Request) time.Duration {
-	// Set the upper limit of delay in retrying at ~five minutes
-	var minTime int64 = 30
+	minDelay := d.MinRetryDelay
 	var initialDelay time.Duration
-
 	throttle := d.shouldThrottle(r)
 	if throttle {
 		if delay, ok := getRetryAfterDelay(r); ok {
 			initialDelay = delay
 		}
-
-		minTime = 500
+		minDelay = d.MinThrottleDelay
 	}
 
 	retryCount := r.RetryCount
-	if throttle && retryCount > 8 {
-		retryCount = 8
-	} else if retryCount > 12 {
-		retryCount = 12
+
+	maxDelay := d.MaxRetryDelay
+	if throttle {
+		maxDelay = d.MaxThrottleDelay
+	}
+
+	var delay time.Duration
+
+	// Logic to cap the retry count based on the minDelay provided
+	actualRetryCount := int(math.Log2(float64(minDelay))) + 1
+	if actualRetryCount < 63-retryCount {
+		delay = time.Duration(1<<uint64(retryCount)) * getJitterDelay(minDelay)
+		if delay > maxDelay {
+			delay = getJitterDelay(maxDelay / 2)
+		}
+	} else {
+		delay = getJitterDelay(maxDelay / 2)
 	}
+	return delay + initialDelay
+}
 
-	delay := (1 << uint(retryCount)) * (seededRand.Int63n(minTime) + minTime)
-	return (time.Duration(delay) * time.Millisecond) + initialDelay
+// getJitterDelay returns a jittered delay for retry
+func getJitterDelay(duration time.Duration) time.Duration {
+	return time.Duration(seededRand.Int63n(int64(duration)) + int64(duration))
 }
 
 // ShouldRetry returns true if the request should be retried.
@@ -73,16 +119,18 @@ func (d DefaultRetryer) ShouldRetry(r *Request) bool {
 
 // ShouldThrottle returns true if the request should be throttled.
 func (d DefaultRetryer) shouldThrottle(r *Request) bool {
-	switch r.HTTPResponse.StatusCode {
-	case 429:
-	case 502:
-	case 503:
-	case 504:
-	default:
-		return r.IsErrorThrottle()
+	if r.HTTPResponse != nil {
+		switch r.HTTPResponse.StatusCode {
+		case 429:
+		case 502:
+		case 503:
+		case 504:
+		default:
+			return r.IsErrorThrottle()
+		}
+		return true
 	}
-
-	return true
+	return r.IsErrorThrottle()
 }
 
 // This will look in the Retry-After header, RFC 7231, for how long

aws/default_retryer_test.go

@@ -56,7 +56,9 @@ func TestRetryThrottleStatusCodes(t *testing.T) {
 		},
 	}
 
-	d := DefaultRetryer{NumMaxRetries: 10}
+	d := NewDefaultRetryer(func(d *DefaultRetryer) {
+		d.NumMaxRetries = 100
+	})
 	for i, c := range cases {
 		throttle := d.shouldThrottle(&c.r)
 		retry := d.ShouldRetry(&c.r)
@@ -71,7 +73,7 @@ func TestRetryThrottleStatusCodes(t *testing.T) {
 	}
 }
 
-func TestCanUseRetryAfter(t *testing.T) {
+func TestGetRetryAfterDelay(t *testing.T) {
 	cases := []struct {
 		r Request
 		e bool
@@ -164,7 +166,9 @@ func TestGetRetryDelay(t *testing.T) {
 }
 
 func TestRetryDelay(t *testing.T) {
-	d := DefaultRetryer{100}
+	d := NewDefaultRetryer(func(d *DefaultRetryer) {
+		d.NumMaxRetries = 100
+	})
 	r := Request{}
 	for i := 0; i < 100; i++ {
 		rTemp := r
@@ -190,7 +194,7 @@ func TestRetryDelay(t *testing.T) {
 	rTemp.RetryCount = 1
 	rTemp.HTTPResponse = &http.Response{StatusCode: 503, Header: http.Header{"Retry-After": []string{"300"}}}
 	a := d.RetryRules(&rTemp)
-	if a < 5*time.Minute{
+	if a < 5*time.Minute {
 		t.Errorf("retry delay should not be less than retry-after duration, received %s for retrycount %d", a, 1)
 	}
 }

aws/http_request_retry_test.go

@@ -25,7 +25,9 @@ func TestRequestCancelRetry(t *testing.T) {
 	reqNum := 0
 	cfg := unit.Config()
 	cfg.EndpointResolver = aws.ResolveWithEndpointURL("http://endpoint")
-	cfg.Retryer = aws.DefaultRetryer{NumMaxRetries: 10}
+	cfg.Retryer = aws.NewDefaultRetryer(func(d *aws.DefaultRetryer) {
+		d.NumMaxRetries = 10
+	})
 
 	s := mock.NewMockClient(cfg)
 

aws/no_op_retryer.go

@@ -0,0 +1,24 @@
+package aws
+
+import "time"
+
+// NoOpRetryer provides a retryer that performs no retries.
+// It should be used when we do not want retries to be performed.
+type NoOpRetryer struct{}
+
+// MaxRetries returns the number of maximum returns the service will use to make
+// an individual API; For NoOpRetryer the MaxRetries will always be zero.
+func (d NoOpRetryer) MaxRetries() int {
+	return 0
+}
+
+// ShouldRetry will always return false for NoOpRetryer, as it should never retry.
+func (d NoOpRetryer) ShouldRetry(_ *Request) bool {
+	return false
+}
+
+// RetryRules returns the delay duration before retrying this request again;
+// since NoOpRetryer does not retry, RetryRules always returns 0.
+func (d NoOpRetryer) RetryRules(_ *Request) time.Duration {
+	return 0
+}

aws/no_op_retryer_test.go

@@ -0,0 +1,44 @@
+package aws
+
+import (
+	"net/http"
+	"testing"
+	"time"
+)
+
+func TestNoOpRetryer(t *testing.T) {
+	cases := []struct {
+		r                Request
+		expectMaxRetries int
+		expectRetryDelay time.Duration
+		expectRetry      bool
+	}{
+		{
+			r: Request{
+				HTTPResponse: &http.Response{StatusCode: 200},
+			},
+			expectMaxRetries: 0,
+			expectRetryDelay: 0,
+			expectRetry:      false,
+		},
+	}
+
+	d := NoOpRetryer{}
+	for i, c := range cases {
+		maxRetries := d.MaxRetries()
+		retry := d.ShouldRetry(&c.r)
+		retryDelay := d.RetryRules(&c.r)
+
+		if e, a := c.expectMaxRetries, maxRetries; e != a {
+			t.Errorf("%d: expected %v, but received %v for number of max retries", i, e, a)
+		}
+
+		if e, a := c.expectRetry, retry; e != a {
+			t.Errorf("%d: expected %v, but received %v for should retry", i, e, a)
+		}
+
+		if e, a := c.expectRetryDelay, retryDelay; e != a {
+			t.Errorf("%d: expected %v, but received %v as retry delay", i, e, a)
+		}
+	}
+}

aws/request_1_6_test.go

@@ -21,7 +21,7 @@ func TestRequestInvalidEndpoint(t *testing.T) {
 		cfg,
 		aws.Metadata{},
 		cfg.Handlers,
-		aws.DefaultRetryer{},
+		aws.NewDefaultRetryer(),
 		&aws.Operation{},
 		nil,
 		nil,

aws/request_pagination_test.go

@@ -58,7 +58,9 @@ func TestPagination(t *testing.T) {
 		},
 	}
 
-	retryer := aws.DefaultRetryer{NumMaxRetries: 2}
+	retryer := aws.NewDefaultRetryer(func(d *aws.DefaultRetryer) {
+		d.NumMaxRetries = 2
+	})
 	op := aws.Operation{
 		Name: "Operation",
 		Paginator: &aws.Paginator{
@@ -160,7 +162,9 @@ func TestPaginationTruncation(t *testing.T) {
 	}
 
 	reqNum := 0
-	retryer := aws.DefaultRetryer{NumMaxRetries: 2}
+	retryer := aws.NewDefaultRetryer(func(d *aws.DefaultRetryer) {
+		d.NumMaxRetries = 2
+	})
 	ops := []aws.Operation{
 		{
 			Name: "Operation",
@@ -271,7 +275,9 @@ func BenchmarkPagination(b *testing.B) {
 		{aws.String("3"), aws.String("")},
 	}
 
-	retryer := aws.DefaultRetryer{NumMaxRetries: 2}
+	retryer := aws.NewDefaultRetryer(func(d *aws.DefaultRetryer) {
+		d.NumMaxRetries = 2
+	})
 	op := aws.Operation{
 		Name: "Operation",
 		Paginator: &aws.Paginator{
@@ -339,7 +345,9 @@ func TestPaginationWithContextCancel(t *testing.T) {
 		},
 	}
 
-	retryer := aws.DefaultRetryer{NumMaxRetries: 2}
+	retryer := aws.NewDefaultRetryer(func(d *aws.DefaultRetryer) {
+		d.NumMaxRetries = 2
+	})
 	op := aws.Operation{
 		Name: "Operation",
 		Paginator: &aws.Paginator{
@@ -350,7 +358,7 @@ func TestPaginationWithContextCancel(t *testing.T) {
 
 	for _, c := range cases {
 		input := c.input
-		inValues := []string{}
+		var inValues []string
 		p := aws.Pager{
 			NewRequest: func(ctx context.Context) (*aws.Request, error) {
 				h := defaults.Handlers()
@@ -380,7 +388,7 @@ func TestPaginationWithContextCancel(t *testing.T) {
 		ctx, cancelFn := context.WithCancel(context.Background())
 		cancelFn()
 
-		results := []*string{}
+		var results []*string
 		for p.Next(ctx) {
 			page := p.CurrentPage()
 			output := page.(*mockOutput)