aws: Refactors request retry behavior path logic (#384)

Ports v1 SDK's retry utilities to follow a consistent code path. aws.IsErrorRetryable is the primary entry point to determine if a request is retryable. Also ports support for service/kinesis to retry service specific API errors . Corrects sdk's behavior by not retrying errors with status code 501.

Related to aws/aws-sdk-go#2744, aws/aws-sdk-go#2774, aws/aws-sdk-go#2751 and aws/aws-sdk-go#1826

Fixes #372; Fixes #145

Author: skotambkar

CHANGELOG_PENDING.md

@@ -14,6 +14,10 @@
   * 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)
+* `aws` : Refactors request retry behavior path logic ([#384](https://github.com/aws/aws-sdk-go-v2/pull/384))
+  * Retry utilities now follow a consistent code path. aws.IsErrorRetryable is the primary entry point to determine if a request is retryable. 
+  * Corrects sdk's behavior by not retrying errors with status code 501. Adds support for retrying the Kinesis API error, LimitExceededException.
+  * Fixes [#372](https://github.com/aws/aws-sdk-go-v2/issues/372), [#145](https://github.com/aws/aws-sdk-go-v2/issues/145)
 
 ### SDK Bugs
 * `aws`: Fixes bug in calculating throttled retry delay ([#373](https://github.com/aws/aws-sdk-go-v2/pull/373))

aws/default_retryer.go

@@ -66,23 +66,21 @@ func NewDefaultRetryer(opts ...func(d *DefaultRetryer)) DefaultRetryer {
 // Note: RetryRules method must be a value receiver so that the
 // defaultRetryer is safe.
 func (d DefaultRetryer) RetryRules(r *Request) time.Duration {
+
 	minDelay := d.MinRetryDelay
+	maxDelay := d.MaxRetryDelay
+
 	var initialDelay time.Duration
-	throttle := d.shouldThrottle(r)
-	if throttle {
+	isThrottle := r.IsErrorThrottle()
+	if isThrottle {
 		if delay, ok := getRetryAfterDelay(r); ok {
 			initialDelay = delay
 		}
 		minDelay = d.MinThrottleDelay
-	}
-
-	retryCount := r.RetryCount
-
-	maxDelay := d.MaxRetryDelay
-	if throttle {
 		maxDelay = d.MaxThrottleDelay
 	}
 
+	retryCount := r.RetryCount
 	var delay time.Duration
 
 	// Logic to cap the retry count based on the minDelay provided
@@ -111,26 +109,7 @@ func (d DefaultRetryer) ShouldRetry(r *Request) bool {
 		return *r.Retryable
 	}
 
-	if r.HTTPResponse.StatusCode >= 500 {
-		return true
-	}
-	return r.IsErrorRetryable() || d.shouldThrottle(r)
-}
-
-// ShouldThrottle returns true if the request should be throttled.
-func (d DefaultRetryer) shouldThrottle(r *Request) bool {
-	if r.HTTPResponse != nil {
-		switch r.HTTPResponse.StatusCode {
-		case 429:
-		case 502:
-		case 503:
-		case 504:
-		default:
-			return r.IsErrorThrottle()
-		}
-		return true
-	}
-	return r.IsErrorThrottle()
+	return r.IsErrorRetryable() || r.IsErrorThrottle()
 }
 
 // This will look in the Retry-After header, RFC 7231, for how long

aws/default_retryer_test.go

@@ -60,7 +60,7 @@ func TestRetryThrottleStatusCodes(t *testing.T) {
 		d.NumMaxRetries = 100
 	})
 	for i, c := range cases {
-		throttle := d.shouldThrottle(&c.r)
+		throttle := c.r.IsErrorThrottle()
 		retry := d.ShouldRetry(&c.r)
 
 		if e, a := c.expectThrottle, throttle; e != a {

aws/defaults/handlers.go

@@ -104,7 +104,7 @@ var SendHandler = aws.NamedHandler{
 
 		// TODO remove this complexity the SDK's built http.Request should
 		// set Request.Body to nil, if there is no body to send. #318
-		if aws.NoBody == r.HTTPRequest.Body {
+		if http.NoBody == r.HTTPRequest.Body {
 			// Strip off the request body if the NoBody reader was used as a
 			// place holder for a request body. This prevents the SDK from
 			// making requests with a request body when it would be invalid
@@ -158,9 +158,10 @@ func handleSendError(r *aws.Request, err error) {
 			Body:       ioutil.NopCloser(bytes.NewReader([]byte{})),
 		}
 	}
-	// Catch all other request errors.
+
+	// Catch all request errors, and let the retryer determine
+	// if the error is retryable.
 	r.Error = awserr.New("RequestError", "send request failed", err)
-	r.Retryable = aws.Bool(true) // network errors are retryable
 
 	// Override the error with a context canceled error, if that was canceled.
 	ctx := r.Context()
@@ -183,34 +184,36 @@ var ValidateResponseHandler = aws.NamedHandler{Name: "core.ValidateResponseHandl
 
 // AfterRetryHandler performs final checks to determine if the request should
 // be retried and how long to delay.
-var AfterRetryHandler = aws.NamedHandler{Name: "core.AfterRetryHandler", Fn: func(r *aws.Request) {
-	// If one of the other handlers already set the retry state
-	// we don't want to override it based on the service's state
-	if r.Retryable == nil || r.Config.EnforceShouldRetryCheck {
-		r.Retryable = aws.Bool(r.ShouldRetry(r))
-	}
+var AfterRetryHandler = aws.NamedHandler{
+	Name: "core.AfterRetryHandler",
+	Fn: func(r *aws.Request) {
+		// If one of the other handlers already set the retry state
+		// we don't want to override it based on the service's state
+		if r.Retryable == nil || r.Config.EnforceShouldRetryCheck {
+			r.Retryable = aws.Bool(r.ShouldRetry(r))
+		}
 
-	if r.WillRetry() {
-		r.RetryDelay = r.RetryRules(r)
+		if r.WillRetry() {
+			r.RetryDelay = r.RetryRules(r)
 
-		if err := sdk.SleepWithContext(r.Context(), r.RetryDelay); err != nil {
-			r.Error = awserr.New(aws.ErrCodeRequestCanceled,
-				"request context canceled", err)
-			r.Retryable = aws.Bool(false)
-			return
-		}
+			if err := sdk.SleepWithContext(r.Context(), r.RetryDelay); err != nil {
+				r.Error = awserr.New(aws.ErrCodeRequestCanceled,
+					"request context canceled", err)
+				r.Retryable = aws.Bool(false)
+				return
+			}
 
-		// when the expired token exception occurs the credentials
-		// need to be expired locally so that the next request to
-		// get credentials will trigger a credentials refresh.
-		if p, ok := r.Config.Credentials.(sdk.Invalidator); ok && r.IsErrorExpired() {
-			p.Invalidate()
-		}
+			// when the expired token exception occurs the credentials
+			// need to be expired locally so that the next request to
+			// get credentials will trigger a credentials refresh.
+			if p, ok := r.Config.Credentials.(sdk.Invalidator); ok && r.IsErrorExpired() {
+				p.Invalidate()
+			}
 
-		r.RetryCount++
-		r.Error = nil
-	}
-}}
+			r.RetryCount++
+			r.Error = nil
+		}
+	}}
 
 // ValidateEndpointHandler is a request handler to validate a request had the
 // appropriate Region and Endpoint set. Will set r.Error if the endpoint or

aws/defaults/handlers_1_8_test.go

@@ -398,3 +398,33 @@ func TestBuildContentLength_WithBody(t *testing.T) {
 		t.Errorf("expect no error, got %v", err)
 	}
 }
+
+func TestSendHandler_HEADNoBody(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusOK)
+	}))
+
+	cfg := defaults.Config()
+	cfg.Region = "mock-region"
+	cfg.EndpointResolver = aws.ResolveWithEndpointURL(server.URL)
+
+	svc := s3.New(cfg)
+	svc.ForcePathStyle = true
+
+	req := svc.HeadObjectRequest(&s3.HeadObjectInput{
+		Bucket: aws.String("bucketname"),
+		Key:    aws.String("keyname"),
+	})
+
+	if e, a := http.NoBody, req.HTTPRequest.Body; e != a {
+		t.Fatalf("expect %T request body, got %T", e, a)
+	}
+
+	_, err := req.Send(context.Background())
+	if err != nil {
+		t.Fatalf("expect no error, got %v", err)
+	}
+	if e, a := http.StatusOK, req.HTTPResponse.StatusCode; e != a {
+		t.Errorf("expect %d status code, got %d", e, a)
+	}
+}

aws/defaults/handlers_test.go

@@ -18,6 +18,7 @@ type Handlers struct {
 	UnmarshalError   HandlerList
 	Retry            HandlerList
 	AfterRetry       HandlerList
+	CompleteAttempt  HandlerList
 	Complete         HandlerList
 }
 
@@ -34,6 +35,7 @@ func (h *Handlers) Copy() Handlers {
 		UnmarshalMeta:    h.UnmarshalMeta.copy(),
 		Retry:            h.Retry.copy(),
 		AfterRetry:       h.AfterRetry.copy(),
+		CompleteAttempt:  h.CompleteAttempt.copy(),
 		Complete:         h.Complete.copy(),
 	}
 }
@@ -50,6 +52,7 @@ func (h *Handlers) Clear() {
 	h.ValidateResponse.Clear()
 	h.Retry.Clear()
 	h.AfterRetry.Clear()
+	h.CompleteAttempt.Clear()
 	h.Complete.Clear()
 }
 
@@ -172,6 +175,21 @@ func (l *HandlerList) SwapNamed(n NamedHandler) (swapped bool) {
 	return swapped
 }
 
+// Swap will swap out all handlers matching the name passed in. The matched
+// handlers will be swapped in. True is returned if the handlers were swapped.
+func (l *HandlerList) Swap(name string, replace NamedHandler) bool {
+	var swapped bool
+
+	for i := 0; i < len(l.list); i++ {
+		if l.list[i].Name == name {
+			l.list[i] = replace
+			swapped = true
+		}
+	}
+
+	return swapped
+}
+
 // SetBackNamed will replace the named handler if it exists in the handler list.
 // If the handler does not exist the handler will be added to the end of the list.
 func (l *HandlerList) SetBackNamed(n NamedHandler) {

aws/handlers.go

@@ -1,11 +1,7 @@
-// +build go1.5
-
 package aws_test
 
 import (
 	"context"
-	"errors"
-	"fmt"
 	"strings"
 	"testing"
 	"time"
@@ -20,13 +16,10 @@ func TestRequestCancelRetry(t *testing.T) {
 	restoreSleep := mockSleep()
 	defer restoreSleep()
 
-	c := make(chan struct{})
-
 	reqNum := 0
 	cfg := unit.Config()
-	cfg.EndpointResolver = aws.ResolveWithEndpointURL("http://endpoint")
 	cfg.Retryer = aws.NewDefaultRetryer(func(d *aws.DefaultRetryer) {
-		d.NumMaxRetries = 10
+		d.NumMaxRetries = 1
 	})
 
 	s := mock.NewMockClient(cfg)
@@ -37,15 +30,14 @@ func TestRequestCancelRetry(t *testing.T) {
 	s.Handlers.UnmarshalError.Clear()
 	s.Handlers.Send.PushFront(func(r *aws.Request) {
 		reqNum++
-		r.Error = errors.New("net/http: request canceled")
 	})
 	out := &testData{}
+	ctx, cancelFn := context.WithCancel(context.Background())
 	r := s.NewRequest(&aws.Operation{Name: "Operation"}, nil, out)
-	r.HTTPRequest.Cancel = c
-	close(c)
+	r.SetContext(ctx)
+	cancelFn() // cancelling the context associated with the request
 
 	err := r.Send()
-	fmt.Println("request error", err)
 	if e, a := "canceled", err.Error(); !strings.Contains(a, e) {
 		t.Errorf("expect %q to be in %q", e, a)
 	}