Skip to content

Commit

Permalink
Always retry a request even if the sender returns a non-nil error
Browse files Browse the repository at this point in the history
  • Loading branch information
jhendrixMSFT committed Sep 27, 2019
1 parent 69b4126 commit f81aab2
Show file tree
Hide file tree
Showing 5 changed files with 22 additions and 30 deletions.
10 changes: 8 additions & 2 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
# CHANGELOG

## v13.0.2

### Bug Fixes

- Always retry a request even if the sender returns a non-nil error.

## v13.0.1

## Bug Fixes
Expand All @@ -12,9 +18,9 @@

The `tracing` package has been rewritten to provide a common interface for consumers to wire in the tracing package of their choice.
What this means is that by default no tracing provider will be compiled into your program and setting the `AZURE_SDK_TRACING_ENABLED`
environment variable will have no effect. To enable this previous behavior you must now add the following include to your source file.
environment variable will have no effect. To enable this previous behavior you must now add the following import to your source file.
```go
include _ "github.com/Azure/go-autorest/tracing/opencensus"
import _ "github.com/Azure/go-autorest/tracing/opencensus"
```
The APIs required by autorest-generated code have remained but some APIs have been removed and new ones added.
The following APIs and variables have been removed (the majority of them were moved to the `opencensus` package).
Expand Down
27 changes: 8 additions & 19 deletions autorest/adal/token.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import (
"fmt"
"io/ioutil"
"math"
"net"
"net/http"
"net/url"
"strings"
Expand Down Expand Up @@ -913,10 +912,8 @@ func retryForIMDS(sender Sender, req *http.Request, maxAttempts int) (resp *http

for attempt < maxAttempts {
resp, err = sender.Do(req)
// retry on temporary network errors, e.g. transient network failures.
// if we don't receive a response then assume we can't connect to the
// endpoint so we're likely not running on an Azure VM so don't retry.
if (err != nil && !isTemporaryNetworkError(err)) || resp == nil || resp.StatusCode == http.StatusOK || !containsInt(retries, resp.StatusCode) {
// we want to retry if err is not nil or the status code is in the list of retry codes
if err == nil && !responseHasStatusCode(resp, retries...) {
return
}

Expand All @@ -940,20 +937,12 @@ func retryForIMDS(sender Sender, req *http.Request, maxAttempts int) (resp *http
return
}

// returns true if the specified error is a temporary network error or false if it's not.
// if the error doesn't implement the net.Error interface the return value is true.
func isTemporaryNetworkError(err error) bool {
if netErr, ok := err.(net.Error); !ok || (ok && netErr.Temporary()) {
return true
}
return false
}

// returns true if slice ints contains the value n
func containsInt(ints []int, n int) bool {
for _, i := range ints {
if i == n {
return true
func responseHasStatusCode(resp *http.Response, codes ...int) bool {
if resp != nil {
for _, i := range codes {
if i == resp.StatusCode {
return true
}
}
}
return false
Expand Down
4 changes: 0 additions & 4 deletions autorest/sender.go
Original file line number Diff line number Diff line change
Expand Up @@ -289,10 +289,6 @@ func doRetryForStatusCodesImpl(s Sender, r *http.Request, count429 bool, attempt
return
}
resp, err = s.Do(rr.Request())
// if the error isn't temporary don't bother retrying
if err != nil && !IsTemporaryNetworkError(err) {
return
}
// we want to retry if err is not nil (e.g. transient network failure). note that for failed authentication
// resp and err will both have a value, so in this case we don't want to retry as it will never succeed.
if err == nil && !ResponseHasStatusCode(resp, codes...) || IsTokenRefreshError(err) {
Expand Down
9 changes: 5 additions & 4 deletions autorest/sender_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -923,19 +923,20 @@ func (fe fatalError) Temporary() bool {
}

func TestDoRetryForStatusCodes_NilResponseFatalError(t *testing.T) {
const retryAttempts = 3
client := mocks.NewSender()
client.AppendResponse(nil)
client.SetError(fatalError{"fatal error"})
client.AppendAndRepeatResponse(nil, retryAttempts+1)
client.SetAndRepeatError(fatalError{"fatal error"}, retryAttempts+1)

r, err := SendWithSender(client, mocks.NewRequest(),
DoRetryForStatusCodes(3, time.Duration(1*time.Second), StatusCodesForRetry...),
DoRetryForStatusCodes(retryAttempts, time.Duration(1*time.Second), StatusCodesForRetry...),
)

Respond(r,
ByDiscardingBody(),
ByClosing())

if err == nil || client.Attempts() > 1 {
if err == nil || client.Attempts() < retryAttempts+1 {
t.Fatalf("autorest: Sender#TestDoRetryForStatusCodes_NilResponseFatalError -- Got: nil error or wrong number of attempts - %v", err)
}
}
Expand Down
2 changes: 1 addition & 1 deletion autorest/version.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import (
"runtime"
)

const number = "v13.0.1"
const number = "v13.0.2"

var (
userAgent = fmt.Sprintf("Go/%s (%s-%s) go-autorest/%s",
Expand Down

0 comments on commit f81aab2

Please sign in to comment.