Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Anti-patterns for Fallback in V7 #1535

Closed
peter-csala opened this issue Aug 31, 2023 · 0 comments
Closed

Anti-patterns for Fallback in V7 #1535

peter-csala opened this issue Aug 31, 2023 · 0 comments

Comments

@peter-csala
Copy link
Contributor

peter-csala commented Aug 31, 2023

As it was discussed in this PR I try to collect those anti-patterns what I have seen in past 3+ years on StackOverflow.

In this issue I will focus only on Fallback.

1 - Using fallback to replace thrown exception

❌ DON'T
Throw custom exception from the onFallback

var fallback = Policy<HttpResponseMessage>
    .Handle<HttpRequestException>()
    .Fallback(new HttpResponseMessage(),
    onFallback: (dr, _) => throw new CustomNetworkException(dr.Exception));

Reasoning:

  • Throwing an exception in an user-defined delegate is never a good idea
    • It is breaking the normal control flow

✅ DO
Use ExecuteAndCapture and then FinalException

var result = whateverPolicy.ExecuteAndCapture(...);
if(result.Outcome != OutcomeType.Successful && result.FinalExecption is HttpRequestException)
   throw new CustomNetworkException(result.FinalExecption)

Reasoning:

  • This approach executes the policy/policies 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 make the to-be-decorated method private and expose the "remapping" logic as public
public Result XYZ()
{
  var result = whateverPolicy.ExecuteAndCapture(XYZCore);
  if(result.Outcome != OutcomeType.Successful && result.FinalExecption is HttpRequestException)
     throw new CustomNetworkException(result.FinalExecption);
  ...
}

private Result XYZCore()
{
  ...
}

2 - Using retry to perform fallback

Lets suppose you have a primary and a secondary endpoint. If primary fails then you want to call the secondary.

❌ DON'T
Use retry to perform fallback

var secondaryEndpointFallback = Policy<HttpResponseMessage>
    .Handle<HttpRequestException>(ex => ex.StatusCode == HttpStatusCode.RequestTimeout)
    .OrResult(res => res.StatusCode == HttpStatusCode.RequestTimeout)
    .RetryAsync(1, async (_, __, ctx) => ctx["FallbackResult"] = await CallSecondary());

var context = new Context();
var policyResult = await secondaryEndpointFallback.ExecuteAndCaptureAsync((ctx) => CallPrimary(), context);

var result = policyResult.Outcome == OutcomeType.Successful 
            ? policyResult.Result
            : policyResult.Context["FallbackResult"];

Reasoning:

  • Retry policy by default executes the exact same operation n times
    • where n is the initial attempt + retryCount so, in this case it means 2
  • Here the fallback is produced as a side-effect rather than as a substitute

✅ DO
Use fallback to call secondary

var secondaryEndpointFallback = Policy<HttpResponseMessage>
    .Handle<HttpRequestException>(ex => ex.StatusCode == HttpStatusCode.RequestTimeout)
    .OrResult(res => res.StatusCode == HttpStatusCode.RequestTimeout)
    .FallbackAsync(ct => CallSecondary());

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 ExecuteAndCapture)

3 - Nesting ExecuteAsync calls

There are many ways to combine multiple policies together. One of the least desired one is the Execute hell

NOTE: This is not strictly related to Fallback but I've seen it many times when Fallback was the most outer.

❌ DON'T
Nest ExecuteAsync calls

var result = await secondaryEndpointFallback.ExecuteAsync(async (outerCT) =>
{
    return await primaryEndpointTimeout.ExecuteAsync(async (innerCT) =>
    {
        ...
    }, CancellationToken.None);
}, CancellationToken.None);

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 PolicyWrap to chain them

var combinedPolicy = Policy.WrapAsync(secondaryEndpointFallback, primaryEndpointTimeout);
var result = await combinedPolicy.ExecuteAsync(async (ct) => { ... }, CancellationToken.None);

Reasoning:

  • Here we are relying Polly provided escalation mechanism rather than building our own via nesting
  • The CancellationTokens are propagated between the policies automatically on your behalf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant