[Bug] Documentation has incorrect examples for ExecuteOutcomeAsync

[kevincathcart-cas]

Many of the examples of ExecuteOutcomeAsync in the documentation are buggy. Per the xml documentation: "The caller must make sure that the callback does not throw any exceptions.". The discussion in #2025 confirms this.

But the documentation has a number of places where this is not done properly.

Buggy example in Migration Guide

For example, the migration guide says that:

var context = ResilienceContextPool.Shared.Get();
Outcome<int> pipelineResult = await pipeline.ExecuteOutcomeAsync(
    static async (ctx, state) => Outcome.FromResult(await MethodAsync(ctx.CancellationToken)), context, "state");
ResilienceContextPool.Shared.Return(context);

is the V8 equivalent to

PolicyResult<int> asyncPolicyResult = await asyncPolicy.ExecuteAndCaptureAsync(MethodAsync, CancellationToken.None);

But that is isn't correct. The equivalent is:

var context = ResilienceContextPool.Shared.Get();
Outcome<int> pipelineResult = await pipeline.ExecuteOutcomeAsync(
    static async (ctx, state) => {
        try 
        {
            return Outcome.FromResult(await MethodAsync(ctx.CancellationToken));
        }
        catch (Exception e)
        {
            return Outcome.FromException<int>(e);
        }
    }), context, "state");
ResilienceContextPool.Shared.Return(context);

The code without the catch could potentially be correct for a callback that will never throw and instead indicates errors with a 0 or negative return or something, but this example is trying to show the equivalents, which makes it incorrect.

Buggy example on Fallback strategy page

Look at the reasoning portion of the "Using fallback to replace thrown exception" antipattern section:

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 requestException)
    {
        throw new CustomNetworkException("Replace thrown exception", requestException);
    }

    ResilienceContextPool.Shared.Return(context);
    return outcome.Result!;
}

The callback (which must not throw) can only return a successful result, but the code is looking for an HttpRequestException in the returned outcome, which almost certainly would be coming from the callback.

Buggy example on Circuit breaker strategy page

This one is from the Do example for Circuit Breaker's "Reducing thrown exceptions" section:

var context = ResilienceContextPool.Shared.Get();
var circuitBreaker = new ResiliencePipelineBuilder()
    .AddCircuitBreaker(new()
    {
        ShouldHandle = new PredicateBuilder().Handle<HttpRequestException>(),
        BreakDuration = TimeSpan.FromSeconds(0.5),
    })
    .Build();

Outcome<HttpResponseMessage> outcome = await circuitBreaker.ExecuteOutcomeAsync(static async (ctx, state) =>
{
    var response = await IssueRequest();
    return Outcome.FromResult(response);
}, context, "state");

ResilienceContextPool.Shared.Return(context);

if (outcome.Exception is BrokenCircuitException)
{
    // The execution was stopped by the circuit breaker
}
else
{
    HttpResponseMessage response = outcome.Result!;
    // Your code goes here to process the response
}

Here we a circuit breaker looking for HttpRequestException, but once again, the callback (which must not throw) can only return a result outcome.

Aside: This example is also not great because the code afterward looks likely to be totally ignoring any exception other than BrokenCircuitException, including the HttpRequestException that we know can happen, as that is what gets used to break the circuit). Removing the line getting .Result!, and changing the comment to be //Execution occurred without being stopped by the breaker. check outcome.Result for result or outcome.Exception for any thrown exception`, would fix this.

In fact I can find only two places in the documentation show the correct pattern for wrapping user code that might throw: the Performance page and the Pipelines page.

While these bad examples can simply be fixed, it is worth questioning why they happened. Part of it comes from the simple fact is that while not correct, incorrectly letting the exception escape will work just fine in many cases, as long as the last strategy in the pipeline is one of the built in ones that use StrategyHelper.ExecuteCallbackSafeAsync, (which I know both Retry and Circuit Breaker do, and maybe some others too).

But this is also partly due to the design of having only one overload of ExecuteOutcomeAsync, namely: the fastest, lowest allocation version possible. Now I certainly agree that the fastest lowest allocation version should be provided. But I'm not sure it needs to be the only overload. Clearly most of these examples really want an overload with a callback signature more like Func<ResilienceContext, TState, ValueTask<TResult>>, instead of Func<ResilienceContext, TState, ValueTask<Outcome<TResult>>>, which would avoid this correctness issues.

That would be easy to implement via a wrapping static async helper like is done with ExecuteAsync methods currently. It might even be possible to trade some allocations to be faster, using some approach similar to #2132.

The primary downside is of adding such an overload would needing to be more specific when trying to mention the approach with the minimal allocations and best speed, that would only the overload that takes the Func<ResilienceContext, TState, ValueTask<Outcome<TResult>>>.

[martincostello]

Thanks for raising this.

A helper overload sounds reasonable, provided it wouldn't create any resolution overload ambiguity for existing code.

We already have a performance-specific page in our documentation, so more specific recommendations for the performance conscious could go there.

Would you like to open a PR to try adding this overload and making the appropriate updates to the documentation?