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

Add API allowing to disable retries for a given list of HTTP methods #5634

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

iliar-turdushev
Copy link
Contributor

@iliar-turdushev iliar-turdushev commented Nov 13, 2024

The PR introduces an API that allows to disable automatic retries for

  1. a given list of HTTP methods;
  2. a list of unsafe/non-readonly HTTP methods: POST, PATCH, PUT, DELETE, and CONNECT.

Here is the approved API #5248 (comment).

Closes #5248.

Adds APIs allowing to disable automatic retries for a given list of HTTP methods
/// Disables retry attempts for POST, PATCH, PUT, DELETE, and CONNECT HTTP methods.
/// </summary>
/// <param name="options">The retry strategy options.</param>
public static void DisableForUnsafeHttpMethods(this HttpRetryStrategyOptions options)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, we can return HttpRetryStrategyOptions to make this and other extension-methods configuring HttpRetryStrategyOptions "chainable". Honestly, at the moment I don't see a necessity for that, but probably in future we'll want to introduce more API and make them fluent.

I would personally leave the API as-is returning void. What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more a question for the API Review Board. Here are the general rules: https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/breaking-change-rules.md.

I think that changing the return type from void to T should be an acceptable change in the future, but I may be wrong... @terrajobst @bartonjs what's your take on this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that changing the return type from void to T should be an acceptable change in the future, but I may be wrong...

Methods in .NET are named for their full signature, including the declared return type. So any callers from before you change it will get a MissingMethodException if they run against a library after you change it.

If you want to change it between previews that will annoy your users, but you can do it (with a breaking change notice). Once you abandon the preview label it is what it is forever (you'd have to leave this one as-is and make some other overload that happens to return the data, but clearly can't have the same calling signature).

Copy link
Member

@RussKie RussKie Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @bartonjs, for clarifications.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, in that breaking change guidelines doc, it says it's OK to change the return value, but that it's not OK to change the return type.

Returning a value of a more derived type for a property, field, return or out value

Note, again, that the static type cannot change. e.g. it is OK to return a string instance where an object was returned previously, but it is not OK to change the return type from object to string.

Emphasis mine

@iliar-turdushev iliar-turdushev marked this pull request as ready for review November 13, 2024 14:10
@iliar-turdushev iliar-turdushev requested a review from a team as a code owner November 13, 2024 14:10
if (result &&
args.Outcome.Result is HttpResponseMessage response &&
response.RequestMessage is HttpRequestMessage request)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need this condition to satisfy the compiler because RequestMessage is nullable HttpRequestMesssage?. I don't think in reality it could be null. Alternatively, we can throw an exception here, but I think it is better not to fail here, but rather let the original ShouldHandle predicate to take a decision whether to do retries or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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