[Feature request]: Simpler way to do basic ShouldHandle delegates for retry

[martincostello]

Is your feature request related to a specific problem? Or an existing feature?

Doing some changes to an application of my own, it occurred to me that for basic use cases for retries, the syntax is more verbose than it could be:

// From https://github.com/martincostello/costellobot/blob/e3ef21ed1db09c65e876a3d2b2650184bde69c62/src/Costellobot/Handlers/DeploymentStatusHandler.cs#L106-L115
var pipeline = new ResiliencePipelineBuilder()
    .AddRetry(new()
    {
        Delay = TimeSpan.FromSeconds(2),
        ShouldHandle = new PredicateBuilder().Handle<ApiValidationException>(),
    })
    .Build();

The PredicateBuilder class is a good way to express complex conditions, but needing to allocate the object (and the underlying array) to then create the delegate to assign to ShouldHandle seems a bit over-the-top.

It feels like we could add something that's a bit simpler (and then implement PredicateBuilder in terms of it).

This is just a rough sketch of an idea to see what people think for now, I haven't tried to actually prototype it yet or anything.

Describe the solution you'd like

Something like this, where the existing instance methods in the PredicateBuilder would just call new static methods, but simple cases can use them directly:

public static class ResiliencePredicates
{
    public static Predicate<Outcome<TResult>> Handle<TException>()
        where TException : Exception
    {
        // Implementation ...
    }

    public static Predicate<Outcome<TResult>> Handle<TException>(Func<TException, bool> predicate)
        where TException : Exception
    {
        // Implementation ...
    }

    // etc.
}

That would then simplify the original example to something like this:

var pipeline = new ResiliencePipelineBuilder()
    .AddRetry(new()
    {
        Delay = TimeSpan.FromSeconds(2),
-       ShouldHandle = new PredicateBuilder().Handle<ApiValidationException>(),
+       ShouldHandle = ResiliencePredicates.Handle<ApiValidationException>,
    })
    .Build();

Additional context

No response

[peter-csala]

Your proposed solution (as far as I understand) works with single exception type only. You can't chain them.

IMHO having different classes for simple and advanced scenarios does not help the usage of the API.

If we want to rewrite the predicate builder then we should look at how others have address this problem.

Like resilience4j or guava-retrying.

[martincostello]

Your proposed solution (as far as I understand) works with single exception type only. You can't chain them.

Exactly - this suggestion is about making the simplest cases easier where you only want to check for one thing, not many.

It's just on reflection seems a bit overly-verbose and wasteful to allocate an object and then immediately throw it away. I guess if you cared enough about that you could just write a delegate directly instead, but then you need to account for the ValueTask and Outcome machinery so then it ends up being more complicated.

We have all the pieces, they're just hidden away in the internals.

[martintmk]

Hey @martincostello

I think making this work will be more difficult, even not possible.

The thing is that the predicates are more complex than Predicate<Outcome<TResult>> and assigning it to RetryOptions.ShouldHandle is not possible. For example, this is the signature of RetryOptions.ShouldHandle predicate:

Func<RetryPredicateArguments<object>, ValueTask<bool>>

Assigning PredicateBuilder to this value works because we define a custom converter to this delegate:

Polly/src/Polly.Core/PredicateBuilder.Operators.cs

Line 18 in badf0df

public static implicit operator Func<RetryPredicateArguments<TResult>, ValueTask<bool>>(PredicateBuilder<TResult> builder)

Now, we cannot do the same for Predicate<Outcome<TResult>>, I have actually tried before :)

It's just on reflection seems a bit overly-verbose and wasteful to allocate an object and then immediately throw it away.
FYI, this is "do once and never again" as it happens only when initializing the pipeline. There are no allocations on hot path.

[github-actions]

This issue is stale because it has been open for 60 days with no activity. It will be automatically closed in 14 days if no further updates are made.

[github-actions]

This issue was closed because it has been inactive for 14 days since being marked as stale.