dodopizza / httpclient-resilience-policies Goto Github PK

In the latest major version 2.0.0 we have strict binging to the net5.0 because of dependencies.

Problem description
Currently NuGet package documentation section is provided by hand via NuGet.org management panel. You have to provide new documentation text for each release. It must be automated somehow.

Currently, we have strict bounds with Polly library. For example, in the settings classes we have code like this Action<DelegateResult<HttpResponseMessage>, TimeSpan> DoNothingOnBreak, where DelegateResult is a Polly class which exposed to the end-users. We want to get rid of it in the public classes to separate library API and details of it implementation.
It gives us huge benefits:

• We can replace Polly with something else without breaking changes for users.
• It relaxes the requirement to have the same version of Polly in the client code and our library, which is quite important to integration.

The downside of this solution is bunch of boilerplate code for wrapping some Polly classes.

.NET 5 finally released. We should add support for this target.

AddJsonClient extension method provided by the library has clientName argument to create named clients but actually never use it.

Actually it is not a bug, it was never implemented. We should fix it.

Previous AddDefaultPolicies method was the most frequently used extension method. We should return it, but we want to rename it to AddResiliencePolicies because word "default" has unclear meaning.

Currently library builds for .netstandard2.0 framework. We have to support .netcoreapp3.1.

Add PDB to NuGet package to support code navigation.
See AllowedOutputExtensionsInPackageBuildOutputFolder csproj property.

Because of namespace clash users should provide fully qualified name to use our library.

Remove separate logic for host specific and generic CB. We should leave only host specific CB.

Add support for full framework. Check library on Windows with full framework.

Problem description
Add wiki documentation to project and move specified files to wiki:

• Contributing.md
• README.md
• CI_AND_RELEASE.md

It would be nice to measure code coverage.

Description
Add the possibility to use bulkhead policy.

This issue is required to think about the strategy of using this policy. Maybe it's not necessary for this library and needs only to advanced users who can use Polly itself.

Additional information
https://github.com/App-vNext/Polly/wiki/Bulkhead

Update README to more precisely describe the problem which is solved by this library. The essential part of this repo is not only provide wraps of Polly policies, but also create proper order in which policies should applied to provide correct request/response flow. Otherwise you may get unexpected behavior of you http client.

Hello!

I hope you are doing well!

We are a security research team. Our tool automatically detected a vulnerability in this repository. We want to disclose it responsibly. GitHub has a feature called Private vulnerability reporting, which enables security research to privately disclose a vulnerability. Unfortunately, it is not enabled for this repository.

Can you enable it, so that we can report it?

Thanks in advance!

PS: you can read about how to enable private vulnerability reporting here: https://docs.github.com/en/code-security/security-advisories/repository-security-advisories/configuring-private-vulnerability-reporting-for-a-repository

.NET Core 2.1 is out of support. We should remove its support too. Additionally, it allows us to get rid of conditionals in csproj.

Problem description

The first problem is the name of HttpClientSettings does not reflect the core idea of the library. It would be better PollyDefaultSettings or something like this.

The second problem is according to framework design guidelines using constructors is not very usable in this type of object. It would be better to make public setters for all settings.

Problem description
The choice of the correct timeout is always a complicated task.

The idea is rather than using a configured constant timeout continually measure response times and automatically adjust timeouts according to the observation of response time distribution. This can be done with a Phi Accrual failure detector algorithm.

Exception:
Polly.Timeout.TimeoutRejectedException : The delegate executed asynchronously through TimeoutPolicy did not complete within the timeout.
----> System.OperationCanceledException : The operation was canceled.

Way to reproduce:

		[Test]
		public async Task OverallTimeoutError()
		{
			var settings = new ResiliencePoliciesSettings
			{
				OverallTimeout = TimeSpan.FromMilliseconds(10)
			};
			var wrapper = Create.HttpClientWrapperWrapperBuilder
				.WithStatusCode(HttpStatusCode.ServiceUnavailable)
				.WithResiliencePolicySettings(settings)
				.Please();

			await wrapper.Client.GetAsync("http://localhost");
		}

Description
A Fallback policy defines how the operation should react if the underlying operation fails. To simplify use by clients need to add the possibility to use fallback policy.

Polly recommends to add fallback policy the first one:

Additional information
https://github.com/App-vNext/Polly/wiki/Fallback

Description
Library uses only timeout-per-try and all responsibility rests with the client.
But It'll be better to add an overall-timeout-policy to concentrate full responsibility in the library.

Suggested ordering of overall-timeout-policy at the below image:

All additional information see at:
https://github.com/App-vNext/Polly/wiki/Timeout
https://github.com/App-vNext/Polly/wiki/Polly-and-HttpClientFactory#use-case-applying-timeouts

Test Should_break_after_4_concurrent_calls looks flaky. It usually represents on my local machine. 2 of 3 tests are failed for netcoreapp2.1:

Dodo.HttpClientResiliencePolicies.Tests.CircuitBreakerTests.Should_break_after_4_concurrent_calls

  Expected: 2
  But was:  3

According to the https://dotnet.microsoft.com/download/dotnet-core LTS versions are: 2.1 and 3.1. We have to add support of 2.1 and run build and tests on this version too.

Problem description
Release pipeline mark some fields as deprecated. See logs below:

##[warning]/opt/hostedtoolcache/dncs/3.1.100/x64/sdk/3.1.100/Sdks/NuGet.Build.Tasks.Pack/build/NuGet.Build.Tasks.Pack.targets(198,5): warning NU5125: The 'licenseUrl' element will be deprecated. Consider using the 'license' element instead. [/home/runner/work/httpclient-resilience-policies/httpclient-resilience-policies/src/Dodo.HttpClient.ResiliencePolicies/Dodo.HttpClient.ResiliencePolicies.csproj]
##[warning]/opt/hostedtoolcache/dncs/3.1.100/x64/sdk/3.1.100/Sdks/NuGet.Build.Tasks.Pack/build/NuGet.Build.Tasks.Pack.targets(198,5): warning NU5048: The 'PackageIconUrl'/'iconUrl' element is deprecated. Consider using the 'PackageIcon'/'icon' element instead. Learn more at https://aka.ms/deprecateIconUrl [/home/runner/work/httpclient-resilience-policies/httpclient-resilience-policies/src/Dodo.HttpClient.ResiliencePolicies/Dodo.HttpClient.ResiliencePolicies.csproj]

If server response with transient error code we also should check Retry-After HTTP header. If it exists it allows us to adjust retry policy and avoid server overwhelming with requests.

See https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Retry-After