Simplifying predicate configuration for resilience strategies in V8

[martintmk]

In Polly V8, resilience strategies are configured using options and delegates.

Here are some patterns you can use to accomplish this:

// Switch-based pattern (recommended)
builder.AddRetry(new RetryStrategyOptions
{
    ShouldHandle = args => args.Exception switch
    {
        InvalidOperationException => PredicateResult.True,
        _ => PredicateResult.False
    }
});

// Simplified pattern
builder.AddRetry(new RetryStrategyOptions
{
    ShouldHandle = args =>
    {
        if (args.Exception is InvalidOperationException)
        {
            return PredicateResult.True;
        }

        return PredicateResult.False;
    }
});

// Inline pattern
builder.AddRetry(new RetryStrategyOptions
{
    ShouldHandle = args => new ValueTask<bool>(args.Exception is InvalidOperationException)
});

For handling results, the pattern is slightly more complex:

builder.AddRetry(new RetryStrategyOptions<HttpResponseMessage>
{
    ShouldHandle = args => args switch
    {
        { Exception: InvalidOperationException } => PredicateResult.True,
        { Result: HttpResponseMessage response } when !response.IsSuccessStatusCode => PredicateResult.True,
        _ => PredicateResult.False
    }
});

I'm currently exploring ways to streamline the configuration of predicates. Here are some options I've considered:

Option 1: Introduction of new Constructors

This would involve adding new sets of constructors for every generic option:

public class RetryStrategyOptions<TResult> : ResilienceStrategyOptions
{
    public RetryStrategyOptions()
    {
        // ...
    }

    public RetryStrategyOptions(PredicateBuilder<TResult> shouldHandle)
    {
        // ...
    }

    public RetryStrategyOptions(Func<OutcomeArguments<TResult, RetryPredicateArguments>> shouldHandle)
    {
        // ...
    }

With these, you can now configure strategies like so:

// Simple
builder.AddRetry(new RetryStrategyOptions<HttpResponseMessage>(args => args.Exception is InvalidOperationException));

// With results
builder.AddRetry(new RetryStrategyOptions<HttpResponseMessage>(args =>
{
    if (args.Exception is InvalidOperationException)
    {
        return true;
    }

    if (args.Result != null && !args.Result.IsSuccessStatusCode)
    {
        return false;
    }

    return false;
}));

// Using predicate builder
var predicate = new PredicateBuilder<HttpResponseMessage>()
    .Handle<InvalidOperationException>()
    .HandleResult(r => !r.IsSuccessStatusCode);

builder.AddRetry(new RetryStrategyOptions<HttpResponseMessage>(predicate));

This option complements the pattern well but limits extensibility to constructors only.

Option 2: Use extension methods for options

In this scenario, the previous example would look like this:

// Simple
builder.AddRetry(new RetryStrategyOptions<HttpResponseMessage>().WithShouldHandle(args => args.Exception is InvalidOperationException));

// With results
builder.AddRetry(new RetryStrategyOptions<HttpResponseMessage>().WithShouldHandle(args =>
{
    if (args.Exception is InvalidOperationException)
    {
        return true;
    }

    if (args.Result != null && !args.Result.IsSuccessStatusCode)
    {
        return false;
    }

    return false;
}));

// Using predicate builder
var predicate = new PredicateBuilder<HttpResponseMessage>()
    .Handle<InvalidOperationException>()
    .HandleResult(r => !r.IsSuccessStatusCode);

builder.AddRetry(new RetryStrategyOptions<HttpResponseMessage>(predicate).WithShouldHandle(builder));

While this method allows for greater future extensibility, the syntax is a bit more complex.

Option 3: Extensions for builders

Here, we'd offer extensions for the ResilienceStrategyBuilder to facilitate easier predicate configuration. For instance, these convenience extensions could take the handling configuration as the first parameter, followed by the options.

// Simple
builder.AddRetry(args => args.Exception is InvalidOperationException, new RetryStrategyOptions<HttpResponseMessage>());

// With results
builder.AddRetry(
   args =>
  {
      if (args.Exception is InvalidOperationException)
      {
          return true;
      }
  
      if (args.Result != null && !args.Result.IsSuccessStatusCode)
      {
          return false;
      }
  
      return false;
  }, 
  new RetryStrategyOptions<HttpResponseMessage>());

// Using predicate builder
var predicate = new PredicateBuilder<HttpResponseMessage>()
    .Handle<InvalidOperationException>()
    .HandleResult(r => !r.IsSuccessStatusCode);

builder.AddRetry(predicate , new RetryStrategyOptions<HttpResponseMessage>(predicate));

This approach could lead to the development of more extensions and even the complete omission of options. However, the number of extension types could potentially increase exponentially.

Option 4: Implicit conversion from PredicateBuilder to delegate

This allows the following usage:

new ResilienceStrategyBuilder<HttpResponseMessage>()
    .AddRetry(new()
    {
        ShouldHandle = new PredicateBuilder<HttpResponseMessage>()
            .Handle<HttpRequestException>()
            .HandleResult(result => result.IsSuccessStatusCode)
    });

Or for non-generic options:

new ResilienceStrategyBuilder()
    .AddRetry(new()
    {
        ShouldHandle = new PredicateBuilder().Handle<HttpRequestException>()
    });

The idea si to define built-in implicit conversion between PredicateBuilder and the delegate:

public static implicit operator Func<OutcomeArguments<TResult, RetryPredicateArguments>, ValueTask<bool>>(PredicateBuilder<TResult> builder)
{
    return builder.CreatePredicate<RetryPredicateArguments>();
}

We wouldn't expose any additional extensions for individual strategies. Simple use-cases can be done with PredicateBuilder while the more complex (args access, async predicates) can be done by defining the full delegate.

Option 5: Embracing switch Expressions, no new API

This option involves accepting switch expressions for all predicate configurations. It keeps the API minimal, but the configuration could seem foreign to those unfamiliar with the latest C# features.

I welcome your thoughts on these options. @martincostello, @joelhulen, @geeknoid, what are your preferences? Are there any other alternatives you'd suggest?

cc @PeterCsalaHbo

[martincostello]

Thinking out loud, is there a fifth option with some kind of implicit delegate conversion?

I'll read through options 1-3 properly a bit later today, but I would agree that we should do something. The feedback about usage of expressions sort of maps with my earlier hesitancy on using newer language features might look a bit "complicated" to the "average developer" just trying to adopt Polly.

[martintmk]

Thinking out loud, is there a fifth option with some kind of implicit delegate conversion?

I would love this, if some implicit conversion was allowed. But so far, I don't know how. Any tips?

[martincostello]

A quick Google suggests I'm just imagining things 😄

I guess we'd need to go with the overloads option?

[martintmk]

@martincostello

This actually gave me idea. We can combine implicit conversion with PredicateBuilder and allow:

public static implicit operator Func<OutcomeArguments<TResult, RetryPredicateArguments>, ValueTask<bool>>(PredicateBuilder<TResult> builder)
{
    return builder.CreatePredicate<RetryPredicateArguments>();
}

new ResilienceStrategyBuilder<HttpResponseMessage>()
    .AddRetry(new()
    {
        ShouldHandle = new PredicateBuilder<HttpResponseMessage>()
            .Handle<HttpRequestException>()
            .HandleResult(result => result.IsSuccessStatusCode)
    });

Or even simpler for-non-generic options:

new ResilienceStrategyBuilder()
    .AddRetry(new()
    {
        ShouldHandle = new PredicateBuilder().Handle<HttpRequestException>()
    });

Wdyt?

[martincostello]

I like it

[martintmk]

I like it

Me too, I put it as another option.

[adamnova]

I like the option 4 the most, from my point of view it is the only one that really adds value to the API. Here are my opinions on the options listed.

Option 1 seems kind of nice, but it does the same thing as initializer, but requires more work to make and maintain.
Option 2 provides API that is more standard but in the end does not really simplify anything.
Option 3 could get really complicated over time, at times it can be easier to find a property, than the 15th overload that you want.
Option 4 makes it easier to use PredicateBuilder, like a first class citizen 👍
Option 5 is an option as well 😄

[geeknoid]

I'm with @adamnova , I like option 4 too.

[martincostello]

In descending order of preference: 4, 3, 2, 1, 5.

[martintmk]

The only downside of 4 is that it will only work with built-in options because of:
https://learn.microsoft.com/en-us/dotnet/csharp/misc/cs0556?f1url=%3FappId%3Droslyn%26k%3Dk(CS0556)

However, we can have a common PredicateArguments and allow implicit conversion from these delegate types:

Func<OutcomeArguments<TResult, PredicateArguments>, ValueTask<bool>>
Func<Outcome<TResult>. bool>

Or for custom strategies callers can just call the Build:

var handle = new PredicateBuilder<HttpResponseMessage>()
            .Handle<HttpRequestException>()
            .HandleResult(result => result.IsSuccessStatusCode)
            .Build();

[juraj-blazek]

I like 4. But considering it doesn't cover 100% of cases, we can also just stick with requiring the call to Build():

new ResilienceStrategyBuilder()
    .AddRetry(new()
    {
        ShouldHandle = new PredicateBuilder().Handle<HttpRequestException>().Build()
    });