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

[Docs] Anti-pattern docs #1558

Closed
martintmk opened this issue Sep 6, 2023 · 6 comments
Closed

[Docs] Anti-pattern docs #1558

martintmk opened this issue Sep 6, 2023 · 6 comments

Comments

@martintmk
Copy link
Contributor

We should consolidate the info from anti-patterns issues (thanks to @peter-csala) into official documentation:

Every strategy has now dedicated page under the /docs/strategies folder. We should add the Anti-patterns sections to the bottom of each respective strategy and extract the content from the individual issues.

Note that the examples need to be converted to v8 API, optionally we can also include them in the v7 wiki.

@peter-csala
Copy link
Contributor

peter-csala commented Sep 20, 2023

Let me try to give it a shot to try to convert examples to V8

Misc 1.

Don't

var retryPolicy = new ResiliencePipelineBuilder()
    .AddRetry(new() {
        ShouldHandle = new PredicateBuilder()
        .Handle<HttpRequestException>()
        .Handle<BrokenCircuitException>()
        .Handle<TimeoutRejectedException>()
        .Handle<SocketException>()
        .Handle<RateLimitRejectedException>(),
        MaxRetryAttempts = ...,
    })
    .Build();

Do

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

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

var retryPolicy = new ResiliencePipelineBuilder()
    .AddRetry(new() {
        ShouldHandle = ex => new ValueTask<bool>(
            networkExceptions.Union(policyExceptions).Contains(ex.GetType())),
        MaxRetryAttempts = ...,
    })
    .Build();

Misc 2.

Quite frankly I did not find any resource how to integrate ResiliencePipeline with HttpClient. The pre-release versions of Microsoft.Extensions.Http.Polly still anticipates IAsyncPolicy<HttpResponseMessage>.

Misc 3.

Don't

var context = ResilienceContextPool.Shared.Get();
Outcome<int> outcome = await policy.ExecuteOutcomeAsync(
    static async (context, state) =>
    {
        var result = await Action(context.CancellationToken);
        return Outcome.FromResult(result);
    },
    context, "state");
ResilienceContextPool.Shared.Return(context);

outcome.ThrowIfException();
return outcome.Result;

Do

var outcome = await policy.ExecuteAsync(Action);

Misc 4.

Don't

var timeout = new ResiliencePipelineBuilder().AddTimeout(TimeSpan.FromSeconds(1)).Build();

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, timeout);
 await sut.Call();  

Do

I'm not sure about the NoOp

var policyMock = ResiliencePipeline.Empty; 

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

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

Misc 5.

Same as misc 2.

Misc 6.

Same as misc 2.

@martintmk What do you think?

@martintmk
Copy link
Contributor Author

@peter-csala

We actually have migration guide for v8 published now :)

https://www.pollydocs.org/migration-v8.html

So if you or anyone else from community wants to take a stab at adding the anti-pattern to docs here is the flow:

@peter-csala
Copy link
Contributor

@martintmk

Unfortunately few things are missing from the migration doc: ExecuteAndCapture, HttpClient integration.

@martintmk
Copy link
Contributor Author

Yes, we will add more content as the feedback comes in.

@peter-csala
Copy link
Contributor

@martintmk Alright then assign to me this issue.

@peter-csala
Copy link
Contributor

@martintmk You can close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants