[app-configuration] ThrottlingRetryPolicy can retry infinitely

[richardpark-msft]

Problem

The throttlingRetryPolicy in AppConfig has the potential to retry for an extended period if the service continues returning "retry after" headers on subsequent calls.

Here's the snippet of code that handles the "retry after" retries:

  public async sendRequest(httpRequest: WebResource): Promise<HttpOperationResponse> {
    return this._nextPolicy.sendRequest(httpRequest.clone()).catch((err) => {
        // other code elided....
        return delay(delayInMs).then((_: any) => this.sendRequest(httpRequest.clone()));

Full source here: link

Some possible improvements:

• We should check the abortSignal on the request to see if the user has requested us to stop
• We should log to indicate why we are retrying (the exponentialRetryPolicy does). So even with debug logging on it's not apparent what's going on.
• Call this._nextPolicy.sendRequest, rather than this.sendRequest. This might cooperate more nicely with other policies that also handle retries (exponentialRetryPolicy). There would have to be some adjustments since the RestError+429 combo is not retryable, which was part of the reason we needed our own copy of throttlingRetryPolicy.

CC: @MaryanneNjeri who owns the original issue in filed in the appconfig repo

References:

• Issue where we added the throttlingRetryPolicy: link

[richardpark-msft]

@ramya-rao-a , @xirzec , @chradek , @bterlson : I've never been quite sure what the right interaction between various retry methods should be. I'd like all of your architectural input on this.

[chradek]

I think we should honor the max retry attempt setting across all retry policies. So if max retries was set to 3, and we encountered expired creds 2 times, we'd only allow the throttling retry policy to be invoked max 2 times. (for 4 total attempts.)

Given we have multiple retry policies today, this would mean we'd need some state that was shareable across policies to keep track of the number of retries that have been encountered. I'd prefer a single retry policy that you could configure (possibly by giving it functions that each check if a request is retriable) so the retryCount is kept solely in that single policy, but otherwise storing that state in WebResource is probably the way to go.

[ramya-rao-a]

This is worth cross checking with our friends from other languages to ensure we are treating retries similarly across the board, especially the throttling retry policy we have in Azure Core

@jeremymeng Can you reach out and confirm?

[xirzec]

Given we have multiple retry policies today, this would mean we'd need some state that was shareable across policies to keep track of the number of retries that have been encountered. I'd prefer a single retry policy that you could configure (possibly by giving it functions that each check if a request is retriable) so the retryCount is kept solely in that single policy, but otherwise storing that state in WebResource is probably the way to go.

I also want to merge our policies. .NET at least only has one policy and it seems much simpler that way.