Add antipatterns to fallback strategy (#1607)

docs/strategies/fallback.md

@@ -67,3 +67,195 @@ new ResiliencePipelineBuilder<UserAvatar>()
 | `ShouldHandle`   | Predicate that handles all exceptions except `OperationCanceledException`. | Predicate that determines what results and exceptions are handled by the fallback strategy. |
 | `FallbackAction` | `Null`, **Required**                                                       | Fallback action to be executed.                                                             |
 | `OnFallback`     | `null`                                                                     | Event that is raised when fallback happens.                                                 |
+
+
+## Patterns and Anti-patterns
+Throughout the years many people have used Polly in so many different ways. Some reoccuring patterns are suboptimal. So, this section shows the donts and dos.
+
+### 1 - Using fallback to replace thrown exception
+
+❌ DON'T
+
+Throw custom exception from the `OnFallback`
+
+<!-- snippet: fallback-anti-pattern-1 -->
+```cs
+var fallback = new ResiliencePipelineBuilder<HttpResponseMessage>()
+    .AddFallback(new()
+    {
+        ShouldHandle = new PredicateBuilder<HttpResponseMessage>().Handle<HttpRequestException>(),
+        FallbackAction = args => Outcome.FromResultAsValueTask(new HttpResponseMessage()),
+        OnFallback = args => throw new CustomNetworkException("Replace thrown exception", args.Outcome.Exception!)
+    })
+    .Build();
+```
+<!-- endSnippet -->
+
+**Reasoning**:
+- Throwing an exception in an user-defined delegate is never a good idea because it is breaking the normal control flow.
+
+✅ DO
+
+Use `ExecuteOutcomeAsync` and then assess `Exception`
+
+<!-- snippet: fallback-pattern-1 -->
+```cs
+var outcome = await WhateverPipeline.ExecuteOutcomeAsync(Action, context, "state");
+if (outcome.Exception is HttpRequestException hre)
+{
+    throw new CustomNetworkException("Replace thrown exception", hre);
+}
+```
+<!-- endSnippet -->
+
+**Reasoning**:
+- This approach executes the strategy/pipeline without "jumping out from the normal flow"
+- If you find yourself in a situation that you write this Exception "remapping" logic again and again
+  - then mark the to-be-decorated method as `private`
+  - and expose the "remapping" logic as `public`
+
+<!-- snippet: fallback-pattern-1-ext -->
+```cs
+public static async ValueTask<HttpResponseMessage> Action()
+{
+    var context = ResilienceContextPool.Shared.Get();
+    var outcome = await WhateverPipeline.ExecuteOutcomeAsync<HttpResponseMessage, string>(
+        async (ctx, state) =>
+        {
+            var result = await ActionCore();
+            return Outcome.FromResult(result);
+        }, context, "state");
+
+    if (outcome.Exception is HttpRequestException hre)
+    {
+        throw new CustomNetworkException("Replace thrown exception", hre);
+    }
+
+    ResilienceContextPool.Shared.Return(context);
+    return outcome.Result!;
+}
+
+private static ValueTask<HttpResponseMessage> ActionCore()
+{
+    // The core logic
+    return ValueTask.FromResult(new HttpResponseMessage());
+}
+```
+<!-- endSnippet -->
+
+### 2 - Using retry to perform fallback
+
+Lets suppose you have a primary and a secondary endpoints. If primary fails then you want to call the secondary.
+
+❌ DON'T
+
+Use retry to perform fallback
+
+<!-- snippet: fallback-anti-pattern-2 -->
+```cs
+var fallback = new ResiliencePipelineBuilder<HttpResponseMessage>()
+    .AddRetry(new()
+    {
+        ShouldHandle = new PredicateBuilder<HttpResponseMessage>()
+            .HandleResult(res => res.StatusCode == HttpStatusCode.RequestTimeout),
+        MaxRetryAttempts = 1,
+        OnRetry = async args =>
+        {
+            args.Context.Properties.Set(fallbackKey, await CallSecondary(args.Context.CancellationToken));
+        }
+    })
+    .Build();
+
+var context = ResilienceContextPool.Shared.Get();
+var outcome = await fallback.ExecuteOutcomeAsync<HttpResponseMessage, string>(
+    async (ctx, state) =>
+    {
+        var result = await CallPrimary(ctx.CancellationToken);
+        return Outcome.FromResult(result);
+    }, context, "none");
+
+var result = outcome.Result is not null
+    ? outcome.Result
+    : context.Properties.GetValue(fallbackKey, default);
+
+ResilienceContextPool.Shared.Return(context);
+
+return result;
+```
+<!-- endSnippet -->
+
+**Reasoning**:
+- Retry strategy by default executes the exact same operation at most `n` times
+  - where `n` equals to the initial attempt + `MaxRetryAttempts`
+  - So, in this particular case this means __2__
+- Here the fallback is produced as a side-effect rather than as a substitute
+
+✅ DO
+
+Use fallback to call secondary
+
+<!-- snippet: fallback-pattern-2 -->
+```cs
+var fallback = new ResiliencePipelineBuilder<HttpResponseMessage>()
+    .AddFallback(new()
+    {
+        ShouldHandle = new PredicateBuilder<HttpResponseMessage>()
+            .HandleResult(res => res.StatusCode == HttpStatusCode.RequestTimeout),
+        OnFallback = async args => await CallSecondary(args.Context.CancellationToken)
+    })
+    .Build();
+
+return await fallback.ExecuteAsync(CallPrimary, CancellationToken.None);
+```
+<!-- endSnippet -->
+
+**Reasoning**:
+- The to-be-decorated code is executed only once
+- The fallback value will be returned without any extra code (no need for `Context` or `ExecuteOutcomeAsync`)
+
+### 3 - Nesting `ExecuteAsync` calls
+
+There are many ways to combine multiple strategies together. One of the least desired one is the `Execute` hell.
+
+> [!NOTE]
+> This is not strictly related to Fallback but we have seen it many times when Fallback was the most outer.
+
+❌ DON'T
+
+Nest `ExecuteAsync` calls
+
+<!-- snippet: fallback-anti-pattern-3 -->
+```cs
+var result = await fallback.ExecuteAsync(async (CancellationToken outerCT) =>
+{
+    return await timeout.ExecuteAsync(async (CancellationToken innerCT) =>
+    {
+        return await CallExternalSystem(innerCT);
+    }, outerCT);
+}, CancellationToken.None);
+
+return result;
+```
+<!-- endSnippet -->
+
+**Reasoning**:
+- This is the same as javascript's callback hell or pyramid of doom
+- It is pretty easy to refer to the wrong `CancellationToken` parameter
+
+✅ DO
+Use `ResiliencePipelineBuilder` to chain them
+
+<!-- snippet: fallback-pattern-3 -->
+```cs
+var pipeline = new ResiliencePipelineBuilder<HttpResponseMessage>()
+    .AddPipeline(timeout)
+    .AddPipeline(fallback)
+    .Build();
+
+return await pipeline.ExecuteAsync(CallExternalSystem, CancellationToken.None);
+```
+<!-- endSnippet -->
+
+**Reasoning**:
+- Here we are relying Polly provided escalation mechanism rather than building our own via nesting
+- The `CancellationToken`s are propagated between the policies automatically on your behalf

src/Snippets/Docs/Fallback.cs

@@ -1,4 +1,6 @@
-using Polly.Fallback;
+using System.Net;
+using System.Net.Http;
+using Polly.Fallback;
 using Snippets.Docs.Utils;
 
 namespace Snippets.Docs;
@@ -61,4 +63,173 @@ public class UserAvatar
 
         public static UserAvatar GetRandomAvatar() => new();
     }
+
+    private class CustomNetworkException : Exception
+    {
+        public CustomNetworkException()
+        {
+        }
+
+        public CustomNetworkException(string message)
+        : base(message)
+        {
+        }
+
+        public CustomNetworkException(string message, Exception innerException)
+        : base(message, innerException)
+        {
+        }
+    }
+
+    public static void AntiPattern_1()
+    {
+        #region fallback-anti-pattern-1
+
+        var fallback = new ResiliencePipelineBuilder<HttpResponseMessage>()
+            .AddFallback(new()
+            {
+                ShouldHandle = new PredicateBuilder<HttpResponseMessage>().Handle<HttpRequestException>(),
+                FallbackAction = args => Outcome.FromResultAsValueTask(new HttpResponseMessage()),
+                OnFallback = args => throw new CustomNetworkException("Replace thrown exception", args.Outcome.Exception!)
+            })
+            .Build();
+
+        #endregion
+    }
+
+    private static readonly ResiliencePipeline<HttpResponseMessage> WhateverPipeline = ResiliencePipeline<HttpResponseMessage>.Empty;
+    private static ValueTask<Outcome<HttpResponseMessage>> Action(ResilienceContext context, string state) => Outcome.FromResultAsValueTask(new HttpResponseMessage());
+    public static async Task Pattern_1()
+    {
+        var context = ResilienceContextPool.Shared.Get();
+        #region fallback-pattern-1
+
+        var outcome = await WhateverPipeline.ExecuteOutcomeAsync(Action, context, "state");
+        if (outcome.Exception is HttpRequestException hre)
+        {
+            throw new CustomNetworkException("Replace thrown exception", hre);
+        }
+        #endregion
+
+        ResilienceContextPool.Shared.Return(context);
+    }
+
+    #region fallback-pattern-1-ext
+    public static async ValueTask<HttpResponseMessage> Action()
+    {
+        var context = ResilienceContextPool.Shared.Get();
+        var outcome = await WhateverPipeline.ExecuteOutcomeAsync<HttpResponseMessage, string>(
+            async (ctx, state) =>
+            {
+                var result = await ActionCore();
+                return Outcome.FromResult(result);
+            }, context, "state");
+
+        if (outcome.Exception is HttpRequestException hre)
+        {
+            throw new CustomNetworkException("Replace thrown exception", hre);
+        }
+
+        ResilienceContextPool.Shared.Return(context);
+        return outcome.Result!;
+    }
+
+    private static ValueTask<HttpResponseMessage> ActionCore()
+    {
+        // The core logic
+        return ValueTask.FromResult(new HttpResponseMessage());
+    }
+    #endregion
+
+    private static ValueTask<HttpResponseMessage> CallPrimary(CancellationToken ct) => ValueTask.FromResult(new HttpResponseMessage());
+    private static ValueTask<HttpResponseMessage> CallSecondary(CancellationToken ct) => ValueTask.FromResult(new HttpResponseMessage());
+    public static async Task<HttpResponseMessage?> AntiPattern_2()
+    {
+        var fallbackKey = new ResiliencePropertyKey<HttpResponseMessage?>("fallback_result");
+
+        #region fallback-anti-pattern-2
+
+        var fallback = new ResiliencePipelineBuilder<HttpResponseMessage>()
+            .AddRetry(new()
+            {
+                ShouldHandle = new PredicateBuilder<HttpResponseMessage>()
+                    .HandleResult(res => res.StatusCode == HttpStatusCode.RequestTimeout),
+                MaxRetryAttempts = 1,
+                OnRetry = async args =>
+                {
+                    args.Context.Properties.Set(fallbackKey, await CallSecondary(args.Context.CancellationToken));
+                }
+            })
+            .Build();
+
+        var context = ResilienceContextPool.Shared.Get();
+        var outcome = await fallback.ExecuteOutcomeAsync<HttpResponseMessage, string>(
+            async (ctx, state) =>
+            {
+                var result = await CallPrimary(ctx.CancellationToken);
+                return Outcome.FromResult(result);
+            }, context, "none");
+
+        var result = outcome.Result is not null
+            ? outcome.Result
+            : context.Properties.GetValue(fallbackKey, default);
+
+        ResilienceContextPool.Shared.Return(context);
+
+        return result;
+
+        #endregion
+    }
+
+    public static async ValueTask<HttpResponseMessage?> Pattern_2()
+    {
+        #region fallback-pattern-2
+
+        var fallback = new ResiliencePipelineBuilder<HttpResponseMessage>()
+            .AddFallback(new()
+            {
+                ShouldHandle = new PredicateBuilder<HttpResponseMessage>()
+                    .HandleResult(res => res.StatusCode == HttpStatusCode.RequestTimeout),
+                OnFallback = async args => await CallSecondary(args.Context.CancellationToken)
+            })
+            .Build();
+
+        return await fallback.ExecuteAsync(CallPrimary, CancellationToken.None);
+
+        #endregion
+    }
+
+    private static ValueTask<HttpResponseMessage> CallExternalSystem(CancellationToken ct) => ValueTask.FromResult(new HttpResponseMessage());
+    public static async ValueTask<HttpResponseMessage?> Anti_Pattern_3()
+    {
+        var timeout = ResiliencePipeline<HttpResponseMessage>.Empty;
+        var fallback = ResiliencePipeline<HttpResponseMessage>.Empty;
+
+        #region fallback-anti-pattern-3
+        var result = await fallback.ExecuteAsync(async (CancellationToken outerCT) =>
+        {
+            return await timeout.ExecuteAsync(async (CancellationToken innerCT) =>
+            {
+                return await CallExternalSystem(innerCT);
+            }, outerCT);
+        }, CancellationToken.None);
+
+        return result;
+        #endregion
+    }
+
+    public static async ValueTask<HttpResponseMessage?> Pattern_3()
+    {
+        var timeout = ResiliencePipeline<HttpResponseMessage>.Empty;
+        var fallback = ResiliencePipeline<HttpResponseMessage>.Empty;
+
+        #region fallback-pattern-3
+        var pipeline = new ResiliencePipelineBuilder<HttpResponseMessage>()
+            .AddPipeline(timeout)
+            .AddPipeline(fallback)
+            .Build();
+
+        return await pipeline.ExecuteAsync(CallExternalSystem, CancellationToken.None);
+        #endregion
+    }
 }