[Question]: Usage of ExecuteOutcomeAsync

[troygould-stack]

What are you wanting to achieve?

Reading the documentation for ResiliencePipeline<T>.ExecuteOutcomeAsync, and in the remarks section it mentions "The caller must make sure that the callback does not throw any exceptions. Instead, it converts them to Outcome<TResult>."

When I jump through the source code, I end up in the RetryResilienceStrategy which uses the StrategyHelper. Within the StrategyHelper, it has a try/catch around the call to the callback method. Thus, when using ExecuteOutcomeAsync, a user isn't required to catch Exceptions and call Outcome.FromException<TResult>(exception) themselves. The Polly framework already does this.

This is causing some confusion on whether or not we need to add a try/catch blocks within the callback, and return an Outcome with Exception property set, or just let the Polly framework do it for us. Some guidance on this would be appreciated.

What code or approach do you have so far?

Catching exceptions within the callback requires me to be very careful about what exceptions I do catch because this can interfere with the ShouldHandle method of of a retry strategy.

Additional context

No response

[peter-csala]

I've put together two simple unit tests.

[Fact]
public async Task ThrowNotHandledException()
{
    //Arrange
    const int expectedRetryCount = 0;
    int actualRetryCount = 0;
    var context = ResilienceContextPool.Shared.Get();
    var retry = new ResiliencePipelineBuilder<int>()
    .AddRetry(new () {
        ShouldHandle = new PredicateBuilder<int>().Handle<MyException>(),
        MaxRetryAttempts = 3,
        OnRetry = _ =>
        {
            actualRetryCount++;
            return default;
        }
    }).Build();

    //Act
    Outcome<int> actual = await retry.ExecuteOutcomeAsync<int, string>((ctx, stt) => throw new Exception("blah"), context, "a");
    ResilienceContextPool.Shared.Return(context);

    //Assert
    Assert.Equal(expectedRetryCount, actualRetryCount);
    Assert.IsType<Exception>(actual.Exception);
}

[Fact]
public async Task ThrowHandledException()
{
    //Arrange
    const int expectedRetryCount = 3;
    int actualRetryCount = 0;
    var context = ResilienceContextPool.Shared.Get();
    var retry = new ResiliencePipelineBuilder<int>()
    .AddRetry(new () {
        ShouldHandle = new PredicateBuilder<int>().Handle<MyException>(),
        MaxRetryAttempts = 3,
        OnRetry = _ =>
        {
            actualRetryCount++;
            return default;
        }
    }).Build();

    //Act
    Outcome<int> actual = await retry.ExecuteOutcomeAsync<int, string>((ctx, stt) => throw new MyException(), context, "a");
    ResilienceContextPool.Shared.Return(context);

    //Assert
    Assert.Equal(expectedRetryCount, actualRetryCount);
    Assert.IsType<MyException>(actual.Exception);
}

Yes, it seems like we don't need to wrap the Exception explicitly into an Outcome.

But let's double-check this with @martintmk

[martintmk]

Unfortunately, this won't work 100%. For example this piece of code fails:

var outcome = ResiliencePipeline.Empty.ExecuteOutcomeAsync<string, string>(
    (_, _) => throw new InvalidOperationException(),
    ResilienceContextPool.Shared.Get(),
    "dummy");

To fix this, try catch needs to be added here : https://github.com/App-vNext/Polly/blob/f99a4cea335ab7891ea6b56284b1d8e47f8e63ab/src/Polly.Core/ResiliencePipeline.AsyncT.cs#L32

Basically, we need to define a static lambda and call callback inside it with try/catch block. The reason why I haven't done this is tht async machinery adds constant overhead (around 40ns) on my machine and since this was intended to be high-performance API I thought it's not necessary there.

@peter-csala Your example works by luck because the retry strategy does exception handling inside. if you added other strategy (custom for example) the test would fail.

[peter-csala]

@peter-csala Your example works by luck because the retry strategy does exception handling inside. if you added other strategy (custom for example) the test would fail.

I did not know that. Thanks for calling it out!

[troygould-stack]

Ok, while it works by luck, we don't want to depend on the fact that its works by luck. Someone else might look at this, and do the same with a different strategy, and it doesn't work the same.

Based on the discussion, it feels like it's not a great idea to try to catch exceptions inside of the callback, and throw them as our own custom exception. Keep it simple inside the Polly pipeline? Maybe this is just a personal preference, and not really an anti-pattern.

Not so good?

            var outcome = await pipeline.ExecuteOutcomeAsync(static async (_, state) =>
            {
                try
                {
                    var httpResponseMessage = await state.HttpClient.PostAsJsonAsync(state.Path, state.Content);
                    return Outcome.FromResult(httpResponseMessage);
                }
                catch (Exception e)
                {
                    return Outcome.FromException<HttpResponseMessage>(new MyApplicationCustomException("something bad happened", e));
                }

            }, context, new { HttpClient = httpClient, Content = content, Path = purgePath });

This will interfere with the ShouldHandle method without looking at the InnerException of Outcome.Exception.

Better?

            var outcome = await pipeline.ExecuteOutcomeAsync(static async (_, state) =>
            {
                try
                {
                    var httpResponseMessage = await state.HttpClient.PostAsJsonAsync(state.Path, state.Content);
                    return Outcome.FromResult(httpResponseMessage);
                }
                catch (Exception e)
                {
                    return Outcome.FromException<HttpResponseMessage>(e);
                }

            }, context, new { HttpClient = httpClient, Content = content, Path = purgePath });

            // Deal with returning a custom application exception here instead.

[martintmk]

We can handle this on Polly level, the question is whether we want to take a perf hit @martincostello, @peter-csala ?

[martincostello]

Is there a way to make this opt-in somehow?

I think we'd only want to "spend" some of our theoretical performance budget on making the change for all usage if this is proving a common stumbling block for people.

[peter-csala]

I think we can agree on that the current situation is sub-optimal. Asking the caller via a documentation comment to wrap the exceptions into Outcome objects is not ideal.

Even though the use of ExecuteOutcomeAsync is considered as advanced scenario I still think that every public API should be easy to use, safe and then performant. Because C# allows you to simply throw the exception rather than requiring to wrap it into an Outcome object I think it is the library's responsibility to handle this case gracefully as well.

opt-in vs opt-out

I would personally vote for opt-out instead of opt-in. The default behavior should work with throw statements as well. But when all nanoseconds and bytes matter then you can turn off the "safe guard" and let it run without the try-catch.

[troygould-stack]

As the author of this question, I like that you are putting yourselves in the user's shoes. Thanks for thinking this through. IMO, I think it should work the same for all strategies if possible. Adding some user documentation around opt-in and opt-out is a great idea to add clarity to the situation as well.

[github-actions[bot]]

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[bot]]

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

[ben-page-csat]

I think this should be reopened.