Polly V8: Public API Review

[martintmk]

Details on the issue fix or feature implementation

Great news! We've been working hard and we're excited to finally introduce Polly V8 for public API review. The purpose of this PR is to outline the API signatures in the upcoming release. We welcome your comments, suggestions, and improvements to refine the API. Once we reach a consensus on the outcome, we will move forward to publish alpha packages for V8.

Summary of Changes

In the following table, we provide a comparison between V7 and V8:

V7	V8	Remarks
IAsyncPolicy	ResiliencePipeline	ResiliencePipeline can be utilized in both synchronous and asynchronous scenarios.
ISyncPolicy	ResiliencePipeline	ResiliencePipeline can be utilized in both synchronous and asynchronous scenarios.
IAsyncPolicy<T>	ResiliencePipeline<T>	ResiliencePipeline<T> can be utilized in both synchronous and asynchronous scenarios.
ISyncPolicy<T>	ResiliencePipeline<T>	ResiliencePipeline<T> can be utilized in both synchronous and asynchronous scenarios.
Context	ResilienceContext
Policy	ResiliencePipelineBuilder	Utilized when the non-generic resilience strategy is required.
Policy	ResiliencePipelineBuilder<T>	Utilized when the generic resilience strategy is required.

Building ResiliencePipeline<TResult> that handles single type of the result

Below is an example of how to create a ResiliencePipeline<HttpResponseMessage> using convenience API:

ResiliencePipeline<HttpResponseMessage> strategy = new ResiliencePipelineBuilder<HttpResponseMessage>()
    .AddRetry(builder => builder
        .Handle<HttpRequestException>()
        .HandleResult(r => r.IsSuccessStatusCode))
    .AddTimeout(TimeSpan.FromSeconds(5))
    .Build();

In addition, for maximum flexibility, you can utilize options as shown below:

ResiliencePipeline<HttpResponseMessage> strategy = new ResiliencePipelineBuilder<HttpResponseMessage>()
    .AddRetry(new RetryStrategyOptions<HttpResponseMessage>
    {
        ShouldRetry = args =>
        {
            if (args .Exception is HttpRequestException)
            {
                return PredicateResult.True;
            }

            if (args.Outcome.TryGetResult(out var result) && !result.IsSuccessStatusCode)
            {
                return PredicateResult.True;
            }

            return PredicateResult.False;
        }
    })
    .AddTimeout(new TimeoutStrategyOptions
    {
        Timeout = TimeSpan.FromSeconds(5)
    })
    .Build();

Building ResiliencePipeline that handles any type of the result

For scenarios where the caller does not care about the results and only need exception handling, the non-generic resilience strategy can be used:

ResiliencePipeline strategy = new ResiliencePipelineBuilder()
    .AddRetry(new RetryStrategyOptions
    {
        ShouldRetry = args =>
        {
            if (outcome.Exception is HttpRequestException)
            {
                return PredicateResult.True;
            }

            return PredicateResult.False;
        }
    })
    .AddTimeout(new TimeoutStrategyOptions
    {
        Timeout = TimeSpan.FromSeconds(5)
    })
    .Build();

Performance

Polly V8 is FAST. We have 70x less allocations and ~20% faster execution compared to V7. Actually, most V8 strategies have zero allocations.

See our benchmark results:
https://github.com/App-vNext/Polly/tree/main/bench/BenchmarkDotNet.Artifacts/results

For more detailed information on the architecture of Polly V8, please refer to the README.md in the Polly.Core directory.

[codecov-commenter]

Codecov Report

Merging #1233 (a42c4f3) into main (46dc47f) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1233   +/-   ##
=======================================
  Coverage   83.59%   83.59%           
=======================================
  Files         268      268           
  Lines        6377     6377           
  Branches     1007     1007           
=======================================
  Hits         5331     5331           
  Misses        837      837           
  Partials      209      209           

Flag	Coverage Δ
linux	83.59% <ø> (ø)
macos	83.59% <ø> (ø)
windows	83.59% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

[andrey-noskov]

I think all non-generic variants must be nuked and we should work through the model of how to use generic flavors of policies to support all possible cases

[martintmk]

I am transferring some comments from internal review to public so these will be visible by broader audience.

Accidentally approved

[geeknoid]

You know, if you introduced a type such as Nothing (a struct with no state), you could eliminate a ton of APIs and code throughout. You'd only need to keep the generic version of everything, and if the case at hand has no state, then you'd use something like Outcome.

This is a recognized flaw in the CLR's generic system, it should have allowed to use 'void' as a generic type parameter, but it doesn't. But using Nothing achieves the same effect.

[martintmk]

You know, if you introduced a type such as Nothing (a struct with no state), you could eliminate a ton of APIs and code throughout. You'd only need to keep the generic version of everything, and if the case at hand has no state, then you'd use something like Outcome.

This is a recognized flaw in the CLR's generic system, it should have allowed to use 'void' as a generic type parameter, but it doesn't. But using Nothing achieves the same effect.

The non-generic policies are not about void results. These are used in scenarios where the consumer only cares about exceptions. This way, they can use a single policy across their entire stack and do not care about handling each individual result type.

Actually, in my previous work we did not use generic policies at all, as everything was handled using domain-specific exceptions.

@joelhulen or @martincostello can hopefully provide more details.

[andrey-noskov]

Suggested change

	public ResilienceRateLimiter? RateLimiter { get; set; }
	public Func<ValueTask<RateLimitLease>, ResilienceContext> RateLimiter { get; set; }

[martintmk]

I think you meant:

public Func<ResilienceContext, ValueTask<RateLimitLease>> RateLimiter { get; set; }

Looks good, but we would have to solve the problem with resource management i.e. disposing unused rate limiters after pipeline is reloaded.

[martintmk]

Give this a shot.

[andrey-noskov]

is this a good name?

Suggested change

	public TimeSpan Timeout { get; set; }
	public TimeSpan DefaultTimeout { get; set; }

[martintmk]

I am 50/50 on both.

[andrey-noskov]

there is no consistency across various On*Arguments types on whether ResilienceContext should be included in the args or not.

[martintmk]

The Context property is on all outcome-less arguments. All outcome-based delegates use OutcomeArguments that enforces the ResilienceContext and Outcome on construction.

[martintmk]

Let's try to get rid of OutcomeArguments

[andrey-noskov]

I would expect Result getter to throw an exception if there was an exception.
similar to how Task.Result throws a captured exception.
but is this method even needed?

[martintmk]

I think we should avoid throwing exceptions from Outcome as it is low-level API used in high-perf scenarios and in delegates.

[juraj-blazek]

This is similar pattern that's used for HttpResponseMessage to check for successful status code. Also Cosmos DB SDK has similar implementation - you either work with DTOs directly (higher level) and get the exceptions, or you work with ResponseMessage (lower level) but need to check for the result explicitly using response.EnsureSuccessStatusCode().

[martintmk]

Closing in favor of #1507

[martintmk]

@martincostello, closed this one and opened #1507 with remaining comments transferred there.