-
Notifications
You must be signed in to change notification settings - Fork 759
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
[API Proposal, Microsoft.Extensions.Http.Resilience]: API for removing and replacing the standard resilience/hedging handlers #5695
Comments
Looks great! Just one note here. Given that having multiple standard resilience handlers is anti-pattern and almost always a bug, rather than introducing Essentially, calling |
Looks good to me. I agree with @martintmk, if having multiple handlers is a bug anyway, I think it's a legit to change the behavior of existing methods. |
Good point @martintmk. Actually, I was proposing that in this comment #5021 (comment). But then I started thinking of HttpClient's delegating handlers as a list, therefore I decided that it could be confusing that the method |
@iliar-turdushev, I'm marking this as ready for the ARB, please coordinate with @terrajobst for the suitable time. |
@RussKie At the moment, I'd like to continue discussion here. Especially, given the comments above (#5695 (comment) and #5695 (comment)) we might end up introducing the only method BTW Do we really need for every new API have that online API review session? As far as I know, for some APIs that are not that complex, we can discuss and approve it without bringing it to the online API review session, right? |
The rules are somewhat fluent. But in general, if we're adding an API which more-or-less looks or behaves like an existing API (e.g., provide a new implementation to an interface, which already has other implementations), then we can have a discussion about the nuances of the implementation and pretty much rubber-stamp it. However, for brand new API we, generally, want the API Review Board to grace with their approval because the ARB may provide us with perspectives we may miss. This proposal looks to me as though it falls in the latter category. Please correct me, if I misread it. |
My understanding is that We generally don't review |
@RussKie Thank you for the information. No, you didn't misread that :). I think that it would be helpful to have API Review team's perspective on the proposed API. |
@terrajobst I just want to clarify. By saying |
Background and motivation
.NET provides the
ConfigureHttpClientDefaults
method that allows you to configure the default behavior of theHttpClient
. With the help of this method you can register theStandardResiliencePipeline
as part of the default configuration of theHttpClient
:This is convenient, since you don't need to configure the resilience pipeline for each
HttpClient
. But with the API we currently provide users face a few issues/challenges when they need to change the default configuration. Below are a couple of known issues/challanges.Issue 1: Removing the existing default resilience pipeline and adding a custom one
For example, a user registers the
StandardResilienceHandler
as default configuration, but at the same time he/she wants to use theStandardHedgingHandler
for a particular namedHttpClient
:Currently, there is no API that allows users to remove the
StandardResilienceHandler
and register a new one. Now users can work that around as described here #4814 (comment).Issue 2: Override configuration of the default resilience pipeline for a particular
HttpClient
A user registers the
StandardResilienceHandler
as default configuration, and wants to provide custom configuration for a particularHttpClient
:Currently, there is no API to override the default configuration, see #4814.
Issue 3: Replace the default resilience pipeline with a custom one
In oppose to the "Issue 1" a user wants to replace the default resilience pipeline with a custom one. He/she wants to replace it, because it might be important to keep the order where the default resilience pipeline was registered, for example, it could be important for metering purposes.
Supporting such scenario could be useful to fix issues like the following #5021. Because the default resilience pipeline and its state are shared by all
HttpClient
instances it could lead to issues, like the one mentioned in the previous sentence. And having an ability to replace the default pipeline with a custom one for a particularHttpClient
could be a solution to that issue.API Proposal
Remove all resilience handlers:
Add or replace the
StandardResilienceHandler
:Add or replace the
StandardHedgingHandler
:API Usage
With the help of the proposed API the issues described in the beginning could be fixed as following.
Issue 1: Removing the existing default resilience pipeline and adding a custom one
Issue 2: Override configuration of the default resilience pipeline for a particular
HttpClient
Issue 3: Replace the default resilience pipeline with a custom one
Alternative Designs
The API design above considers HttpClient's delegating handlers as a list (the order of the items is important), therefore it introduces the
AddOrReplace
methods that implicitly "say" that the standard resilience handler (which is a delegating handler) will be replaced if any. Otherwise, it will be added to the end of the list.The alternative design suggests not to introduce
AddOrReplace
methods, but instead to implementAddOrReplace
semantics in the existingAdd
methods. The justification behind that is: having more than one standard resilience handlers in the HttpClient is INCORRECT.Since it is not correct to have more than one standard resilience handlers in the HttpClient, we consider reasonable to implement
AddOrReplace
semantics in the existingAdd
methods:With such an approach we'll need to introduce the only method
RemoveAllResilienceHandlers
mentioned above, and then the issues described above could be fixed as following:Issue 1: Removing the existing default resilience pipeline and adding a custom one
Issue 2: Override configuration of the default resilience pipeline for a particular
HttpClient
Issue 3: Replace the default resilience pipeline with a custom one
Risks. It could be confusing for customers that
Add
methods hasAddOrReplace
semantics. We'll need to properly document that.Risks
There are a few risks:
1. Names of the methods
AddOrReplace...
are not perfectNames
AddOrReplaceStandardResilienceHandler
andAddOrReplaceStandardHedgingHandler
are not perfect. The suffixesReplaceStandardResilienceHandler
andReplaceStandardHedgingHandler
might bring confusion that the methods replace only the corresponding standard resilience handler type, i.e. either the Resilience handler or the Hedging handler. In fact each method replaces any of the standard handlers. So the names of the methods don't fully describe what the methods do. The name that will fully describe the methods would beAddStandardResilienceHandler_Or_ReplaceAnyStandardHandlerWithStandardResilienceHandler
, but it is too long, therefore we have to find some balance.2. We'll need to introduce a breaking change to the behavior of the existing methods registering standard Resilience and Hedging handlers
In order to introduce
AddOrReplace
methods we'll need to ensure that HttpClient request pipeline has only one Resilience or Hedging handler. For that we'll need to change the behavior of the existing methods registering Resilience and Hedging handlers in HttpClient request pipeline. In particular, the new behavior will throw an exception when a user attempts to register one of those handlers twice. Currently, we don't throw an exception but register the handler twice which is also not correct. Therefore this risk could be not so high/important.3. Additional effort to maintaining new
AddOrReplace
methodsWith the introduction of new
AddOrReplace
methods we'll have to maintain them, meaning that adding some capability to existingAddStandardResilienceHandler
methods would result in mirroring it to theAddOrReplace
methods.Currently we have 2 standard handlers: Resilience and Hedging. Probably, introducing a new standard handler would lead to creating similar
AddOrReplace
methods to have feature parity.The text was updated successfully, but these errors were encountered: