Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Retries cannot be disabled entirely #712

Open
nojnhuh opened this issue Oct 10, 2022 · 3 comments
Open

Retries cannot be disabled entirely #712

nojnhuh opened this issue Oct 10, 2022 · 3 comments

Comments

@nojnhuh
Copy link
Member

nojnhuh commented Oct 10, 2022

Here it's documented that to disable retries, the client's RetryAttempts should be set to 1:

// RetryAttempts sets the total number of times the client will attempt to make an HTTP request.
// Set the value to 1 to disable retries. DO NOT set the value to less than 1.
RetryAttempts int

That value is used here:

autorest.DoRetryForStatusCodes(client.RetryAttempts, client.RetryDuration, autorest.StatusCodesForRetry...),

And gets passed through to here, where the same value is being interpreted as number of retries beyond the first attempt:

func doRetryForStatusCodesImpl(s Sender, r *http.Request, count429 bool, attempts int, backoff, cap time.Duration, codes ...int) (resp *http.Response, err error) {
rr := NewRetriableRequest(r)
// Increment to add the first call (attempts denotes number of retries)
for attempt, delayCount := 0, 0; attempt < attempts+1; {

Setting the client's RetryAttempts to 1 then ultimately results in requests eligible for retry always occurring twice.

@jhendrixMSFT
Copy link
Member

This looks to be a doc bug. Setting RetryAttempts to zero will disable retries.

@jhendrixMSFT
Copy link
Member

Hmm there might be a bit more to this per #597.

@nojnhuh
Copy link
Member Author

nojnhuh commented Oct 10, 2022

I took a stab at tweaking some of the loop accounting to ensure requests are always tried at least once and so client.RetryAttempts represents retries, not total attempts. To the best of my knowledge, that preserves the existing behavior while allowing RetryAttempts to be 0 to disable all retries. Here's what I have so far: main...nojnhuh:go-autorest:retry-fix

Does that seem like a reasonable approach? If so I can open a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants