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

[already possible] Respect Retry-After HTTP header #414

Closed
nmurf opened this issue Mar 9, 2018 · 12 comments
Closed

[already possible] Respect Retry-After HTTP header #414

nmurf opened this issue Mar 9, 2018 · 12 comments

Comments

@nmurf
Copy link

nmurf commented Mar 9, 2018

The Retry-After response header is used to indicate how long a client should wait before sending the next request. It would be great if RetryPolicy took this into account.

Pseudocode:

var serverWaitDuration = getServerWaitDuration(response.Headers.Get("Retry-After"));
var waitDuration = Math.Max(clientWaitDuration.TotalMilliseconds, serverWaitDuration.TotalMilliseconds);
@reisenberger
Copy link
Member

Hi @nmurf! You can already do this with current Polly. You can use a wait-and-retry overload taking the returned response as input to the sleepDurationProvider delegate. Something like:

IAsyncPolicy<HttpResponseMessage> retryHonouringRetryAfter =
    Policy.Handle<HttpRequestException>
        .OrResult<HttpResponseMessage>(r => /* clauses for other status codes you want to handle */
            || r.StatusCode == (HttpStatusCode)429) // RetryAfter
        .WaitAndRetryAsync(
            retryCount: 3, 
            sleepDurationProvider: (retryCount, response, context) => {
                // taking the pseudocode from the question
                var serverWaitDuration = getServerWaitDuration(response.Result?.Headers.Get("Retry-After")); // explaining response.Result? : The response is a Polly type DelegateResult<HttpResponseMessage>. Use null defence, as response.Result will be null if the cause for retry was an exception (in which case there was on result).
                var waitDuration = Math.Max(clientWaitDuration.TotalMilliseconds, serverWaitDuration.TotalMilliseconds);
                return TimeSpan.FromMilliseconds(waitDuration); 
            },
            onRetryAsync: async (response, timespan, retryCount, context) => { 
                /* perhaps some logging, eg the retry count, the timespan delaying for */ 
            }
        );

This is pseudo-code (for speed of response) : you may have to vary some details. For example, Azure Cosmos DB client SDK will wrap this response up in an Exception with a RetryAfter property after making its own retries first. We'd love to see your final version!

This is documented in the Polly wiki here but I'll aim to reference that also from the main Polly readme, to make it more discoverable. More info also in this similar question.

Let us know how you get on!

@reisenberger reisenberger changed the title Respect Retry-After HTTP header [already possible] Respect Retry-After HTTP header Mar 9, 2018
@nmurf
Copy link
Author

nmurf commented Mar 9, 2018

Thanks for the quick response, @reisenberger! Great library -- I did not notice the provider overload until you pointed it out.

For posterity, here is an implementation of the method to obtain the header value as a TimeSpan (assumes server is in UTC):

private TimeSpan getServerWaitDuration(DelegateResult<HttpResponseMessage> response)
{
	var retryAfter = response?.Result?.Headers?.RetryAfter;
	if (retryAfter == null)
		return TimeSpan.Zero;
	
	return retryAfter.Date.HasValue
		? retryAfter.Date.Value - DateTime.UtcNow
		: retryAfter.Delta.GetValueOrDefault(TimeSpan.Zero);
}

Here's the rest:

var clientWaitDurations = DecorrelatedJitter(numRetries, firstWaitDuration.Value, maxWaitDuration.Value)
	.ToArray();

_policy = Policy
	.Handle<HttpRequestException>()
	.OrResult(filter)
	.WaitAndRetryAsync(
		retryCount: numRetries,
		sleepDurationProvider: (retryCount, response, context) => 
			TimeSpan.FromMilliseconds(
				Math.Max(
					clientWaitDurations[retryCount - 1].TotalMilliseconds, 
					getServerWaitDuration(response).TotalMilliseconds));

@nmurf nmurf closed this as completed Mar 9, 2018
@reisenberger
Copy link
Member

reisenberger commented Mar 21, 2018

Thanks @nmurf for sharing this!

One thought: enumerating DecorrelatedJitter(...) with .ToArray() will cause the randomisation of delay times (the jitter) to be executed only once and stored in the array. All executions through the policy would then get that same jitter sequence (so jitter across executions would be 'correlated' across concurrent calls rather than 'decorrelated', if you like).

I see the challenge though created by the fact that responding to RetryAfter naturally requires the sleepDurationProvider: Func<..., TimeSpan> overload, while jitter naturally requires a sleepDurations: IEnumerable<TimeSpan>.

One approach to solve this could be to split the original policy into two retry policies, and then combine both retry policies in a PolicyWrap. For example:

_retryAfterPolicy = Policy
	.HandleResult<HttpResponseMessage>(r => r?.Headers?.RetryAfter != null) // EDIT: .Net Framework only: https://msdn.microsoft.com/en-us/library/system.net.http.headers.httpresponseheaders.aspx;  not yet in this form in .NET Core?
	.WaitAndRetryAsync(
		retryCount: numRetries,
		sleepDurationProvider: (retryCount, response, context) => getServerWaitDuration(response)
        );

_generalRetryPolicy = Policy.Handle<HttpRequestException>()
	.OrResult(filter) // omit the RetryAfter case now from 'filter'!
	.WaitAndRetryAsync(sleepDurations: DecorrelatedJitter(numRetries, firstWaitDuration.Value, maxWaitDuration.Value));

_policy = _generalRetryPolicy.WrapAsync(_retryAfterPolicy);

_policy will then handle retry-after cases (when they occur) with the exact retry-after duration returned by the server, and other retry cases with the jitter - with that jitter being decorrelated between every execution through the policy.

A minor edge-case with the two retry policies nested in a PolicyWrap is that specific sequences of responses from the server (mixing retry-after and other) could cause more than numRetries tries to be made overall. From a real-world perspective and for small values of numRetries this is probably unlikely and insignificant enough to not wring hands over, but it's worth pointing out for completeness.

If anyone can suggest further refinements, please do add them to this thread!

@nmurf
Copy link
Author

nmurf commented Mar 22, 2018

@reisenberger, good catch with the .ToArray() causing correlation.

I think the correct solution here is to generate the sleep durations as needed. I don't have time to test this at the moment, but this is what I am thinking:

var jitterer = new Random();

var policy = Policy
	.Handle<HttpRequestException>()
	.OrResult(filter)
	.WaitAndRetryAsync(
		retryCount: numRetries,
		sleepDurationProvider: (retryCount, response, context) =>
		{
			object lastWaitDuration;
			if (!context.TryGetValue("lastWaitDuration", out lastWaitDuration))
			{
				lastWaitDuration = firstWaitDuration.Value;
			}

			var clientWaitDuration = GetNextDecorrelatedWaitDuration(
				jitterer, firstWaitDuration.Value, (TimeSpan) lastWaitDuration, maxWaitDuration.Value);

			context["lastWaitDuration"] = clientWaitDuration;

			return TimeSpan.FromMilliseconds(
				Math.Max(
					clientWaitDuration.TotalMilliseconds,
					getServerWaitDuration(response).TotalMilliseconds));
		});

Here's the new jitter method. It could be tweaked if you need to support 64 bit millisecond durations.

private static TimeSpan GetNextDecorrelatedWaitDuration(Random jitterer,
	TimeSpan seedDelay, TimeSpan lastDelay, TimeSpan maxDelay)
{
	var seed = (int)seedDelay.TotalMilliseconds;
	var last = (int)lastDelay.TotalSeconds;
	var cap = (int)maxDelay.TotalMilliseconds;

	var current = Math.Min(cap, jitterer.Next(seed, last * 3));
	return TimeSpan.FromMilliseconds(current);
}

@reisenberger
Copy link
Member

reisenberger commented Mar 22, 2018

@nmurf Yes, I figured yesterday that the two policies could be joined back together by sharing state through Context. Thanks for sharing your approach!

@ikabar
Copy link

ikabar commented Jul 9, 2018

How can we use retryafter with retry forever + timeout?
Say I want to poll a result and the server returns the retryafter.
I want to wait until the result is available and no longer then X minutes. I want the retry interval to be set according to the server response.

@reisenberger
Copy link
Member

@ikabar Use PolicyWrap (see that wiki about it), and wrap a TimeoutPolicy (for X minutes) outside of the retry-after retry policy. This is what PolicyWrap is designed for: combining policies.

@WorldMaker
Copy link

What about 429's that include the retry after in the JSON message body? It doesn't appear that there is a sleepDurationProviderAsync to try to read the body contents.

@reisenberger
Copy link
Member

@WorldMaker Yes, that's correct: current Polly does not offer a sleepDurationProviderAsync variant.

You should still be able to handle that case with current Polly by placing that read-body logic, for the 429 case, in the delegate passed to ExecuteAsync(...).

At present, we are preferring not to add new feature-variants like sleepDurationProviderAsync to Polly, where there is an existing workround, until after the projected syntax refactor, ie in favour of prioritising that refactor. (I hope to embark this soon; but it is a major project so I cannot definitively confirm.).

Thanks again for the question!

@zstarr
Copy link

zstarr commented Apr 28, 2022

What would be the updated way to handle this? I see on May 6th these are out of date, and I'm getting an error when trying it.

I wrote a custom policy and added it with:
.AddPolicyHandler(GetRetryAfterPolicy());

Here is my attempted implementation to get access to RetryAfter.Delta:

static IAsyncPolicy<HttpResponseMessage> GetRetryAfterPolicy()
{
    return Policy.HandleResult<HttpResponseMessage>
        (msg =>  msg.Headers.RetryAfter != null)
        .WaitAndRetryAsync(
            retryCount: 3,
            sleepDurationProvider: (retryCount, response, context) =>
            {
                return response.Result.Headers.RetryAfter.Delta.Value;
            }
        );
}

The error it gives me is:
Error CS1929 'PolicyBuilder<HttpResponseMessage>' does not contain a definition for 'WaitAndRetryAsync' and the best extension method overload 'AsyncRetrySyntax.WaitAndRetryAsync(PolicyBuilder, int, Func<int, TimeSpan>)' requires a receiver of type 'PolicyBuilder'

Extra context if it helps: Slack API returns a 429 when being rate limited with a delta in seconds to wait. I'd love to use that delta instead of doing just exponential waits. Currently I'm just using a for loop and reading the header without Polly to make it work.

@martincostello
Copy link
Member

@zstarr This should achieve your goal - the missing bit is that you haven't specified the onRetryAsync parameter.

static IAsyncPolicy<HttpResponseMessage> GetRetryAfterPolicy()
{
    return Policy.HandleResult<HttpResponseMessage>
        (msg => msg.Headers.RetryAfter is not null)
        .WaitAndRetryAsync(
            retryCount: 3,
            sleepDurationProvider: (_, response, _) => response.Result.Headers.RetryAfter.Delta.Value,
            onRetryAsync: (_, _, _, _) => Task.CompletedTask
        );
}

@zstarr
Copy link

zstarr commented Apr 29, 2022

That fixed it, thanks!
I had tried with an onRetryAsync: () => but not with 4 _'s.

@App-vNext App-vNext locked as resolved and limited conversation to collaborators Apr 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants