Retrying will retry properly on timeouts (#981)

* Fixing bug #947 * Updating tests * Updating tests * Refactoring to work with any version * Update request.go
aws · Dec 12, 2016 · d5fd698 · d5fd698
1 parent 6545347
commit d5fd698
Show file tree

Hide file tree

Showing 5 changed files with 57 additions and 5 deletions.
diff --git a/aws/request/http_request_retry_test.go b/aws/request/http_request_retry_test.go
@@ -24,7 +24,7 @@ func TestRequestCancelRetry(t *testing.T) {
 	s.Handlers.UnmarshalError.Clear()
 	s.Handlers.Send.PushFront(func(r *request.Request) {
 		reqNum++
-		r.Error = errors.New("net/http: canceled")
+		r.Error = errors.New("net/http: request canceled")
 	})
 	out := &testData{}
 	r := s.NewRequest(&request.Operation{Name: "Operation"}, nil, out)

diff --git a/aws/request/request.go b/aws/request/request.go
@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"fmt"
 	"io"
+	"net"
 	"net/http"
 	"net/url"
 	"reflect"
@@ -297,7 +298,7 @@ func (r *Request) Send() error {
 
 		r.Handlers.Send.Run(r)
 		if r.Error != nil {
-			if strings.Contains(r.Error.Error(), "net/http: request canceled") {
+			if !shouldRetryCancel(r) {
 				return r.Error
 			}
 
@@ -364,3 +365,26 @@ func AddToUserAgent(r *Request, s string) {
 	}
 	r.HTTPRequest.Header.Set("User-Agent", s)
 }
+
+func shouldRetryCancel(r *Request) bool {
+	awsErr, ok := r.Error.(awserr.Error)
+	timeoutErr := false
+	errStr := r.Error.Error()
+	if ok {
+		err := awsErr.OrigErr()
+		netErr, netOK := err.(net.Error)
+		timeoutErr = netOK && netErr.Temporary()
+		if urlErr, ok := err.(*url.Error); !timeoutErr && ok {
+			errStr = urlErr.Err.Error()
+		}
+	}
+
+	// There can be two types of canceled errors here.
+	// The first being a net.Error and the other being an error.
+	// If the request was timed out, we want to continue the retry
+	// process. Otherwise, return the canceled error.
+	return timeoutErr ||
+		(errStr != "net/http: request canceled" &&
+			errStr != "net/http: request canceled while waiting for connection")
+
+}
diff --git a/aws/request/request_1_5_test.go b/aws/request/request_1_5_test.go
@@ -0,0 +1,11 @@
+// +build !go1.6
+
+package request_test
+
+import (
+	"errors"
+
+	"github.com/aws/aws-sdk-go/aws/awserr"
+)
+
+var errTimeout = awserr.New("foo", "bar", errors.New("net/http: request canceled Timeout"))
diff --git a/aws/request/request_1_6_test.go b/aws/request/request_1_6_test.go
@@ -3,11 +3,13 @@
 package request_test
 
 import (
+	"errors"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 
 	"github.com/aws/aws-sdk-go/aws"
+	"github.com/aws/aws-sdk-go/aws/awserr"
 	"github.com/aws/aws-sdk-go/aws/client"
 	"github.com/aws/aws-sdk-go/aws/client/metadata"
 	"github.com/aws/aws-sdk-go/aws/defaults"
@@ -31,3 +33,19 @@ func TestRequestInvalidEndpoint(t *testing.T) {
 
 	assert.Error(t, r.Error)
 }
+
+type timeoutErr struct {
+	error
+}
+
+var errTimeout = awserr.New("foo", "bar", &timeoutErr{
+	errors.New("net/http: request canceled"),
+})
+
+func (e *timeoutErr) Timeout() bool {
+	return true
+}
+
+func (e *timeoutErr) Temporary() bool {
+	return true
+}
diff --git a/aws/request/request_test.go b/aws/request/request_test.go
@@ -3,7 +3,6 @@ package request_test
 import (
 	"bytes"
 	"encoding/json"
-	"errors"
 	"fmt"
 	"io"
 	"io/ioutil"
@@ -313,7 +312,7 @@ func TestRequestRecoverTimeoutWithNilBody(t *testing.T) {
 		{StatusCode: 200, Body: body(`{"data":"valid"}`)},
 	}
 	errors := []error{
-		errors.New("timeout"), nil,
+		errTimeout, nil,
 	}
 
 	s := awstesting.NewClient(aws.NewConfig().WithMaxRetries(10))
@@ -349,7 +348,7 @@ func TestRequestRecoverTimeoutWithNilResponse(t *testing.T) {
 		{StatusCode: 200, Body: body(`{"data":"valid"}`)},
 	}
 	errors := []error{
-		errors.New("timeout"),
+		errTimeout,
 		nil,
 	}