Fix default HTTP resilience not to retry non-idempotent requests (POST, PUT, etc.)

[SteveSandersonMS]

Original issue description added by @SteveSandersonMS

Currently if a backend service takes a while to handle a request (e.g., > 5 sec), the default HTTP resilience policy will retry.

This is fine for idempotent requests (e.g., GET), but dangerous/harmful for other request types (POST, PUT). While building eShopSupport I often encountered cases where it would create duplicate messages/tickets just because either:

[1] I was debugging, and hence causing things to be slow
[2] Or, some lower-level service was still starting up and loading an AI model or similar, causing it to be temporarily delayed

It's fine for developers to configure a retry policy for POST/PUT/etc themselves, but I don't think the default should be to retry those and was very surprised when finding that it is.

---

API Proposal added by @iliar-turdushev

Background and motivation

Microsoft.Extensions.Http.Resilience package provides a delegating handler called StandardResilienceHandler that applies various resilience strategies (retry, timeout, rate limiting) to HttpClient. For example, it automatically makes retries of an HTTP request if it fails with a transient error. You can register StandardResilienceHandler for HttpClient as following:

services.AddHttpClient("test").AddStandardResilienceHandler();

or you can use an overload that accepts options configuring resilience strategies:

services.AddHttpClient("test").AddStandardResilienceHandler(options =>
{
    // You can configure resilience options here.
});

Each resilience strategy of StandardResilienceHandler has a predicate deciding whether to apply the strategy or not based on the outcome of an HTTP request, i.e. whether the outcome is a transient error or not. Retry strategy by default is configured to make retries for all HTTP methods, i.e. it will make retries for both idempotent and non-idempotent HTTP methods. That brings the issue described above at the beginning of this comment. While the current default behavior might be undesired for some services, we don't want to change it because:

• In general, we cannot make assumptions about idempotency of APIs based on HTTP methods being used to access those APIs.
• Such a change will be breaking and would lead to unpleasant results for existing services that already use StandardResilienceHandler.

As a compromise solution to the issue we would like to introduce a new API allowing to easily disable the default behavior and don't do retries for non-idempotent HTTP methods.

API Proposal

namespace Microsoft.Extensions.Http.Resilience;

/// <summary>
/// Extensions for HttpRetryStrategyOptionsExtensions.
/// </summary>
public static class HttpRetryStrategyOptionsExtensions
{
    /// <summary>
    /// Disables retries for POST, PATCH, PUT, DELETE, and CONNECT.
    /// </summary>
    /// <param name="option">Retry strategy options.</param>
    public static void DisableForUnsafeHttpMethods(this HttpRetryStrategyOptions option);

    /// <summary>
    /// Disables retries for the given list of HTTP methods.
    /// </summary>
    /// <param name="options">Retry strategy options.</param>
    /// <param name="methods">List of HTTP methods.</param>
    public static void DisableFor(this HttpRetryStrategyOptions options, params HttpMethod[] methods);
}

According to RFC safe HTTP methods are those whose semantics is read-only. POST, PATCH, PUT, DELETE, and CONNECT are unsafe. While for PUT and DELETE doing retries might be safe, we propose an API method DisableForUnsafeHttpMethods disabling retries for all unsafe HTTP methods, i.e. HTTP methods that could be harmful for the server. Additionally, to have some flexibility we want to introduce a method DisableFor allowing users to specify which HTTP methods their service considers as non-retriable.

API Usage

DisableForUnsafeHttpMethods:

services.AddHttpClient("test").AddStandardResilienceHandler(options =>
{
    options.Retry.DisableForUnsafeHttpMethods();
});

DisableFor:

services.AddHttpClient("test").AddStandardResilienceHandler(options =>
{
    options.Retry.DisableFor(HttpMethod.Post, HttpMethod.Patch);
});

Alternative Designs

Extend existing AddStandardResilienceHandler methods with an argument accepting a list of HTTP methods for which users don't want to do retries.

API:

namespace Microsoft.Extensions.DependencyInjection;

public static partial class ResilienceHttpClientBuilderExtensions
{
    // Registers StandardResilienceHanlder in the given IHttpClientBuilder and configures its retry strategy to not do retries for the given list of HTTP methods.
    public static IHttpStandardResiliencePipelineBuilder AddStandardResilienceHandler(this IHttpClientBuilder builder, params HttpMethod[] nonIdempotentHttpMethods);

    // Like the first method + accepts a delegate to configure resilience options.
    public static IHttpStandardResiliencePipelineBuilder AddStandardResilienceHandler(this IHttpClientBuilder builder, Action<HttpStandardResilienceOptions> configure, params HttpMethod[] nonIdempotentHttpMethods);

    // Like the first method + accepts a configuration section to configure resilience options.
    public static IHttpStandardResiliencePipelineBuilder AddStandardResilienceHandler(this IHttpClientBuilder builder, IConfigurationSection section, params HttpMethod[] nonIdempotentHttpMethods);
}

Usage:

services.AddHttpClient("test").AddStandardResilienceHandler(HttpMethod.Post, HttpMethod.Patch);

The downside of this approach is that:

• We have to duplicate each overload of AddStandardResilienceHandler method with an argument accepting a list of non-idempotent HTTP methods.
• A list of non-idempotent HTTP methods affects only Retry strategy, but we pass it to a method registering a resilience handler also combining other resilience strategies, e.g. timeout, rate limiting, circuit breaker.

Risks

Not aware of any risks.

[davidfowl]

@joperezr This should be built into the resiliency package.

[iliar-turdushev]

Changing the default behavior to not retry non-idempotent calls would be a breaking change, since some customers already rely on current default behavior.

What if we introduce a new method checking whether the operation is retriable or not (we already have a class for such methods):

// Returns true if the outcome of an operation is Transient and Retriable, otherwise - false.
public static bool IsRetriable(Outcome<HttpResponseMessage> outcome, CancellationToken token);

and then users would need to use it to change default behavior of Retry strategy:

services
    .AddHttpClient()
    .AddStandardResilienceHandler(options =>
    {
        options.Retry.ShouldHandle = args =>
            new ValueTask<bool>(HttpClientResiliencePredicates.IsRetriable(args.Outcome, args.Context.CancellationToken));
    });

What do you think?

[martincostello]

Maybe there's scope for a helper built on top of that like options.Retry.DisableRetriesForNonIdempotentRequests() or options.Retry.EnableOnlyForHttpMethods([...])? It's a terrible name and might not be the right shape, but that's the idea I'm getting at. It feels a bit verbose to get the user to use the snippet above as-is for the default case. They could always do something like that if they want to customise further or be more granular.

[davidfowl]

Changing the default behavior to not retry non-idempotent calls would be a breaking change, since some customers already rely on current default behavior.

I understand but this feels like a bug.

[RussKie]

Only POST and CONNECT methods are not idempotent. Please refer to RFC 7231.

Summarizing, the HTTP methods are classified as following:

+---------+------+------------+
| Method  | Safe | Idempotent |
+---------+------+------------+
| CONNECT | no   | no         |
| DELETE  | no   | yes        |
| GET     | yes  | yes        |
| HEAD    | yes  | yes        |
| OPTIONS | yes  | yes        |
| POST    | no   | no         |
| PUT     | no   | yes        |
| TRACE   | yes  | yes        |
+---------+------+------------+

Please refer to https://stackoverflow.com/a/45019073/2338036 for more information.

[RussKie]

I like the idea of disabling retry for non-idempotent HTTP methods by default and giving the flexibility of enabling/disabling retry for any HTTP method.

Since this could be a breaking change, we should bake it in to the .NET 9 release (the "dev" branch) before we snap (i.e., make the dev the main). This will be happening in a matter of days.

[iliar-turdushev]

I like the idea of disabling retry for non-idempotent HTTP methods by default and giving the flexibility of enabling/disabling retry for any HTTP method.

Since this could be a breaking change, we should bake it in to the .NET 9 release (the "dev" branch) before we snap (i.e., make the dev the main). This will be happening in a matter of days.

Do I understand it correctly that we are allowed to introduce breaking changes in major versions? If so, then we can disable retries for non-idempotent HTTP methods by default in .NET 9. In addition to POST and CONNECT we need to disable retries for PATCH which may be non-idempotent, see RFC 5789, Chapter 2, The PATCH method. I think at this stage we don't need to introduce new API to enable/disable retries for various HTTP methods. If a user needs that he/she we'll be able to achieve that using current API as shown above #5248 (comment). If users ask to introduce more convenient API for enabling/disabling retries for HTTP methods we'll consider their ask reactively.

Hedging has the same issue. Hence we'll need similarly disable non-idempotent HTTP methods for hedging by default.

To summarize above:

• In .NET 9 we'll disable retries and hedging for POST, CONNECT, and PATCH HTTP methods.
• We'll not introduce new API for enabling/disabling retries or hedging for any HTTP methods. Currently users are able to achieve that using existing API.

Question: What is the best way to communicate such a breaking change, so that users will not miss it?

What do you think?

cc @geeknoid @martintmk

[RussKie]

Question: What is the best way to communicate such a breaking change, so that users will not miss it?

Our release notes and the docs.
We can/should probably document the breaking changes at https://learn.microsoft.com/dotnet/core/compatibility/9.0; we'll need to work with the Docs team.

[martintmk]

In .NET 9 we'll disable retries and hedging for POST, CONNECT, and PATCH HTTP methods.
We'll not introduce new API for enabling/disabling retries or hedging for any HTTP methods. Currently users are able to achieve that using existing API.

While I am not fan of breaking changes such as this, this change seems waranted.

// Returns true if the outcome of an operation is Transient and Retriable, otherwise - false.
public static bool IsRetriable(Outcome outcome, CancellationToken token);

The semantic meaning is the same as the existing IsTransient method. I wouldn't introduce a new method. What about a new overload with parameter such as evaluateHttpMethods. By default, it will be enabled, but existing clients can revert back to old behavior by doing:

services
    .AddHttpClient()
    .AddStandardResilienceHandler(options =>
    {
        options.Retry.ShouldHandle = args =>
            new ValueTask<bool>(HttpClientResiliencePredicates.IsTransient(
                  args.Outcome,
                  evaluateHttpMethods: false,
                  cancellationToken: args.Context.CancellationToken));
    });

Our release notes and the docs. We can/should probably document the breaking changes at https://learn.microsoft.com/dotnet/core/compatibility/9.0; we'll need to work with the Docs team.

For internal Microsoft services, this needs to be a very clearly documented because this has a potential to cause issues. They need to be aware before upgrading.

[geeknoid]

I don't think we should change the default behavior that exists today, for two reasons:

• The actual idempotence/non-idempotence of API calls is very often not coupled to the actual HTTP verbs. Making broad assumptions based on the verb is simply wrong in the real world.

• Given the above, it would be a potential very unpleasant surprise for services designed around the current behavior to suddenly receive different behavior due to some dependency upgrades.

I think we should keep exactly the behavior we have today, document it, and make it super-easy to disable it when needed.

[iliar-turdushev]

I agree with @geeknoid. Let's not change the behavior we have and try and make disabling retries for non-idempotent HTTP methods easier. What if we introduce an extension method on HttpRetryStrategyOptions allowing to disable retries for a given list of HTTP methods (as @martincostello suggested):

// Disables retries for a given list of HTTP methods.
public static void DisableForHttpMethods(this HttpRetryStrategyOptions options, params HttpMethod[] httpMethods);

And the usage will be:

services
    .AddHttpClient()
    .AddStandardResilienceHandler(options => options.Retry.DisableForHttpMethods(HttpMethod.Post, HttpMethod.Patch));

We can also introduce more specific methods:

// Disables retries for POST, PATCH, and CONNECT.
public static void DisableForNonIdempotentHttpMethods(this HttpRetryStrategyOptions options);

// Disables retries for POST, PATCH, PUT, DELETE, and CONNECT.
public static void DisableForUnsafeHttpMethods(this HttpRetryStrategyOptions option);

And we will introduce similar API for hedging because it also hedges requests regardless of the HTTP method.

What do you think? @davidfowl @martintmk @geeknoid

[davidfowl]

At a minimum let's make a new top-level method where this is the default behavior. That seems like a reasonable compromise.