Open sveinungf opened 8 months ago
@martintmk Do you have time to take a look please?
The issue occurs because ConfigureHttpClientDefaults(x => x.AddStandardResilienceHandler())
registers a single resilience pipeline with a name standard
that is then shared by both SucceedingClient
and FailingClient
. As a result, they both share the circuit breaker of the standard
pipeline.
When we use two separate statements to register the resilience pipeline
succeedingClientBuilder.AddStandardResilienceHandler(section)
failingClientBuilder.AddStandardResilienceHandler(section)
we create two distinct resilience pipelines named SucceedingClient-standard
and FailingClient-standard
, and each of them has its own circuit breaker. Each of these pipelines is then used by the respective HttpClient
s.
The reason for that is how AddStandardResilienceHandler()
and ConfigureHttpClientDefaults()
methods work. AddStandardResilienceHandler()
relies on IHttpClientBuilder.Name
when registering and resolving the corresponding resilience pipeline. Whenever you call AddStandardResilienceHandler()
you register a resilience pipeline for the given IHttpClientBuilder.Name
, and when you create an HttpClient
the corresponding resilience pipeline is retrieved by the name or type of the HttpClient
, i.e. by the given IHttpClientBuilder.Name
. ConfigureHttpClientDefaults()
executes its lambda x => x.AddStandardResilienceHandler()
only once, and the x
argument is an IHttpClienBuilder
with the Name=null
, i.e. it doesn't have information about the actual name of HttpClient
. It doesn't execute the lambda for each registered HttpClient
. As a result, we have a single resilience pipeline (and hence a single shared circuit breaker) which is applied to all HttpClient
s regardless of their name or type.
@JamesNK Can ConfigureHttpClientDefaults(Action<IHttpClientBuilder> configure)
be updated to provide the actual name/type of the HttpClient
being resolved? As I understand, currently its configure
action is executed immediately when ConfigureHttpClientDefaults()
is invoked, therefore it doesn't have the actual Name
of HttpClient
. And to be able to provide the Name
it should be updated to execute the configure
action when the actual HttpClient
is being resolved/created, i.e. when there is information about its type/name. Can we update ConfigureHttpClientDefaults()
in a such way?
@sveinungf As a workaround, you can configure the standard resilience pipeline to be applied per HTTP request authority:
builder.Services.ConfigureHttpClientDefaults(x => x
.AddStandardResilienceHandler(section)
.SelectPipelineByAuthority());
If you do so, you'll have distinct resilience pipelines per each HTTP request authority. It'll solve the shared circuit breaker issue. You can even specify a custom criteria how to select resilience pipelines using the method SelectPipelineBy
.
@JamesNK Can ConfigureHttpClientDefaults(Action
configure) be updated to provide the actual name/type of the HttpClient being resolved?
IHttpClientBuilder
has a name property on it. Have you tried using that value?
This feature lives in dotnet/runtime - https://github.com/dotnet/runtime/blob/55c904024601c133f8ad081bc704c3c1fc5c7c9b/src/libraries/Microsoft.Extensions.Http/src/DependencyInjection/HttpClientFactoryServiceCollectionExtensions.cs#L80. I don't work on the HTTP client team and I don't have time to investigate more. I suggest creating an issue there to talk about it with the HTTP client folks.
IHttpClientBuilder
has a name property on it. Have you tried using that value?
Yes, I have. IHttpClientBuilder.Name
is always null.
I don't work on the HTTP client team and I don't have time to investigate more. I suggest creating an issue there to talk about it with the HTTP client folks.
Got it. I noticed that it was you who proposed the API #87914 and then implemented it #87953, therefore I mentioned you in the discussion and asked for help. Sure, I'll create an issue and start discussion with HTTP client folks. Thank you.
Here is an issue in dotnet/runtime to discuss ConfigureHttpClientDefaults
.
I'd like to reborn the discussion and come to an agreement.
Registering the Standard Resilience handler as default HttpClient
configuration leads to having a single Standard Resilience pipeline shared by all HttpClient
s. That could cause issues because all HttpClient
s will share the same rate limiter and circuit breaker. Specifically, consider the following code sample:
// 1. We register the Standard Resilience handler as default configuration, and register a couple of HttpClients.
var services = new ServiceCollection();
services.ConfigureHttpClientDefaults((IHttpClientBuilder x) => x.AddStandardResilienceHandler());
services.AddHttpClient("SucceedingClient", x => x.BaseAddress = new Uri("https://succeeding.api/"));
services.AddHttpClient("FailingClient", x => x.BaseAddress = new Uri("https://failing.api"));
// 2. We create instances of the registered HttpClients for further usage.
var factory = services.BuildServiceProvider().GetRequiredService<IHttpClientFactory>();
HttpClient succeedingClient = factory.CreateClient("SucceedingClient");
HttpClient failingClient = factory.CreateClient("FailingClient");
If failingClient
causes the circuit breaker to open that will also affect succeedingClient
, because both HttpClient
s have the same circuit breaker. To fix this we could register the Standard Resilience handler for each HttpClient
instead of registering it as default configuration:
var services = new ServiceCollection();
services.AddHttpClient("SucceedingClient", x => x.BaseAddress = new Uri("https://succeeding.api/"))
.AddStandardResilienceHandler();
services.AddHttpClient("FailingClient", x => x.BaseAddress = new Uri("https://failing.api"))
.AddStandardResilienceHandler();
Now each of those HttpClient
s will have its own standard resilience pipeline, hence its own circuit breaker.
The problem occurs because of how methods ConfigureHttpClientDefaults
and AddStandardResilienceHandler
work.
AddStandardResilienceHandler
accepts IHttpClientBuilder
and registers the Standard Resilience handler for the given named HttpClient
. For each named HttpClient
there is a single standard resilience pipeline. That single standard resilience pipeline helps us to enforce rate limiter and circuit breaker policies for the given named HttpClient
.
ConfigureHttpClientDefaults
accepts a delegate with an argument of type IHttpClientBuilder
, which can be used to provide default configuration for HttpClient
. The name of the IHttpClientBuilder
is always null
. That is the semantic of the method that is not going to be changed https://github.com/dotnet/runtime/issues/101719#issuecomment-2154781422.
As a result, when we register Standard Resilience handler as default HttpClient
configuration we register only one resilience pipeline with Name=null
:
// This results with a single standard resilience pipeline with Name=null. The pipeline is shared by all `HttpClient`s.
services.ConfigureHttpClientDefaults((IHttpClientBuilder x) => x.AddStandardResilienceHandler());
In opposite, the following will register a separate resilience pipeline for each named HttpClient:
// This registers a resilience pipeline with Name=SuccedingClient, and the pipeline is used only for this named HttpClient.
HttpClient succeedingClient = factory.CreateClient("SucceedingClient");
// This registers a resilience pipeline with Name=FailingClient, and the pipeline is used only for this named HttpClient.
HttpClient failingClient = factory.CreateClient("FailingClient");
As I understand, we don't want to change current behavior of the AddStandardResilienceHandler
method, i.e. we want it to register a single pipeline per given named HttpClient, because it allows us to enforce rate limiter and circuit breaker policies. Right? We support having more than one resilience pipeline per given named HttpClient
though. It could be achieved by using one of the SelectPipelineBy
methods.
In my opinion, the issue with shared circuit breaker when registering the Standard Resilience handler as default HttpClient
configuration is a gap in our documentation. We need to clearly describe this situation and give users recommendations how to fix it. As a potential fix we could recommend them to use SelectPipelineByAuthority method:
services.ConfigureHttpClientDefaults(x => x
x.AddStandardResilienceHandler().SelectPipelineByAuthority());
This would result in having one resilience pipeline per each HTTP request authority which is much better than having a single pipeline for all HttpClient
s.
What do you think? @sveinungf @martintmk @geeknoid @joperezr
@joperezr @geeknoid @martintmk Aspire sets up Standard Resilience handler as one of the service defaults, which could lead to the issue described here. If we decide that having a resilience pipeline per HTTP request authority is better/more reasonable, i.e. registering the pipeline like this:
services.ConfigureHttpClientDefaults(x => x
x.AddStandardResilienceHandler().SelectPipelineByAuthority());
then we could potentially contribute to Aspire.
If I understood the problem and possible solution options, and if the following works as expected:
var services = new ServiceCollection();
services.AddHttpClient("SucceedingClient", x => x.BaseAddress = new Uri("https://succeeding.api/"))
.AddStandardResilienceHandler();
services.AddHttpClient("FailingClient", x => x.BaseAddress = new Uri("https://failing.api/"))
.AddStandardResilienceHandler();
...then is it not a matter of documenting the behaviours and scenarios in which those occur?
Also, what would be the behaviour if the following code was written?
var builder = WebApplication.CreateBuilder(args);
var section = builder.Configuration.GetSection("RetryOptions");
builder.Services.ConfigureHttpClientDefaults(x => x.AddStandardResilienceHandler(section));
var succeedingClientBuilder = builder.Services.AddHttpClient<SucceedingClient>(x => x.BaseAddress = new Uri("https://ipv4.icanhazip.com/"))4
.AddStandardResilienceHandler();
var failingClientBuilder = builder.Services.AddHttpClient<FailingClient>(x => x.BaseAddress = new Uri("http://the-internet.herokuapp.com/status_codes/500"))
.AddStandardResilienceHandler();
@iliar-turdushev , Great investigation!
services.ConfigureHttpClientDefaults(x => x x.AddStandardResilienceHandler().SelectPipelineByAuthority());
This helps slightly in a way that at least multiple clients with different authorities won't share the same circuit breaker. (which is quite an improvement)
However, consider this example:
var services = new ServiceCollection();
services.AddLogging(builder => builder.AddConsole().SetMinimumLevel(LogLevel.Debug));
services.ConfigureHttpClientDefaults(builder =>
{
builder.AddStandardResilienceHandler().SelectPipelineByAuthority();
});
var factory = services.BuildServiceProvider().GetRequiredService<IHttpClientFactory>();
Console.WriteLine("client-1");
var client1 = factory.CreateClient("client-1");
await client1.GetStringAsync("https://jsonplaceholder.typicode.com/todos/1");
Console.WriteLine("client-2");
var client2 = factory.CreateClient("client-2");
await client2.GetStringAsync("https://jsonplaceholder.typicode.com/todos/1");
That output's the following:
info: System.Net.Http.HttpClient.client-1.ClientHandler[100]
Sending HTTP request GET https://jsonplaceholder.typicode.com/todos/1
info: System.Net.Http.HttpClient.client-1.ClientHandler[101]
Received HTTP response headers after 158.4074ms - 200
info: Polly[3]
Execution attempt. Source: '-standard/https://jsonplaceholder.typicode.com/Standard-Retry', Operation Key: '', Result: '200', Handled: 'False', Attempt: '0', Execution Time: 168.3799ms
dbug: Polly[2]
Resilience pipeline executed. Source: '-standard/https://jsonplaceholder.typicode.com', Operation Key: '', Result: '200', Execution Time: 180.2802ms
info: System.Net.Http.HttpClient.client-1.LogicalHandler[101]
End processing HTTP request after 227.9248ms - 200
client-2
info: System.Net.Http.HttpClient.client-2.LogicalHandler[100]
Start processing HTTP request GET https://jsonplaceholder.typicode.com/todos/1
dbug: Polly[1]
Resilience pipeline executing. Source: '-standard/https://jsonplaceholder.typicode.com', Operation Key: '(null)'
info: System.Net.Http.HttpClient.client-2.ClientHandler[100]
Sending HTTP request GET https://jsonplaceholder.typicode.com/todos/1
info: System.Net.Http.HttpClient.client-2.ClientHandler[101]
Received HTTP response headers after 102.2284ms - 200
info: Polly[3]
Execution attempt. Source: '-standard/https://jsonplaceholder.typicode.com/Standard-Retry', Operation Key: '', Result: '200', Handled: 'False', Attempt: '0', Execution Time: 102.366ms
dbug: Polly[2]
Resilience pipeline executed. Source: '-standard/https://jsonplaceholder.typicode.com', Operation Key: '', Result: '200', Execution Time: 102.4594ms
info: System.Net.Http.HttpClient.client-2.LogicalHandler[101]
End processing HTTP request after 102.6431ms - 200
Notice the -standard/https://jsonplaceholder.typicode.com/Standard-Retry
which completely omits the client-1
which should be there as identifier of the pipeline. This is because the ConfigureHttpClientDefaults
does not use any client name.
Also @RussKie, your example bellow breaks the pipeline as it adds the additional standard resilience handler.
I don't believe there is a use-case where 2 standard pipelines are valid on a single HttpClient
.
var builder = WebApplication.CreateBuilder(args);
var section = builder.Configuration.GetSection("RetryOptions");
builder.Services.ConfigureHttpClientDefaults(x => x.AddStandardResilienceHandler(section));
var succeedingClientBuilder = builder.Services.AddHttpClient<SucceedingClient>(x => x.BaseAddress = new Uri("https://ipv4.icanhazip.com/"))4
.AddStandardResilienceHandler();
var failingClientBuilder = builder.Services.AddHttpClient<FailingClient>(x => x.BaseAddress = new Uri("http://the-internet.herokuapp.com/status_codes/500"))
.AddStandardResilienceHandler();
IMHO, much better behavior would be:
ConfigureHttpClientDefaults + AddStandardResilienceHandler
properly preserves the http client name in the telemetry. As additional benefit, there is no need to call SelectPipelineByAuthority
. However, I don't think this is doable given the limitations of ConfigureHttpClientDefaults
.ConfigureHttpClientDefaults + AddStandardResilienceHandler
+ explicit AddStandardResilienceHandler
. This call won't actually add a new handler. Instead, it preserves the order of the original handler and just allows changing the configuration for it. Essentially, duplicates won't be allowed for standard pipeline. @RussKie
is it not a matter of documenting the behaviours and scenarios in which those occur?
That's what I think too. In my opinion, we need to document that registering the Standard Resilience handler as default HttpClient
configuration could lead to "shared circuit breaker" issue, and the issue could be solve either by configuring the standard pipeline to be applied per each HTTP request host or per some custom condition based on HTTP request or by registering the pipeline for each named HttpClient
.
Also, what would be the behaviour if the following code was written?
At the moment it would lead to registering Standard Resilience handler more than once, which is not correct, because it is not reasonable to have more than one standard resilience handlers in a single HttpClient
. Perfectly, the code you provided would lead to replacing Standard Resilience handler registered as default configuration with Standard Resilience handlers registered for those particular HttpClient
s. But that's not how it works now. And there is a corresponding bug reported https://github.com/dotnet/extensions/issues/4814. If we fix that bug then the code you provided could be another way to fix the problem described in the current issue.
I wanted to keep this issue as a documentation gap, and discuss https://github.com/dotnet/extensions/issues/4814 separately, but we can discuss it here, since it can partially contribute to the current issue.
We could also close #4818 as dup of this one, and continue here :)
To sum up, we have two issues:
We could also close https://github.com/dotnet/extensions/pull/4818 as dup of this one, and continue here :)
To sum up, we have two issues:
- A documentation gap which will address the immediate issue (more or less a tactical fix).
- A possible enhancement that will allow overriding the HTTP client resilience settings.
@RussKie Correct, at least that's how I see the problem. Let's for now keep both issues, because they describe two different use-cases, although the second one could help with solving the current issue.
Regarding the enhancement that will allow to override the Standard Resilience Handler.
As @martintmk pointed out we can implement an improvement when a second (and any following) attempt to register the Standard Resilience Handler doesn't add a new handler, but changes the configuration of the first registered handler. If we implement that then the following code sample:
var services = new ServiceCollection();
services.ConfigureHttpClientDefaults((IHttpClientBuilder x) => x.AddStandardResilienceHandler());
services.AddHttpClient("FailingClient", x => x.BaseAddress = new Uri("https://failing.api"));
services.AddHttpClient("SucceedingClient", x => x.BaseAddress = new Uri("https://succeeding.api/"))
AddStandardResilienceHandler();
will result in having two HttpClient
s:
And this solves the issue with the "shared circuit breaker". The downside is that a customer has to explicitly register a named client and register the standard resilience for it.
But there still will be issues we'll need to address. They are listed below.
As @martintmk said, having two standard resilience handlers in HttpClient would break it. Similarly, having two standard hedging handlers or combination of hedging and resilience handlers will break HttpClient. With the approach/solution proposed above we will need not only overwrite the configuration of the handler but also its type, e.g. if someone registers standard resilience handler and then standard hedging handler, then he/she will get HttpClient with the only standard hedging handler, i.e. the last registered handler will "win".
With the proposed approach it is not achievable. We can consider introducing a method RemoveAllStandardResilienceHandlers
similar to RemoveAllLoggers. It will help to remove all standard resilience handlers and put your resilience handler in the required place in the request pipeline. But since there is no such need at the moment we can get back to this when the need occurs.
If some 3rd party library registers its own resilience handler then with the the last registered resilience handler "wins" approach we'll get HttpClient with the resilience handler provided by 3rd party's library which could be undesirable. Do we need to introduce a special method with TryAdd*
semantics that will not overwrite the already registered handler? We'll need to address this too, but I think we can skip for now this since there is no such a request from customers.
To conclude, I suggest to implement the approach suggested above: the last registered resilience or hedging handler overwrites settings of the first registered handler. We don't allow more than one standard resilience or hedging handler in HttpClient. We'll postpone solving potential issues 2 and 3 mentioned above until customers come to us with corresponding asks.
So, what do you think?
Would it not be more compatible to just have .ReplaceXXX or a .ClearResilienceHandlers() then .AddXXX?
This would allow us to imperatively do it with full knowledge of what we're doing and why without breaking anything already there.
Would it not be more compatible to just have .ReplaceXXX or a .ClearResilienceHandlers() then .AddXXX?
This would allow us to imperatively do it with full knowledge of what we're doing and why without breaking anything already there.
How would the API and their use look like?
Something like this:
builder.Services.AddHttpClient<SucceedingClient>(x => x.BaseAddress = new Uri("https://ipv4.icanhazip.com/"))
.ClearResilienceHandlers();
.AddResilienceHandler(x => {stuff here});
and
builder.Services.AddHttpClient<SucceedingClient>(x => x.BaseAddress = new Uri("https://ipv4.icanhazip.com/"))
.ReplaceStandardResilienceHandler(x => {stuff here});
@JohnGalt1717 Thank you for the feedback. A comment above https://github.com/dotnet/extensions/issues/5021#issuecomment-2359380710 suggests to implement AddStandardResilienceHandler
method in a way that it will have behavior of a method AddOrReplaceStandardResilienceHandler
.
Regarding RemoveAll*
method. I think we might want to introduce such a method in order to enable users to change order of the resilience handler. In the scope of this task I propose only changing behavior of the AddStandardResilienceHandler
method.
Description
When registering the standard resilience handler using
builder.Services.ConfigureHttpClientDefaults(x => x.AddStandardResilienceHandler)
, allHttpClient
s seem to be using a shared circuit breaker. When one client is causing the circuit to open, the circuit opens for all clients.Reproduction Steps
Here is my project file:
Here is my appsettings.json (modified to be able to more easily reproduce the issue):
And here is the Program.cs:
Run this sample code and continuously hit the
/test
endpoint until the circuit breaker kicks in.Expected behavior
I would expect the circuit would only open for the client that is facing issues. From the reproduction steps, the circuit should not open for
SucceedingClient
when it opens forFailingClient
.Actual behavior
The circuit breaker opens for both the
SucceedingClient
and theFailingClient
, even though onlyFailingClient
is receiving status code 500.Regression?
No response
Known Workarounds
Use
AddStandardResilienceHandler
on each HttpClient instead of usingConfigureHttpClientDefaults
.Configuration
dotnet --info
.NET SDK: Version: 8.0.200 Commit: 438cab6a9d Workload version: 8.0.200-manifests.e575128c Runtime Environment: OS Name: Windows OS Version: 10.0.22631 OS Platform: Windows RID: win-x64 Base Path: C:\Program Files\dotnet\sdk\8.0.200\ .NET workloads installed: There are no installed workloads to display. Host: Version: 8.0.2 Architecture: x64 Commit: 1381d5ebd2 .NET SDKs installed: 8.0.200 [C:\Program Files\dotnet\sdk] .NET runtimes installed: Microsoft.AspNetCore.App 3.1.32 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 5.0.17 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 6.0.27 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 7.0.16 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 8.0.2 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.NETCore.App 3.1.32 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 5.0.17 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 6.0.27 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 7.0.16 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 8.0.2 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.WindowsDesktop.App 3.1.32 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] Microsoft.WindowsDesktop.App 5.0.17 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] Microsoft.WindowsDesktop.App 6.0.27 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] Microsoft.WindowsDesktop.App 7.0.16 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] Microsoft.WindowsDesktop.App 8.0.2 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] Other architectures found: x86 [C:\Program Files (x86)\dotnet] registered at [HKLM\SOFTWARE\dotnet\Setup\InstalledVersions\x86\InstallLocation] Environment variables: Not set global.json file: Not foundOther information
No response