Honouring retries from 429 response

[mcquiggd]

I am attempting to capture a 429 response, and the advised retry interval from the HTTPResponse, as outlined in the blog post here

I am using .Net Core, with version 5.1 (current latest) of Polly.

I have started trying to get a simple HTTPResponse captured, but ultimately I want to use Polly to handle retries for the .Net Core DocumentDB client.

The example code in the blog post is as follows:

var honoursRetryAfter = Policy<HttpResponseMessage>  
  .Handle<HttpException>
  .OrResult(r => statusCodesToRetry.Contains(r.StatusCode)) // detail elided
  .RetryAsync(retryCount: /* your preferred retry count */, 
         sleepDurationProvider: (retryAttempt, context) => 
             context.ContainsKey("RetryAfter") && context["RetryAfter"] != null
             ? (TimeSpan)context["RetryAfter"] 
             : defaultSleepProvider(retryAttempt)  // detail elided
  );

Unfortunately, this particular item seems to be more pseudocode than a usable example - there is no such named parameter as sleepDurationProvider for RetryAsync. So, I have tried WaitAndRetry as follows:

            var honoursRetryAfterAsync = Policy<HttpResponseMessage>
                .Handle<Exception>()
                .OrResult(r => r.StatusCode.ToString().Contains("429"))
                .WaitAndRetryAsync(                    
                    retryCount: 3,
                    sleepDurationProvider: (retryAttempt, context) => context.ContainsKey("RetryAfter") && context["RetryAfter"] != null
                                                                          ? (TimeSpan)context["RetryAfter"]
                                                                          : TimeSpan.FromSeconds(0.5),
                    onRetryAsync: async (message, timespan, retryAttempt, context) => await DoSomething(context["httpClient"]));

However, it is becoming confusing, as there are so many overrides with different signatures, and their availability depends on the complete signature of the complete Policy, and some of the examples in the documentation do not compile. e.g:

Policy
  .HandleResult<HttpResponseMessage>(r => r.StatusCode == 500) // Error - cannot use equality between r.StatusCode and int
  .OrResult<HttpResponseMessage>(r => r.StatusCode == 502) // Error - should not specify <HttpResponseMessage> again

So, I am feeling as If I am just hacking things together in an inappropriate way ;)

I would really appreciate it if someone can point me in the right direction to get this feature working, as intended.

[reisenberger]

@mcquiggd. Yes, that was pseudo-code-ish (have re-labelled). Thanks for identifying the issues! Let's see if we can sort it out. I'd also been meaning to ask @jmansar (who originally requested the feature, #177) or @tcsatheesh (who was also interested in using it with DocumentDb) if they wanted to expand on my blogged (pseudo-) code: both, feel free to chip in!

More coming shortly.

[reisenberger]

@mcquiggd Having spent a couple of hours on this, I am not convinced we have adequate support within Polly yet for the DocumentDB RetryAfter Use Case.

I can see various ways forward to support this better in Polly, but need more time to consider.

A possible workround for now with Polly might be (using microsoft.azure.documentdb.core) as below:

var honoursRetryAfterFromDocumentDb = Policy
    .Handle<DocumentClientException>(de => de.RetryAfter > TimeSpan.Zero)
    .Or<AggregateException>(ae => ((ae.InnerException as DocumentClientException)?.RetryAfter ?? TimeSpan.Zero) > TimeSpan.Zero)
    .WaitAndRetryAsync(retryCount: 3,
        sleepDurationProvider: (retryAttempt, context) => TimeSpan.Zero,
        onRetryAsync: (exception, timespan, retryAttempt, context) => 
            Task.Delay((exception as DocumentClientException)?.RetryAfter ?? (exception.InnerException as DocumentClientException)?.RetryAfter ?? TimeSpan.FromSeconds(0.5))
       );

Clearly this is sub-optimal: we are bypassing Polly's in-built sleep, and sleeping in the onRetryAsync. We lose the benefit of Polly's in-built cancellation support.

I haven't had time to test the above tonight and would be interested in feedback on whether it works/gets you closer - that would help guide how we wrap this up into a more off-the-shelf/ready-to-go solution!

Thanks!

[reisenberger]

Note: the Microsoft Patterns/Practices team recommend the in-built retry for DocumentDb: Is this relevant? . Also more in this documentation

Would be great to hear if this covers it, or if there's a reason it's still worth us pursuing this use case in Polly - will help guide where to place effort. Many thanks!

EDIT: I guess this paragraph:

If you have more than one client cumulatively operating consistently above the request rate, the default
retry count currently set to 9 internally by the client may not suffice; in this case, the client throws a
DocumentClientException with status code 429 to the application. The default retry count can be changed
by setting the RetryOptions on the ConnectionPolicy instance. By default, the DocumentClientException
with status code 429 is returned after a cumulative wait time of 30 seconds if the request continues to
operate above the request rate. This occurs even when the current retry count is less than the max retry
count, be it the default of 9 or a user-defined value.

in the linked documentation suggests a reason why still handling this in Polly could be useful ....

[reisenberger]

@mcquiggd Thank you also for pointing out the slips in the readme doco. Now fixed in #248 .

[mcquiggd]

@reisenberger

Many thanks for taking the time to look into this - it's appreciated.

We had sort of come to the same point.

My thoughts so far:

1. My understanding is that the Microsoft recommendations are really a general approach to retries with services (as in - 'you should have retries'), rather than any specific technical reason for using their implementation. Having chatted to a member of the DocumentDB team, they did state that the Client is only intended for basic fault handling scenarios. At present you cannot even specify a sliding scale for wait times in the Client; to do so you need to use the Transient Fault Handling package, which will be deprecated when the Client has feature parity. But there are no plans to add other features AFAIK.

2. Polly has a major part to play in Cloud architectures, and we are unlikely to see any similar efforts from Microsoft (the Enterprise Library blocks seem to be left to one side). I want to be able to architect a common set of Policies and patterns for handling errors - be they Blob Storage, Azure Search, DocumentDB, DynamoDB, ElasticSearch.

3. It might therefore be appropriate to take the approach of say, wrapping one of the more advanced patterns (Circuit Breaker) around the individual service-specific, simple retry logic. Perhaps catch specific exceptions such as the DocumentException in a try catch, and when retries are exhausted rethrow and handle a custom 'ServiceException' with a Polly Policy / bundle of Policies, as a small abstraction.

Still thinking about all this... as you mention in your edit, I feel there is a role for Polly to play in this.

I need to ponder it for a while and try some ideas over a cup of tea ... ;-)

[mcquiggd]

24 hours is a long time in technology - now we have Azure Cosmos DB... 🥇

From the documentation:

If you have more than one client cumulatively operating above the request rate, the default retry behavior may not suffice, and the client will throw a DocumentClientException with status code 429 to the application. In cases such as this, you may consider handling retry behavior and logic in your application's error handling routines or increasing the reserved throughput for the container.

Having looked at the DocumentClient source code, I am currently testing an approach that disables the built-in retries, and uses Polly instead. Will report back if I make progress.

[reisenberger]

Thanks for the thoughtful commentary @mcquiggd ^^ (two up). Agree on all counts!

Re your point 3 - wrapping a bunch of policies around a call as a small abstraction - check out Polly's PolicyWrap (if not already seen it), designed for exactly this purpose. We also provide guidelines on ordering the policies within a wrap, although PolicyWrap is intentionally fully flexible, so any ordering can be used.

+1 to your idea of:

architect a common set of Policies and patterns for handling errors -
be they Blob Storage, Azure Search, DocumentDB, DynamoDB, ElasticSearch.

We would love to hear more about this later, if grows and you would like to share. We would love to eg blog/guest-blog about it, or share samples perhaps (duly credited) via a Polly.Contrib. (Contact me on Polly slack later if u want to discuss.)

One issue with the in-built retries for the various Azure services is that they are all different APIs - good in the sense each is probably a good fit/strategy for the particular service - but a slightly fragmented API experience. Another issue with combining Polly policies with Azure's in-built retries, is that with Polly-wraps-Azure-retry you can effectively only have the retry innermost (out of the set of policies). However, you might want to have certain policy types inside a retry, in some PolicyWraps.

Re Topaz and the Enterprise Application Blocks, yes, feels like it is well on the way out. The Microsoft patterns and practices team are now moving away from recommending Topaz, in favour of Polly (I've provided input on the Polly samples).

[mcquiggd]

@reisenberger

Well, I have made a little progress.

Firstly, I have taken the DocumentDB Benchmark project (available from the DocumentDB GitHub repo, under samples), copied it and converted it to .Net Core, so we have both .Net 4.x and .Net Core 1.x versions in the same solution.

I am using this as a testbed for my 'experiments', with the latest DocumentDB emulator set to use a single, non-partitioned collection of 400 Request Unit capacity, the 'slowest'.

I deliberately set the values for threads, and amount of documents to insert, to cause throttling.

With the DocumentDB Client retry, throttling was handled automatically.

Next, I disabled the DocumentDB Client retries; of course exceptions occurred. Bear in mind that these settings are 'global' if you follow the sound advice of using a single Client instance, which does a lot of performance enhancement, but is also a little inflexible...

private static readonly ConnectionPolicy ConnectionPolicy = new ConnectionPolicy 
{ 
    ConnectionMode = ConnectionMode.Direct, 
    ConnectionProtocol = Protocol.Tcp, 
    RequestTimeout = new TimeSpan(1, 0, 0), 
    MaxConnectionLimit = 1000, 
    RetryOptions = new RetryOptions 
    { 
        MaxRetryAttemptsOnThrottledRequests = 0, // was 10
        MaxRetryWaitTimeInSeconds = 60
    } 
};

Then, I created a Polly Retry Policy, as follows:

private static readonly RetryPolicy PollyRetryPolicy = Policy.Handle<DocumentClientException>(de => de.RetryAfter > TimeSpan.Zero).
        Or<AggregateException>(ae => ((ae.InnerException as DocumentClientException)?.RetryAfter ?? TimeSpan.Zero) > TimeSpan.Zero).
        WaitAndRetryAsync(retryCount: 3, 
                sleepDurationProvider: (retryAttempt, context) =>
                {
                    var retryAfter = TimeSpan.FromMilliseconds(10); // Should be 0? 
                    if (context == null || !context.ContainsKey("RetryAfter")) return retryAfter;                       
                    return (TimeSpan)context["RetryAfter"];
                }, 
                onRetryAsync: (exception, timespan, retryAttempt, context) =>
                {
                    context["RetryAfter"] = (exception as DocumentClientException)?.RetryAfter
                                            ?? (exception.InnerException as DocumentClientException)?.RetryAfter 
                                                 ?? TimeSpan.FromMilliseconds(1); //Just an arbitrary chosen value, never seems to be used
                    return Task.CompletedTask;
                }                
        );

So, basically using Context to pass the retry value from the onRetryAsync to the sleepDurationProvider, and using purely Polly to handle waits etc. And now we can create different policies / settings for different 'operations', using the same Client instance, potentially passing the 'operation name' in Context from the call to Execute the Policy... if my understanding of the Retry life-cycle is correct. I have noticed that retry attempt 1 does not seem to include a server retry-after value, but had limited time today to verify.

This Policy is then called in the multi-threaded benchmarking app as below:

var response  = await PollyRetryPolicy.ExecuteAsync(async () => await client.CreateDocumentAsync(
                                                                    UriFactory.CreateDocumentCollectionUri(_appSettings.DatabaseName, _appSettings.CollectionName),
                                                                    newDictionary,
                                                                    new RequestOptions()));

The result - Polly handles all the retries, observing the returned retry interval from the server:

I'll continue to work on this (it can definitely be tidied up, I have a lot of distractions around at the moment), and upload the Solution to Github in case others wish to contribute.

Perhaps you can spot any obvious mistakes - e.g. is it valid to create the Policy as static readonly in a multithreaded environment?

I want to create some more advanced examples for the different scenarios available with Polly, along the lines of your last post... will pop onto Slack at some point...

[mcquiggd]

@reisenberger

Made some changes this morning, including storing the id of the Document to be inserted via Context when ExecuteAsync is called, and that functions as expected, with data subsequently being available within the Policy.

A1. I changed the Policy declaration to be simply private - i.e. not static, not read only - I observed no change in behaviour.

A2. I have also set the parameter continueOnCapturedContext on ExecuteAsync to true - this does not appear to have any impact on results.

A3. I have verified that on retry attempt 1, there is no RetryAfter being returned in the Context, although it is returned from the server.

A4. On investigation, this is due to the sleepDurationProvider being called before the onRetryAsync delegate, which is not what I expected from the Retry lifecycle :

If another try is permitted, the policy:
Raises the onRetry delegate (if configured)
In the case of a wait-and-retry policy, calculates the duration to wait from the IEnumerable or Func<int, TimeSpan> configured, and waits.

A5. I don't see an override of the sleepDurationProvider that provides access to the Exception / handled object - which would allow simplified logic in this instance.

Any thoughts...?

[reisenberger]

@mcquiggd Great to see this all coming together!

👍 using the Context to pass things (operation name; Id) into the call - exactly what intended for.

A1. All Polly policies are fully thread-safe, so you're absolutely fine re-using an instance across multiple calls (if relevant to any decisions around this).

A2. Whether you want to continueOnCapturedContext or not, be driven simply by whether better for your Use Case / usage with DocumentDB (usual considerations around how it influences async context marshalling). Polly doesn't care either way. continueOnCapturedContext == true usually only necessary if you specially need execution after the ExecuteAsync(...) to continue on the same context as before (some actor frameworks; or need preserve UI execution context).

A3/A4. Your analysis is exactly correct. Sleep duration is calculated before onRetry/Async is invoked - even though only used after - because it's an input parameter to onRetry/Async. I've updated the Retry wiki to make this more accurate.

Foresaw your A3 problem as soon as you posted this, btw, but ... busy day this end ... you beat me to the follow-up 😀 and figured it out, so great 👍 . This exact issue is why my example the day before was more circuitous!

( A5. comments in mo )

[reisenberger]

@mcquiggd

A5. So, we could do this (add sleepDurationProvider overloads taking exception, handled result), but I'm favouring adding the Exception / handled result to Context instead to avoid proliferating further overloads (and opens up potential for other Use Cases profiting from the same info).

So, you'd have access to eg, context.LastHandledException and context.LastHandledResult - would this do the trick?

( Great to have this conversation and see the DocumentDb use case in detail: thanks for sharing! )

[mcquiggd]

So, you'd have access to eg, context.LastHandledException and context.LastHandledResult - would this do the trick?

Very nicely, and I was going to suggest that - the existing number of overloads was a little intimidating at first, and of course changing the execution behaviour and breaking backwards compatibility is a non-starter.

[mcquiggd]

@reisenberger

Added some random thoughts:

B1. A Policy can Handle, and also Or different Exception or Result types. As we found in our example for DocumentDB, that could be DocumentClientException or AggregateException. That could result in a bit of spaghetti code to determine what type the context.LastHandledException or context.LastHandledResult should be cast to for accessing the desired properties. Ugly.

B2. As we are using an initial generic Handle<T> or HandleResult<T>, will we be passing context.LastHandledException of type T, context.LastHandledResult of type T (and exploring AggregateExceptions to locate T) ? This would save a lot of repetitive code ... Any use cases that would rule this out?

B3. If we are able to pass the Handled type, makes sense to have context.HandledException and context.HandledResult.

[reisenberger]

@mcquiggd

B1/B2 Handled results: When a policy is a strongly-typed generic Policy<TResult>, executions through the policy can only return results of type TResult. Given that, we should be able to pass context.LastHandledResult as TResult.

It would mean creating a new generic-typed Context<TResult> : Context. We would need to check for breaking changes, but given Context<TResult> : Context, my initial (shallow) assessment is that breaking changes would be avoided.

(thoughts on other categories coming separately)