2
\$\begingroup\$

I am writing a small library where I don't want to include Polly since I want to keep the number of dependencies as small as possible. I also thought it was a good learning exercise to do this on my own since I'm learning the language. However if my implementation is subpar, I might consider Polly instead.

My goal is to have a handler I can attach to any HTTP client where the target RequestUri of the client has a recommendation on this type of retry policy. I don't want to throw exceptions, and I want to return the last failed status if retries are exhausted. If retries are not exhausted, I want to return the result I received.

I would like to know if this is safe to use, if I have missed something or if there are other improvements to be made. I would also like to know if I'm following the conventions of the language properly.

It should work like this:

  • Retry a maximum of MaxRetries times if the response status is in RetryableStatusCodes or if the client throws HttpRequestException
  • If the HttpRequestException contains status code, also verify that it should be retried
  • If HttpRequestException does not contain a status, retry anyway
  • If retries are exhausted, return the last HttpResponseMessage
  • If only receiving exceptions, return a custom HttpResponseMessage to have something to return

Here's what I have:

using System.Net;

namespace MyNamespace.Http;

public abstract class ExponentialBackoffHandler : DelegatingHandler
{
    public abstract int MaxRetries { get; }
    public abstract int MinBackoffMs { get; }
    public abstract int MaxBackoffMs { get; }
    public abstract List<int> RetryableStatusCodes();

    protected override async Task<HttpResponseMessage> SendAsync(
        HttpRequestMessage request,
        CancellationToken ct)
    {
        Func<HttpRequestMessage, Task<HttpResponseMessage>> func = async (req) =>
        {
            var backoff = new ExponentialBackoff(MinBackoffMs, MaxBackoffMs);
            HttpResponseMessage? response = null;
            for (int i = 0; i < MaxRetries; i++)
            {
                try
                {
                    response = await base.SendAsync(request, ct);
                    if (response.IsSuccessStatusCode)
                    {
                        return response;
                    }

                    if (ShouldRetry(i, response.StatusCode))
                    {
                        await backoff.Delay(ct);
                    }
                }
                catch (HttpRequestException e)
                {
                    // Force the return of the custom response on the last attempt
                    if (ShouldRetry(i + 1))
                    { 
                        await backoff.Delay(ct);
                    }
                    else
                    {
                        response = new HttpResponseMessage
                        {
                            StatusCode = e.StatusCode ?? HttpStatusCode.InternalServerError,
                            Content = new StringContent(e.Message)
                        };
                        break;
                    }
                }
            }

            return response!;
        };

        return await func(request);
    }

    private bool ShouldRetry(int attempt, HttpStatusCode? status = null)
    {

        bool hasRetriesLeft = attempt < MaxRetries;
        bool isStatusRetryable = status != null ? RetryableStatusCodes().Contains((int)status) : true;
        return hasRetriesLeft && isStatusRetryable;
    }
}

struct ExponentialBackoff
{
    private readonly int _delayMilliseconds;
    private readonly int _maxDelayMilliseconds;
    private int _retries;

    public ExponentialBackoff(int delayMilliseconds, int maxDelayMilliseconds)
    {
        _delayMilliseconds = delayMilliseconds;
        _maxDelayMilliseconds = maxDelayMilliseconds;
        _retries = 0;
    }

    public Task Delay(CancellationToken ct)
    {
        var delay = TimeSpan.FromMilliseconds(
            Math.Min(_delayMilliseconds * Math.Pow(2, ++_retries) / 2, _maxDelayMilliseconds)
        );
        return Task.Delay(delay, ct);
    }
}

You implement it like this:

class MyExponentialBackoffHandler : ExponentialBackoffHandler
{
    public override int MaxRetries => 3;
    public override int MinBackoffMs => 1;
    public override int MaxBackoffMs => 10;
    public override List<int> RetryableStatusCodes()
    {
        return new() { 404 };
    }
}

And then in the DI wiring:

services
    .AddTransient<MyExponentialBackoffHandler>()
    .AddHttpClient<MyHttpClient>()
    .AddHttpMessageHandler<MyExponentialBackoffHandler>();

I also have a bunch of tests for this if that would be useful.

Thank you for your time, looking forward to your input.

\$\endgroup\$
6
  • 1
    \$\begingroup\$ Retrying against 404 might be a good idea only if you are waiting for a resource to be created asynchrounsly. Otherwise it is not considered a transient failure. \$\endgroup\$ Commented Mar 12, 2023 at 7:43
  • \$\begingroup\$ The API I'm implementing actually considers 404 to be a transient error, interestingly. But above is just an example, there will be more than 404 to consider in reality. \$\endgroup\$
    – jokarl
    Commented Mar 12, 2023 at 8:14
  • 1
    \$\begingroup\$ Currently I'm on holiday until Thursday. After that I can provide you suggestions for correctness, flexibility and composability. \$\endgroup\$ Commented Mar 12, 2023 at 10:32
  • \$\begingroup\$ Thank you, there is no rush! \$\endgroup\$
    – jokarl
    Commented Mar 12, 2023 at 14:31
  • \$\begingroup\$ The only suggestion I'd make is to not "hardcode" to milliseconds and use TimeSpan types instead. Otherwise, it's nice clean code that is self-describing. \$\endgroup\$ Commented Mar 13, 2023 at 16:09

2 Answers 2

1
\$\begingroup\$

In this post I would like to focus on correctness, extensibility and composability.

Correctness

Most of the time whenever we want to make sure that a given implementation works correctly then we write unit (and integration) tests to assure that the observed and expected behaviours are the same.

Which is good, but sometimes it is not enough. Currently that's the case. You have written a generic retrier which nicely integrates with HttpClient. What's the problem with this from correctness point of view?

In order to be able to answer the above question, let's see the prerequisites of retry:

  1. The potentially introduced observable impact is acceptable
  2. The operation can be redone without any irreversible side effect
  3. The introduced complexity is negligible compared to the promised reliability

Does your implementation satisfy all these? Let's see one-by-one:

  1. Because you did not specify explicitly any time-cap for each retry attempt, that's why it might happen that 3 retries could take several minutes (downstream having hard-time to respond to requests). If you are using the decorated HttpClient for a background operation then it might be acceptable. If you are using the decorated HttpClient for user-facing operation most probably (but depends on the use case) it is not acceptable and tolerable.

    • Possible solution: Use timeout and/or circuit breaker.
  2. Because you have decorated the entire HttpClient with the retrier that's why all operations can (and will) be retried. Which can be problematic if the downstream's endpoint is not implemented in an idempotent way. Like many times the POST verb is not idempotent.

    • Possible solution: Use typed client. You can decorate the underlying HttpClient with the retrier and use it only for idempotent operations (mostly GETs)
  3. In order to keep your logic simple, you've made the retry logic "dumb". It performs a retry whenever you receive HRE or whenever the status code is one of the retriable ones. But what if the server exposes throttling? It communicates to the client when should they perform retry with a custom 429 response.

    • At the current stage your code complexity is negligible compared to the promised reliability. But as you start to add more and more sophisticated logic to add resiliency to your communication protocol the answer might change.

The jitter

If you look at the best practices regarding retry logic you can easily bump into exponential backoff. Which is a good start, but it is just half of the story. With any retry logic (regardless it is linear or exponential or other) it can happen that they synchronise clients.

All clients start to perform retry at the exact same time against the recovered downstream service. Which might bring down again the service due to the flood of the requests.

If you would add a jitter to the sleep duration between retry attempts that could spread the clients in time. A jitter can be as simple as a random number between [-1.0, 1.0] or much more sophisticated.

I highly recommend to read the following article by AWS.

Extensibility

How easily can you extend the current implementation to support slightly different use cases? We are looking for answer to this question here.

Let me provide a couple of example use cases. This won't be an exhaustive list rather than just a couple of examples.

Using RetryAfter header

As I have already mentioned under the correctness section it might happen that the server is implemented in the way that it does support rate-limiting / throttling.

The service might respond with a 429 (Too Many Requests) status code and sets the RetryAfter response header to indicate when should the client issue the next retry attempt (to give space for self-healing for the service).

Your current handler can't be easily extended to support this scenario with inheritance.

Conditional retry

There are situations where you need to have branching logic to handle different circumstances differently. Even though at first glance retry does not fall under this category, but in reality it does. :D

Let's image the following scenario:

  • If the response's status code is 429 then retry only once by respecting RetryAfter
  • If the response's status code is 408 then retry 3times with exponential backoff
  • If the response's status code is 502/504 then retry indefinitely (until is succeeds) with fix interval

The current design does not support this.

Custom fallback

In your current implementation if all attempts failed then you return either with the last response or with a made-up HttpResponseMessage (depending on the control flow).

You might need to know consistently that all the retry attempts failed. Users of your retrier might want to provide custom fallback response in this case.

Observability

For observability purposes it might be useful to extend your current implementation with logging to know the distribution of the retries in time and which downstream service is the least reliable.


These were just a couple of examples. Adding support for all of these greatly increase complexity. So, you should evaluate again whether the introduced complexity is still negligible compared to the promised reliability.

Composability

IMHO the greatest feature of Polly is composability. You can chain multiple policies together to define resiliency strategies. A resiliency strategy is nothing more than a predefined protocol between clients and server to overcome transient failure (without causing harm to the other side).

A pretty common resiliency strategy looks like this:

Having a global timeout which overarches all retry attempts

Having a retry policy which is aware of the underlying circuit breaker's state

Having a circuit breaker which monitors the responses' "healthiness" to detect over-flooded/degraded downstream services

Having a local timeout for each request attempt

What I want to emphasize here is that retry is a good start, but please do not stop here. In order to have more robust solution to overcome transient errors you need to combine multiple primitives to achieve the desired behaviour.


Huh ... that's a lots of information. Thanks for reading it all. I hope it helped (at least a bit).

\$\endgroup\$
2
  • 1
    \$\begingroup\$ You are truly going above and beyond with this, much appreciated! The service I am integrating against is Azure Instance Metadata Service, and most of your suggestions apply to that. I will at least look into jitter and supporting the RetryAfter header (although I don't see that mentioned in the documentation) \$\endgroup\$
    – jokarl
    Commented Mar 20, 2023 at 15:26
  • \$\begingroup\$ @jokarl You are welcome. :) Please consider to mark one of my post as the answer if you are satisfied with the review. Thanks mate, have a great day! \$\endgroup\$ Commented Mar 20, 2023 at 16:47
1
\$\begingroup\$

My plan is to post multiple answers (on different days) because otherwise it would be pretty long post :D In this one I will focus on your implementation from coding perspective.

ExponentialBackoffHandler

abstract properties

  • I'm not really a huge fan of this pattern because in the inherited classes you have repeat everything just to set values
  • I would rather suggest to create init only properties with dedicated ctor
public int MaxRetries { get; init; }
public int MinBackoffMs { get; init; }
public int MaxBackoffMs { get; init; }

protected ExponentialBackoffHandler(int maxRetries, int minBackOffMS, int maxBackOffMs) 
  => (MaxRetries, MinBackOffMs, MaxBackOffMs) = (maxRetries, minBackOffMs, maxBackOffMs);
  • I would also suggest to use more appropriate data structure for these properties
public byte MaxRetries { get; init; }
public TimeSpan MinBackoff { get; init; }
public TimeSpan MaxBackoff { get; init; }
  • Because you have a ctor that's why you could validate the received Min and Max backoffs
    • Min should be less or equal than Max
    • Both should represent a positive time range
    • Both should not be extra large (like an hour, but depending on the use case)
    • etc.

RetryableStatusCodes method

  • This should be an init only property as well to avoid recurring memory allocation
    • Calling the method multiple times just to return the same collection is pointless

SendAsync

func

  • First of all func is a terrible name, please try to provide meaningful name for your variables
  • It captures the CancellationToken via closure which can cause a lots of headache if you are not doing it properly
  • I suggest to simply avoid this whole delegate, the SendAsync only does the followings:
    • defining the func
    • and calling it, like await func(request);

await base.SendAsync(request, ct);

  • Please bear in mind that HttpRequestMessage object is not reusable
  • Either you have to clone it before the first use or you have to recreate it in each and every retry attempt

catch (HttpRequestException e)

  • Please use ex or requestEx instead of e
  • Please bear in mind that IsSuccessStatusCode check will not throw HRE if status code is different than 2xx
    • You need to call EnsureSuccessStatusCode to receive an HRE

HttpResponseMessage? response

  • I think you don't need to have this variable at all
    • Inside the try simply use var response
    • Inside the catch return new HttpResponseMessage ... instead of setting response + break

ShouldRetry

  • I think it might make sense to have two separate methods here
    • One with HttpStatusCode parameter and one without
  • As an alternative it might make sense to encapsulate the attempt counting
    • Rather than receiving the current/next attempt counter as parameter the method itself could be responsible to increase the counter

ExponentialBackoff

Delay

\$\endgroup\$
3
  • 1
    \$\begingroup\$ Amazing input, thank you so much. I see that I fell into a few traps, especially about reusing the request. I will revise my solution and post an update when I'm done. \$\endgroup\$
    – jokarl
    Commented Mar 17, 2023 at 12:09
  • 1
    \$\begingroup\$ @jokarl Please bear in mind that here at CodeReview you should not change the question's code fragments after there is a post under it, because those observation would become pointless for future readers. So, if you want to ask a review for the updated version then either post a new question or use pastebin like codesharing sites to validate that the advices were implemented correctly. \$\endgroup\$ Commented Mar 17, 2023 at 12:14
  • 1
    \$\begingroup\$ @jokarl Also as I stated I will leave (at least one) more post(s) where I try to highlight some design faults. \$\endgroup\$ Commented Mar 17, 2023 at 12:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.