Pipeline order of Retry + Fallback in polly 8

[isak1080]

Bug Report

Summary

I'm upgrading from polly 7 to 8 and follow the guide over at https://www.pollydocs.org/migration-v8.html

Specifically, the section about Policy.Wrap which says:

IMPORTANT
In v7, the policy wrap ordering is different; the policy added first was executed last (FILO). In v8, the execution order matches the
order in which they were added (FIFO).

I had a retry with fallback policy in 7.x that looked like

var retryPolicy = Policy<MyResult>.Handle<...>().RetryAsync(...);
var fallbackPolicy = Policy<MyResult>.Handle<...>().FallbackAsync(...);
// in 7 they are executed LIFO
var policy = Policy.Wrap(fallback, retry)

Running the above on v8 works fine.

My attempt to use the new ResiliencePipelineBuilder to achieve the same looks something like this:

// In v8 the are supposedly executed in FIFO order
var pipeline = new ResiliencePipelineBuilder<MyResult>()
     .AddRetry(....)
     .AddFallback(....)
    .Build();

Executing with the new pipeline always immediately goes to the fallback. Retry is never called

Interestingly, swapping the order in the pipeline builder does in fact produce the correct result.

Steps / Code to reproduce the problem

The attached sample sets up a simple retry + fallback pipeline and shows how the retry is never called unless it is the last one added to the pipeline

This is the output I get on Polly 8.0.0

Starting to execute pipeline 1
Fallback! Outcome: This will never work.
Result after pipeline 1: False
Starting to execute pipeline 2
OnRetry attempt 0. Outcome: This will never work
OnRetry attempt 1. Outcome: This will never work
Fallback! Outcome: This will never work.
Result after pipeline 2: False

public static class PollyTest
{
    public static void TestRetryFallbackAsync()
    {
        var nonWorkingPipeline = new ResiliencePipelineBuilder<bool>()
                       .AddRetry(GetRetryStrategyOptions())
                       .AddFallback(GetFallbackStrategyOptions())
                       .Build();

        Debug.WriteLine("Starting to execute pipeline 1");
        bool result = nonWorkingPipeline.Execute(() => TryButFail());
        Debug.WriteLine("Result after pipeline 1: {0}", result);
        
        
        var workingPipeline = new ResiliencePipelineBuilder<bool>()
                       .AddFallback(GetFallbackStrategyOptions())
                       .AddRetry(GetRetryStrategyOptions())
                       .Build();

        Debug.WriteLine("Starting to execute pipeline 2");
        result = workingPipeline.Execute(() => TryButFail());
        Debug.WriteLine("Result after pipeline 2: {0}", result);
    }
    
    private static FallbackStrategyOptions<bool> GetFallbackStrategyOptions()
    {
        return new FallbackStrategyOptions<bool>()
        {
            ShouldHandle = new PredicateBuilder<bool>().Handle<ApplicationException>(),
            OnFallback = args =>
            {
                Debug.WriteLine("Fallback! Outcome: {0}.", args.Outcome);
                return default;
            },
            FallbackAction = args => Outcome.FromResultAsValueTask<bool>(false),
        };
    }

    private static RetryStrategyOptions<bool> GetRetryStrategyOptions()
    {
        return new RetryStrategyOptions<bool>()
        {
            ShouldHandle = new PredicateBuilder<bool>().Handle<ApplicationException>(),
            MaxRetryAttempts = 2,
            Delay = TimeSpan.Zero,
            OnRetry = args =>
            {
                Debug.WriteLine("OnRetry attempt {0}. Outcome: {1}", args.AttemptNumber, args.Outcome);
                return default;
            },
        };
    }
    
    private static bool TryButFail()
    {
        throw new ApplicationException("This will never work");
    }
}

[martintmk]

Hey @isak1080, looking at your sample I think the behavior is correct. The thing to notice is that the fallback strategy immediately translates the ApplicationException into false result (Outcome.FromResultAsValueTask<bool>(false)).

So the retry strategy doesn't see ApplicationException but false instead and because you are only handling ApplicationException in retry strategy, it won't retry the outcome.

[isak1080]

Hmm, so are you saying that the following pipeline, the Fallback gets executed first and therefore hides the exception from Retry? I thought they were executed in the order they were added in the builder.

var pipeline= new ResiliencePipelineBuilder<bool>()
			   .AddRetry(GetRetryStrategyOptions())
			   .AddFallback(GetFallbackStrategyOptions())
			   .Build();

[martintmk]

Yes, the strategies are executed in the order they were added, however, the wording and details might be improved.

In your case, retry strategy receives the callback and executes it, passing it down to fallback strategy.

Fallback strategy uses the fallback value. At this point the execution is finished and retry strategy evaluates the result and decided that no retries are required.

[isak1080]

Ah, I think I finally understand what you mean! :)

I had the mental image wrong. Been thinking of it as a list of independent handlers which Polly would execute one by one like "Do the retry first and then if that gives up, do the fallback"

Feel free to close this issue. But may I suggest you add an example somewhere in the docs for the "retry a few times and if that fails return a fallback value" use case, because I think that is a common scenario and I couldn't find such an example.

And also some clarification on the migration guide page what it actually means to be "...executed in the same order they were added..." because even after knowing now how it works, that text confuses me. I keep thinking of "the order" as "the order they get to handle the error" which is not at all what it means.

[martintmk]

Thanks for the feedback. I have added a new info to the docs, hopefully clarifying the execution order. (#1698 )