Requests Retrying is broken if the error was caused due to a client timeout

[hallmaxw]

This logic in Request.Send() makes it impossible to retry if a Send handler fails due to a client timeout.

Here's what an error returned by the standard library's Client.Do() looks like:

Get http://127.0.0.1:52299/: net/http: request canceled (Client.Timeout exceeded while awaiting headers)

I'd expect to be able to retry a request if it failed due to a timeout I set. Here's a unit test I wrote to check this:

package main

import (
	"errors"
	"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/corehandlers"
	"github.com/aws/aws-sdk-go/aws/request"
	"net"
	"net/http"
	"net/http/httptest"
	"testing"
	"time"
)

func TestRequestRetryClientTimeout(t *testing.T) {
	ts := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
		// sleep for 2 seconds to ensure the client's request times out
		time.Sleep(2 * time.Second)
		rw.Write([]byte("ok"))
	}))
	defer ts.Close()

	var handlers request.Handlers
	handlers.Send.PushBackNamed(corehandlers.SendHandler)
	handlers.AfterRetry.PushBackNamed(corehandlers.AfterRetryHandler)

	// count the number of times we try to send the request
	sendCount := 0
	handlers.Send.PushFront(func(r *request.Request) {
		sendCount++
	})

	// uncomment this to fix the bug
	// handlers.Send.PushBack(fixTimeoutError)

	numMaxRetries := 2
	op := &request.Operation{
		Name:       "Test",
		HTTPMethod: "GET",
		HTTPPath:   "/",
	}
	c := &http.Client{
		Timeout: 1 * time.Second,
	}
	// setup a request to send to our test server
	config := aws.NewConfig().
		WithHTTPClient(c).
		WithSleepDelay(time.Sleep)
	clientInfo := metadata.ClientInfo{Endpoint: ts.URL}
	retryer := client.DefaultRetryer{NumMaxRetries: numMaxRetries}
	req := request.New(*config, clientInfo, handlers, retryer, op, nil, nil)
	req.SetStringBody("testing")

	err := req.Send()
	t.Logf("Error received when from Send(): %v", err)

	expectedSendCount := numMaxRetries + 1
	if sendCount != expectedSendCount {
		t.Errorf("Expected the request to be sent %d times but it was sent %d times", expectedSendCount, sendCount)
	}
}

// Handler that fixes the error handling. I shouldn't have to do this
func fixTimeoutError(r *request.Request) {
	origErr := getOriginalError(r.Error)
	switch typedErr := origErr.(type) {
	case net.Error:
		if typedErr.Timeout() {
			r.Error = errors.New("timeout")
		}
	}
}

func getOriginalError(err error) error {
	switch typedError := err.(type) {
	case awserr.Error:
		return typedError.OrigErr()
	default:
		return err
	}
}

Was this intended when support for cancelling HTTP requests was added? (this commit)

[jasdel]

Thanks for letting us know about this issue, and example. I think you're correct. This change was added to capture users's canceling a request, ensuring the SDK did not attempt to retry a canceled. request. I think we can add additional logic around this correctly allow retries of Timeout, but not cancels.

[jasdel]

The SDK should be able to take advantage of the net package's Error interface for determining if the request can be retried. https://golang.org/pkg/net/#Error

[xibz]

Hello @mbh621, I have a PR, #981, out that addresses this issue. Please let us know if you have any more issues!