aws/retry: Retry token post request attempt not being cleared

[jasdel]

Noticed this relRetryToken value being returned, but the value is never used again, nor released. This potentially could lead to token leaking preventing subsequent retry attempts being made. This needs more investigation, and cleanup to correctly handle the returned retry token for the attempt.

aws-sdk-go-v2/aws/retry/middleware.go

Lines 170 to 173 in a1be727

	relRetryToken, reqErr := r.retryer.GetRetryToken(ctx, err)
	if reqErr != nil {
	return out, attemptResult, reqErr
	}

[Skarlso]

I'll take a look if people don't mind. :)

[Skarlso]

Yeah, there are some unused things in the test too. Especially the mockhandler and retryProvider. So there is a gap in testing as well.

In fact the retry errors are not tested... I suspect there might be a bug in there. I'll increase the coverage and fix the unused releaser.

[Skarlso]

Okay, so this is what I found....

This is a tricky situation and please correct me if I'm wrong, but... looking at this code:

	if err == nil {
		return out, attemptResult, err
	}

	retryable := r.retryer.IsErrorRetryable(err)
	if !retryable {
		r.logf(logger, logging.Debug, "request failed with unretryable error %v", err)
		return out, attemptResult, err
	}

	// set retryable to true
	attemptResult.Retryable = true

	if maxAttempts > 0 && attemptNum >= maxAttempts {
		r.logf(logger, logging.Debug, "max retry attempts exhausted, max %d", maxAttempts)
		err = &MaxAttemptsError{
			Attempt: attemptNum,
			Err:     err,
		}
		return out, attemptResult, err
	}

	relRetryToken, reqErr := r.retryer.GetRetryToken(ctx, err)
	if reqErr != nil {
		return out, attemptResult, reqErr
	}

It looks like, this whole thing stops if err == nil. And the release function is a no-op in case of errors.

func (f releaseToken) release(err error) error {
	if err != nil {
		return nil
	}

	return f()
}

Which means, If I see this function correctly, calling the release there after that getToken call basically won't do anything ever. Because it wouldn't even get to this code segment if there was no error. And in the begin, once this function starts again, the same release is called.

I'll write some more tests however to prove that there is some kind of problem. I wrote a small test already which fiddles with this code path. Still, I might be incorrect, but please double check.

Cheers!

[Skarlso]

Ok, confirmed that never the less, it's possible that because the retry action does take the capacity and there is a new try, the initial token releases only 1.

I made an initial attempt at fixing this particular problem where when the tokens are all taken up and a new Handling is tried, it will exhaust all capacities because the retry's release was not called.

[Skarlso]

Attached initial PR for fixing the releasing of the taken capacity after a successful retry.

[jasdel]

Thanks for taking a look at this, and creating a PR @Skarlso. We'll review that PR and post feedback.

[TomerHeber]

I have run into the same issue. (And was about to open an issue).
As a workaround, I've created my own RateLimiter.

func newCustomRateLimiter() retry.RateLimiter {
	return &customRateLimiter{ratelimit.NewTokenRateLimit(5000)}
}

func (c *customRateLimiter) GetToken(ctx context.Context, cost uint) (releaseToken func() error, err error) {
	b := backoff.NewExponentialBackOff()
	b.MaxInterval = time.Minute * 10
	b.InitialInterval = time.Second * 5
	b.RandomizationFactor = 0.5
	b.Multiplier = 2
	t := backoff.NewTicker(b)
	defer t.Stop()

	for {
		f, err := c.rt.GetToken(ctx, cost)
		if err == nil {
			return f, nil
		}
		c.rt.AddTokens(retry.DefaultRetryCost)
		<-t.C
	}
}

func (c *customRateLimiter) AddTokens(v uint) error {
	return c.rt.AddTokens(v)
}

	aws_config.WithRetryer(func() aws.Retryer {
			// Retry until cannot retry or successful.
			return retry.AddWithMaxAttempts(retry.NewStandard(func(so *retry.StandardOptions) {
				// Use exponential backoff.
				so.Backoff = retry.NewExponentialJitterBackoff(3 * time.Minute)
				so.RateLimiter = newCustomRateLimiter()
			}), 0)
		}),

Obviously, it's not an ideal solution. But simple enough to get it working for now.

[Skarlso]

Yeah, would be nice if someone reviewed my code for this fix. :)