Don't count 429 as a retry attempt. (#215)

* Don't count 429 as a retry attempt.

If an API returns a 429 don't include it in the retry count so that we
can keep retrying until the operation succeeds.
Don't skip retry logic when SkipResourceProviderRegistration is enabled.

* Update 429 test to ensure retry on 429 doesn't exceed retry count.

CHANGELOG.md

@@ -1,5 +1,12 @@
 # CHANGELOG
 
+## v9.5.1
+
+### Bug Fixes
+
+- Don't count http.StatusTooManyRequests (429) against the retry cap.
+- Use retry logic when SkipResourceProviderRegistration is set to true.
+
 ## v9.5.0
 
 ### New Features

autorest/azure/rp.go

@@ -30,9 +30,6 @@ import (
 func DoRetryWithRegistration(client autorest.Client) autorest.SendDecorator {
 	return func(s autorest.Sender) autorest.Sender {
 		return autorest.SenderFunc(func(r *http.Request) (resp *http.Response, err error) {
-			if client.SkipResourceProviderRegistration {
-				return autorest.SendWithSender(s, r)
-			}
 			rr := autorest.NewRetriableRequest(r)
 			for currentAttempt := 0; currentAttempt < client.RetryAttempts; currentAttempt++ {
 				err = rr.Prepare()
@@ -47,7 +44,7 @@ func DoRetryWithRegistration(client autorest.Client) autorest.SendDecorator {
 					return resp, err
 				}
 
-				if resp.StatusCode != http.StatusConflict {
+				if resp.StatusCode != http.StatusConflict || client.SkipResourceProviderRegistration {
 					return resp, err
 				}
 				var re RequestError

autorest/azure/rp_test.go

@@ -79,3 +79,48 @@ func TestDoRetryWithRegistration(t *testing.T) {
 		t.Fatalf("azure: Sender#DoRetryWithRegistration -- Got: StatusCode %v; Want: StatusCode 200 OK", r.StatusCode)
 	}
 }
+
+func TestDoRetrySkipRegistration(t *testing.T) {
+	client := mocks.NewSender()
+	// first response, should retry because it is a transient error
+	client.AppendResponse(mocks.NewResponseWithStatus("Internal server error", http.StatusInternalServerError))
+	// response indicates the resource provider has not been registered
+	client.AppendResponse(mocks.NewResponseWithBodyAndStatus(mocks.NewBody(`{
+	"error":{
+		"code":"MissingSubscriptionRegistration",
+		"message":"The subscription registration is in 'Unregistered' state. The subscription must be registered to use namespace 'Microsoft.EventGrid'. See https://aka.ms/rps-not-found for how to register subscriptions.",
+		"details":[
+			{
+				"code":"MissingSubscriptionRegistration",
+				"target":"Microsoft.EventGrid",
+				"message":"The subscription registration is in 'Unregistered' state. The subscription must be registered to use namespace 'Microsoft.EventGrid'. See https://aka.ms/rps-not-found for how to register subscriptions."
+			}
+		]
+	}
+}`), http.StatusConflict, "MissingSubscriptionRegistration"))
+
+	req := mocks.NewRequestForURL("https://lol/subscriptions/rofl")
+	req.Body = mocks.NewBody("lolol")
+	r, err := autorest.SendWithSender(client, req,
+		DoRetryWithRegistration(autorest.Client{
+			PollingDelay:    time.Second,
+			PollingDuration: time.Second * 10,
+			RetryAttempts:   5,
+			RetryDuration:   time.Second,
+			Sender:          client,
+			SkipResourceProviderRegistration: true,
+		}),
+	)
+	if err != nil {
+		t.Fatalf("got error: %v", err)
+	}
+
+	autorest.Respond(r,
+		autorest.ByDiscardingBody(),
+		autorest.ByClosing(),
+	)
+
+	if r.StatusCode != http.StatusConflict {
+		t.Fatalf("azure: Sender#DoRetryWithRegistration -- Got: StatusCode %v; Want: StatusCode 409 Conflict", r.StatusCode)
+	}
+}

autorest/sender.go

@@ -215,7 +215,7 @@ func DoRetryForStatusCodes(attempts int, backoff time.Duration, codes ...int) Se
 			rr := NewRetriableRequest(r)
 			// Increment to add the first call (attempts denotes number of retries)
 			attempts++
-			for attempt := 0; attempt < attempts; attempt++ {
+			for attempt := 0; attempt < attempts; {
 				err = rr.Prepare()
 				if err != nil {
 					return resp, err
@@ -230,6 +230,11 @@ func DoRetryForStatusCodes(attempts int, backoff time.Duration, codes ...int) Se
 				if !delayed {
 					DelayForBackoff(backoff, attempt, r.Cancel)
 				}
+				// don't count a 429 against the number of attempts
+				// so that we continue to retry until it succeeds
+				if resp.StatusCode != http.StatusTooManyRequests {
+					attempt++
+				}
 			}
 			return resp, err
 		})

autorest/sender_test.go

@@ -781,7 +781,7 @@ func newAcceptedResponse() *http.Response {
 }
 
 func TestDelayWithRetryAfterWithSuccess(t *testing.T) {
-	after, retries := 5, 2
+	after, retries := 2, 2
 	totalSecs := after * retries
 
 	client := mocks.NewSender()
@@ -793,7 +793,7 @@ func TestDelayWithRetryAfterWithSuccess(t *testing.T) {
 	d := time.Second * time.Duration(totalSecs)
 	start := time.Now()
 	r, _ := SendWithSender(client, mocks.NewRequest(),
-		DoRetryForStatusCodes(5, time.Duration(time.Second), http.StatusTooManyRequests),
+		DoRetryForStatusCodes(1, time.Duration(time.Second), http.StatusTooManyRequests),
 	)
 
 	if time.Since(start) < d {