Anti-patterns in V7 - Misc

[peter-csala]

As it was discussed in this PR I try to collect those anti-patterns what I have seen in past 3+ years on StackOverflow.

In this issue I will focus on miscellaneous topics.

1 - Overusing builder methods

Polly uses builder methods everywhere. But it also allows you combine multiple things into a single one.

❌ DON'T
Use more than one Or/OrResult

var policy = Policy
    .Handle<HttpRequestException>()
    .Or<BrokenCircuitException>()
    .Or<TimeoutRejectedException>()
    .Or<SocketException>()
    .Or<RateLimitRejectedException>()
    .WaitAndRetryAsync(...);

Reasoning:

• Even though the builder method signature is quite concise you repeat the same thing over and over (please trigger retry if the to-be-decorated code throws XYZ exception).
• A better approach would be to tell please trigger retry if the to-be-decorated code throws one of the retriable exceptions.

✅ DO
Use collections and predicates

ImmutableArray<Type> networkExceptions = new[] {
    typeof(SocketException),
    typeof(HttpRequestException),
}.ToImmutableArray();

ImmutableArray<Type> policyExceptions = new[] {
    typeof(TimeoutRejectedException),
    typeof(BrokenCircuitException),
    typeof(RateLimitRejectedException),
}.ToImmutableArray();

var policy = Policy
    .Handle<Exception>(ex => networkExceptions.Union(policyExceptions).Contains(ex.GetType()))
    .WaitAndRetryAsync(...);

Reasoning:

• This approach embraces re-usability. For instance the networkExceptions can be reused across all the policies (timeout, circuit breaker, etc..).

2 - Overusing builder methods for Polly extensions

Polly extensions also like builder pattern. :)

❌ DON'T
Use more than one AddPolicyHandler / AddPolicyHandlerFromRegistry

...
.AddPolicyHandler(policy1)
.AddPolicyHandler(policy2)
.AddPolicyHandlerFromRegistry("policy3")
.AddPolicyHandler(policy4);

Reasoning:

• Each and every time when you call AddPolicyHandler a new PolicyHttpMessageHandler will be added to the delegating handlers.
• In the above example you have added 4 DelegatingHandlers for each and every HttpClient call.

✅ DO
Use PolicyWrap instead

var resilienceStrategy = Policy.WrapAsync<HttpResponseMessage>(policy1, policy2, policy3, policy4);
...
.AddPolicyHandler(resilienceStrategy);

Reasoning:

• By using Policy.WrapAsync you can reduce the number of Polly DelegatingHandlers to 1.
• When you need to access the HttpRequestMessage or the IServiceProvider still prefer Policy.WrapAsync over multiple AddPolicyHandler calls

...
.AddPolicyHandler((sp, req) => Policy.WrapAsync<HttpResponseMessage>(
        GetPolicy1(sp), policy2, GetPolicy3(req), policy4);

3 - Using ExecuteAndCapture to replicate Execute behaviour

ExecuteAndCapture allows you to decorate a method with a policy and it will not throw any exception in case of failure

❌ DON'T
Use the ExecuteAndCapture to either throw exception or return the result

var policyResult = await policy.ExecuteAndCaptureAsync(action);
    
if (policyResult.Outcome == OutcomeType.Failure)
{
   throw policyResult.FinalException;
}

return policyResult.Result;

Reasoning:

• This is the recreation of the ExecuteAsync behaviour (with an extra Frame to the stack trace)
• Prefer to use ExecuteAndCapture over Execute only if your implementation differs from the "default behaviour"

✅ DO
Use simply the Execute

return await policy.ExecuteAsync(action);

Reasoning:

• It is simpler and more concise
• It behaves in the way which is suitable for 99% of the times

4- Recreating the policies inside unit tests

Imagine that you have a class that receives an IAsyncPolicy<T> as a ctor parameter for better testability

❌ DON'T
Recreate the policy inside the unit test

//Arrange
var timeoutPolicy = Policy.TimeoutAsync<HttpResponseMessage>(1, TimeoutStrategy.Optimistic);

var mockDownstream= new Mock<IDownstream>();
mockDownstream.Setup(m => m.Call(It.IsAny<CancellationToken>()))
              .Returns(Task.Run(() => { Thread.Sleep(10_000); return new HttpResponseMessage(); }));

//Act
 var sut = new SUT(mockDownstream.Object, timeoutPolicy);
 await sut.Call();  

Reasoning:

• Depending on the your policy it might greatly increase the execution time (for example with several retries and second large sleep durations)
• This violates one of the F.I.R.S.T testing principles, namely the Fast one

✅ DO
Use NoOp or mock the relevant interface

//Testing your code like there is no policy applied
var policyMock = Policy.NoOpAsync<HttpResponseMessage>();

//Testing your code to make sure it handles success cases as expected
var policyMock = new Mock<IAsyncPolicy<HttpResponseMessage>>();
policyMock
    .Setup(p => p.ExecuteAsync(It.IsAny<Func<CancellationToken, Task<HttpResponseMessage>>>(), It.IsAny<CancellationToken>()))
    .ReturnsAsync(new HttpResponseMessage());

//Testing your code to make sure it handles failure cases as expected
var policyMock = new Mock<IAsyncPolicy<HttpResponseMessage>>();
policyMock
    .Setup(p => p.ExecuteAsync(It.IsAny<Func<CancellationToken, Task<HttpResponseMessage>>>(), It.IsAny<CancellationToken>()))
    .ThrowsAsync(new TimeoutRejectedException());

Reasoning:

• This approach does not add too much to the tests' execution time
• It also allows you to easier test different scenarios with simpler mocks

5- Accessing the IServiceCollection instead of IServiceProvider

Whenever you call AddPolicyHandler you can access the IServiceCollection via closure

❌ DON'T
Use IServiceCollection

services.AddHttpClient<IApiService, ApiService>()
        .AddPolicyHandler(GetPolicy(builder.services));
        
IAsyncPolicy<HttpResponseMessage> GetPolicy(IServiceCollection services)
{
    return HttpPolicyExtensions
                        .HandleTransientHttpError()
                        .WaitAndRetryAsync(...,
                            onRetryAsync: async (outcome, timespan, retryAttempt, context) => 
                            {
                                var serviceProvider = services.BuildServiceProvider();
                                var logger = serviceProvider.GetService<ILogger>();
                                ...
                            });
}        

Reasoning:

• This code builds a new ServiceProviderbefore each retry attempt unnecessarily

✅ DO
Use IServiceProvider

services.AddHttpClient<IApiService, ApiService>()
        .AddPolicyHandler((provider, request) => GetPolicy(provider));
        
IAsyncPolicy<HttpResponseMessage> GetPolicy(IServiceProvider svcProvider)
{
    return HttpPolicyExtensions
                        .HandleTransientHttpError()
                        .WaitAndRetryAsync(...,
                            onRetryAsync: async (outcome, timespan, retryAttempt, context) => 
                            {
                                var logger = svcProvider.GetService<ILogger>();
                                ...
                            });

Reasoning:

• This approach takes advantage of the already built ServiceProvider and accesses the same instance many times

6- Recreating the HttpClient registration in test

Imagine you want to make sure that your HttpClient has been decorated correctly

❌ DON'T
Recreate the entire DI registration inside a test

//Arrange
IServiceCollection services = new ServiceCollection();
services.AddHttpClient(...)
        .AddPolicyHandler(...)    

Reasoning:

• Unit tests usually should not deal with DI
• Recreating "startup" code to test a typed client (which relies on an HttpClient) is unnecessary most of the time
  • If you want to test your code like there is no attached policy then simple create an HttpClient with a mocked HttpMessageHandler

✅ DO
Prefer to test the Polly + HttpClient via integration tests using WireMock.Net

Here you can find an example how to do this: https://stackoverflow.com/questions/63032013/mocking-a-httpclient-handler-when-injected-by-ihttpclientfactory/63049086#63049086

Reasoning:

• If you want to test the integration of your components then prefer to use integration testing
• In order to avoid external service calls create an http server mock by WireMock.Net or HttpMock or HttpClient Interception or any other http server mock library

[martintmk]

All of these are real problems and I think we can extract these into a real docs page.

How should we organize these?

• Under each resilience strategy/policy under the Anti Patterns section o
• Have a dedicated anti-patterns page?

[peter-csala]

I would vote for the first approach.

The second one seems less effective IMHO. For example in case of V7 there are two dedicated wiki pages for unit testing: 1, 2. But unfortunately people still tend to use them in an inappropriate way.

I did not added reasoning to the anti-patterns maybe it is worth to extend them with that.

[martincostello]

I did not added reasoning to the anti-patterns maybe it is worth to extend them with that.

Yep, is good to explain why these practices are considered bad, rather than saying they are with no justification.

[peter-csala]

@martincostello , @martintmk

I've extended the suggestions with reasoning. If this format is suitable for you then I will do the same for the previous issues as well.

[martincostello]

• In order to avoid external service calls create an http server mock either via the WireMock.Net library or via the HttpMock library

Suggests my own library to do this too 😄: https://github.com/justeattakeaway/httpclient-interception

[peter-csala]

@martincostello , @martintmk

I've updated all anti-patterns with reasoning.

[peter-csala]

Hey @martintmk, can you please help me where to add these anti-patterns?

• 1 - Overusing builder methods: Already added to the retry
• 2 - Overusing builder methods for Polly extensions: Dependency Injection page?
• 3 - Using ExecuteAndCapture to replicate Execute behavior: Fallback page maybe?
• 4 - Recreating the policies inside unit tests: Testing page?
• 5 - Accessing the IServiceCollection instead of IServiceProvider : Dependency Injection page?
• 6 - Recreating the HttpClient registration in test: Testing page?

[martintmk]

Hey @martintmk, can you please help me where to add these anti-patterns?

• 1 - Overusing builder methods: Already added to the retry
• 2 - Overusing builder methods for Polly extensions: Dependency Injection page?
• 3 - Using ExecuteAndCapture to replicate Execute behavior: Fallback page maybe?
• 4 - Recreating the policies inside unit tests: Testing page?
• 5 - Accessing the IServiceCollection instead of IServiceProvider : Dependency Injection page?
• 6 - Recreating the HttpClient registration in test: Testing page?

2 - Overusing builder methods for Polly extensions: Dependency Injection page?

Can you give me an example what are these?

• 3 - Using ExecuteAndCapture to replicate Execute behavior: Fallback page maybe?

I would put these into Resilience pipeline page.

• 4 - Recreating the policies inside unit tests: Testing page?

Yup.

• 5 - Accessing the IServiceCollection instead of IServiceProvider : Dependency Injection page?

Yup.

• 6 - Recreating the HttpClient registration in test: Testing page?

Yup.

[peter-csala]

2 - Overusing builder methods for Polly extensions: Dependency Injection page?

Can you give me an example what are these?

The AddPolicyHandler is an extension method built on the top of the IHttpClientBuilder. It allows you to decorate an HttpClient call with some policy/policies. It is done by using a special DelegatingHandler. The fundamental issue with the multiple calls of the AddPolicyHandler is that it registers multiple DelegatingHandlers into the HttpClient pipeline.

The preferred way to have a single DelegatingHandler which receives a chain of policies. In other words, the AddPolicyHandler should be called once with the result of the Policy.WrapAsync call.

Does it make sense?

[martintmk]

I think it should got into DI page then with a note that the code will be adopted to Microsoft.Extensions.Resilience.Http package once available.

[peter-csala]

I will omit that anti-pattern for now. I was thinking whether it makes sense to add some temporarily stuff to a brand new documentation page. I think it might make sense to revisit this anti-pattern once the new package is available.