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

Currently impossible to override the AddStandardResillienceHandler() call for a specific client which means that you either use it or can't. #4814

Open
JohnGalt1717 opened this issue Dec 16, 2023 · 8 comments
Assignees
Labels
area-resilience bug This issue describes a behavior which is not expected - a bug.

Comments

@JohnGalt1717
Copy link

Description

I debated if this was a bug or a feature request, but since Aspire sets this by the default (which it should) and then every single HttpClient you create then is forced into those defaults with apparently no way to override them, this becomes a bug because it's both not discoverable with what's happening and the documentation says nothing about this, while simultaneously adding a new resilience handler to a specific client implementation doesn't override it.

Hence, at best, it's a bug in documentation, but more, this is a major blocker, because it's impossible to set anything other than defaults as soon as you opt into this, which I can't imagine was the design intent.

Reproduction Steps

If this is in your project.

		builder.Services.ConfigureHttpClientDefaults(http => {
			//Turn on resilience by default
			http.AddStandardResilienceHandler();

			// Turn on service discovery by default
			http.UseServiceDiscovery();
		});

The timeout on any client that you try and override will be ignored.

This doesn't work.

services.AddHttpClient<IKaiLLamaClient, LLamaKaiLLM>()
.AddStandardResilienceHandler(options => {
	options.AttemptTimeout = new HttpTimeoutStrategyOptions {
		Timeout = TimeSpan.FromMinutes(5)
	};
	options.TotalRequestTimeout = new HttpTimeoutStrategyOptions {
		Timeout = TimeSpan.FromMinutes(15)
	};
	options.CircuitBreaker.SamplingDuration = TimeSpan.FromMinutes(10);
});

This doesn't work: (where timeoutPolicy is a polly timeout policy that sets the time out)

builder.Services.AddHttpClient<IOrca2LLamaClient, LLamaOrca2LLM>(client => {
	client.Timeout = TimeSpan.FromMinutes(5);
})
.AddPolicyHandler(timeoutPolicy);

I've tried every permutation and combination I can think of, and in all cases, it is ignored.

Expected behavior

You should be able to override the defaults on a specific client and how to do so should be documented clearly.

This should just work:

			services.AddHttpClient<IKaiLLamaClient, LLamaKaiLLM>()
				.AddStandardResilienceHandler(options => {
					options.AttemptTimeout = new HttpTimeoutStrategyOptions {
						Timeout = TimeSpan.FromMinutes(5)
					};
					options.TotalRequestTimeout = new HttpTimeoutStrategyOptions {
						Timeout = TimeSpan.FromMinutes(15)
					};
					options.CircuitBreaker.SamplingDuration = TimeSpan.FromMinutes(10);
				});

Or there should be an OverrideResilienceHandler that replaces it for this client or something.

Actual behavior

It appears to just add a new resiliance handler that is suplimental to the other one, so whatever one times out first, kills the client.

Regression?

No response

Known Workarounds

None other than removing all default resiliance handling for http.

Configuration

.NET SDK:
Version: 8.0.100
Commit: 57efcf1350
Workload version: 8.0.100-manifests.71b9f198

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.100\

.NET workloads installed:
Workload version: 8.0.100-manifests.71b9f198
[aspire]
Installation Source: SDK 8.0.100, VS 17.9.34321.82
Manifest Version: 8.0.0-preview.1.23557.2/8.0.100
Manifest Path: C:\Program Files\dotnet\sdk-manifests\8.0.100\microsoft.net.sdk.aspire\8.0.0-preview.1.23557.2\WorkloadManifest.json
Install Type: Msi

[maui-windows]
Installation Source: VS 17.8.34330.188
Manifest Version: 8.0.3/8.0.100
Manifest Path: C:\Program Files\dotnet\sdk-manifests\8.0.100\microsoft.net.sdk.maui\8.0.3\WorkloadManifest.json
Install Type: Msi

[maccatalyst]
Installation Source: VS 17.8.34330.188
Manifest Version: 17.0.8478/8.0.100
Manifest Path: C:\Program Files\dotnet\sdk-manifests\8.0.100\microsoft.net.sdk.maccatalyst\17.0.8478\WorkloadManifest.json
Install Type: Msi

[ios]
Installation Source: VS 17.8.34330.188
Manifest Version: 17.0.8478/8.0.100
Manifest Path: C:\Program Files\dotnet\sdk-manifests\8.0.100\microsoft.net.sdk.ios\17.0.8478\WorkloadManifest.json
Install Type: Msi

[android]
Installation Source: VS 17.8.34330.188
Manifest Version: 34.0.43/8.0.100
Manifest Path: C:\Program Files\dotnet\sdk-manifests\8.0.100\microsoft.net.sdk.android\34.0.43\WorkloadManifest.json
Install Type: Msi

Host:
Version: 8.0.0
Architecture: x64
Commit: 5535e31a71

.NET SDKs installed:
6.0.417 [C:\Program Files\dotnet\sdk]
7.0.404 [C:\Program Files\dotnet\sdk]
8.0.100 [C:\Program Files\dotnet\sdk]

.NET runtimes installed:
Microsoft.AspNetCore.App 6.0.25 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 7.0.14 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 8.0.0 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.NETCore.App 6.0.25 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 7.0.14 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 8.0.0 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.WindowsDesktop.App 6.0.25 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
Microsoft.WindowsDesktop.App 7.0.14 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
Microsoft.WindowsDesktop.App 8.0.0 [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:
E:\Repos\Atteria\Pltfrmd\services\global.json

Learn more:
https://aka.ms/dotnet/info

Download .NET:
https://aka.ms/dotnet/download

Other information

No response

@JohnGalt1717 JohnGalt1717 added bug This issue describes a behavior which is not expected - a bug. untriaged labels Dec 16, 2023
@meinsiedler
Copy link

Additionally to what @JohnGalt1717 suggests: It would also help us if we could override the resiliency settings for one single endpoint of a typed http client.

The use case for this is that we have a typed http client with some endpoints where we want to use the standard resiliency settings. But we have one single endpoint in that typed http client where we know that this endpoint takes longer than the configured AttemptTimeout (which is totally fine). We have no possibility - as I'm aware of - to override the default behavior for this single endpoint.

@silkfire
Copy link

For specific endpoints I'd probably create a separate client registration. You can still reuse a lot of logic by creating an extension method.

@meinsiedler
Copy link

@silkfire I forgot to mention that our typed http client is generated via NSwag and represents a whole API. The typed http client even comes as separate NuGet package, which we cannot influence.

It would be quite cumbersome to split up the client registrations just for having different resiliency settings and it also feels like a leaky abstraction to have two typed http clients just to differeniate between those two resiliency settings. The calling code, which then uses one of the two typed clients, would then need to be aware of which client to use for which endpoint.

I would rather prefer to have specific configuration possiblities per endpoint when registering the typed http client and re-use the same typed http client everywhere.

@silkfire
Copy link

@meinsiedler I just realized you can add a strategy conditionally by configuring the Retry.ShouldHandle predicate:

httpClientBuilder.AddStandardResilienceHandler(o =>
                                              {
                                                  o.Retry.ShouldHandle = args => args.Outcome switch
                                                  {
                                                     { Result.RequestMessage: not null } outcome when outcome.Result.RequestMessage.RequestUri != null && outcome.Result.RequestMessage.RequestUri.AbsolutePath.EndsWith("/exclude-endpoint-from-retry") => PredicateResult.False(),
                                                     var outcome when HttpClientResiliencePredicates.IsTransient(outcome) => PredicateResult.True(),
                                                     { Exception: InvalidOperationException } => PredicateResult.True(),
                                                     _ => PredicateResult.False()
                                                  };
                                              });

@joperezr
Copy link
Member

joperezr commented Apr 5, 2024

@martintmk could you take a look into this issue? Is this a case were we do have a way to configure this but it is not discoverable, or is there no good way of overriding the defaults? If the latter, could it be possible for us to add a feature to support this? As pointed out in the original post, this particularly a problem in Aspire as it adds these by default to both the ServiceDefaults, as well as the scenario tests project.

@iliar-turdushev
Copy link
Contributor

The issue is caused by the following behavior #101719 of ConfigureHttpClientDefaults method. Because of the behavior the method calls ConfigureHttpClientDefaults(x => x.AddStandardResiliencePipeline()) and AddHttpClient<T>().AddStandardResiliencePipline() register two distinct resilience pipelines in the HttpClient request pipeline. We need to figure out whether the issue is with ConfigureHttpClientDefaults. If not, and ConfigureHttpClientDefaults is not going to be updated/fixed to support our scenario, then we'll need to find out what to do with AddStandardResiliencePipeline.

@CarnaViire
Copy link
Member

we'll need to find out what to do with AddStandardResiliencePipeline.

@iliar-turdushev I've put some ideas in a comment here dotnet/runtime#101719 (comment)

@iliar-turdushev
Copy link
Contributor

@JohnGalt1717 The issue you reported is being discussed #5021. Could you, please, take a look at the following comment #5021 (comment) and share your feedback? Thank you.

@DamianEdwards DamianEdwards changed the title Currently impossible to override the AddStandardREsillienceHandler() call for a specific client which means that you either use it or can't. Currently impossible to override the AddStandardResillienceHandler() call for a specific client which means that you either use it or can't. Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-resilience bug This issue describes a behavior which is not expected - a bug.
Projects
None yet
Development

No branches or pull requests

8 participants