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

Synchronous HttpClient Methods Do Not Use Retry Resiliency Strategy #5236

Closed
phil000 opened this issue Jun 19, 2024 · 6 comments · Fixed by #5333
Closed

Synchronous HttpClient Methods Do Not Use Retry Resiliency Strategy #5236

phil000 opened this issue Jun 19, 2024 · 6 comments · Fixed by #5333
Assignees
Labels
area-resilience bug This issue describes a behavior which is not expected - a bug.

Comments

@phil000
Copy link

phil000 commented Jun 19, 2024

Description

Using Microsoft.Extensions.Http.Resilience 8.6.0

When adding HttpClient to the services container this works, and produces a client that can be retrieved by name, and with the associated resilience pipeline:

services.AddHttpClient("client-name")
.AddResilienceHandler("client-name-pipeline", builder =>
{
builder.AddRetry(new HttpRetryStrategyOptions
{
	MaxRetryAttempts = 8,
	UseJitter = true,
	ShouldRetryAfterHeader = true,
	Delay = TimeSpan.FromMilliseconds(100),
	MaxDelay = TimeSpan.FromSeconds(10),
	BackoffType = DelayBackoffType.Exponential,
	OnRetry = (msg) =>
	{
		Debug.WriteLine("");
		Debug.WriteLine("Now: " + DateTime.Now.ToString("O"));
		Debug.WriteLine("RetryDelay: " + msg.RetryDelay);
		Debug.WriteLine("");
		return ValueTask.CompletedTask;
	}
});
});

However, when using a typed HttpClient the implementation of the interface gets an HttpClient without any resilience pipeline.
e.g. This provides an HttpClient in the implementation class that performs 0 retries:

services.AddHttpClient<IConfigurationServiceClient, ConfigurationServiceClient>((sp, client) =>
		{
			configure(sp, client);
		})
		.AddResilienceHandler("some name", builder =>
		{
			builder.AddRetry(new HttpRetryStrategyOptions
			{
				MaxRetryAttempts = 6,
				UseJitter = true,
				ShouldRetryAfterHeader = true,
				Delay = TimeSpan.FromMilliseconds(100),
				MaxDelay = TimeSpan.FromSeconds(10),
				BackoffType = DelayBackoffType.Exponential,
				OnRetry = (msg) =>
				{
					Debug.WriteLine("");
					Debug.WriteLine("Now: " + DateTime.Now.ToString("O"));
					Debug.WriteLine("RetryDelay: " + msg.RetryDelay);
					Debug.WriteLine("");
					return ValueTask.CompletedTask;
				}
			});
		});

Reproduction Steps

Use code similar to above to use a typed HttpClient.
Use the HttpClient and observe that no retries are performed.

Expected behavior

Typed HttpClients would be configured with resilience pipeline.

Actual behavior

Typed HttpClients do not appear to be configured with resilience pipeline.

Regression?

No response

Known Workarounds

No response

Configuration

.net 8.0

Other information

Using Microsoft.Extensions.Http.Resilience 8.6.0

@phil000 phil000 added bug This issue describes a behavior which is not expected - a bug. untriaged labels Jun 19, 2024
@phil000
Copy link
Author

phil000 commented Jun 19, 2024

OK, so I think I have a repo for this.
It doesn't relate to the client being typed, but seem to relate to the fact our ConfigurationServiceClient uses synchronous methods on HttpClient.

See repo below.

With this registration

services.AddHttpClient<IConfigurationClientRepo, ConfigurationClientRepo>(client =>
{
	client.Timeout = TimeSpan.FromSeconds(59);
})
	.AddResilienceHandler("IConfigurationClient", builder =>
	{
		builder.AddRetry(new HttpRetryStrategyOptions
		{
			MaxRetryAttempts = 8,
			UseJitter = true,
			ShouldRetryAfterHeader = true,
			Delay = TimeSpan.FromMilliseconds(100),
			MaxDelay = TimeSpan.FromSeconds(10),
			BackoffType = DelayBackoffType.Exponential,
			OnRetry = (msg) =>
			{
				Debug.WriteLine("Now: " + DateTime.Now.ToString("O"));
				Debug.WriteLine("RetryDelay: " + msg.RetryDelay);
				return ValueTask.CompletedTask;
			}
		});
	});

This code will NOT invoke the retires when GetRequest() is called, but will invoke the retries when GetRequestAsync() is called:

public class ConfigurationClientRepo(HttpClient httpClient) : IConfigurationClientRepo
{
	public HttpResponseMessage GetRequest()
	{
		//This 408 failure does *NOT* invoke the resilience retry.
		using HttpRequestMessage httpRequest = new(HttpMethod.Get, "https://httpstat.us/408");
		using HttpResponseMessage httpResponse = httpClient.Send(httpRequest);
		return httpResponse;
	}

	public async Task<HttpResponseMessage> GetRequestAsync()
	{
		//This 408 failure DOES invoke the resilience retry.
		using HttpRequestMessage httpRequest = new(HttpMethod.Get, "https://httpstat.us/408");
		using HttpResponseMessage httpResponse = await httpClient.SendAsync(httpRequest);
		return httpResponse;
	}
}

e.g.

//No retires performed:
_configurationClient.GetRequest();

//Retries performed:
await _configurationClient.GetRequestAsync();

@joperezr
Copy link
Member

Thanks for the report @phil000! @iliar-turdushev could you please take a look here?

@phil000
Copy link
Author

phil000 commented Jun 19, 2024

This small console app repo's the issue using both a typed client and named client.

HttpClientPollyRepo.zip

@phil000 phil000 changed the title AddResilienceHandler Does Not Work with Typed HttpClient Synchronous HttpClient Methods Do Not Use Retry Resiliency Strategy Jun 19, 2024
@YumeiTenerife
Copy link

I am having the same issue. Is there any fix for this?

@iliar-turdushev
Copy link
Contributor

@phil000 Thank you for reporting this. We'll add support of resilience strategies for synchronous HttpClient requests.
cc @YumeiTenerife

iliar-turdushev added a commit that referenced this issue Aug 5, 2024
Adds StandardPipeline and Retry benchmarks for synchronous HttpClient requests
iliar-turdushev added a commit that referenced this issue Aug 8, 2024
…HttpClient requests (#5333)

* Add support of resilience for synchronous HttpClient requests

* Added one more test

* Mention a variable type explicitly if it is not obvious from the assignment

* Fixes #5236

Adds StandardPipeline and Retry benchmarks for synchronous HttpClient requests
@RussKie
Copy link
Member

RussKie commented Aug 8, 2024

The change is available on https://dev.azure.com/dnceng/public/_artifacts/feed/dotnet8 feed. It will be formally released later this month.

@github-actions github-actions bot locked and limited conversation to collaborators Sep 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-resilience bug This issue describes a behavior which is not expected - a bug.
Projects
None yet
5 participants