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
[Question]: Usage of ExecuteOutcomeAsync #2025
Comments
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 But let's double-check this with @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 :
Basically, we need to define a static lambda and call @peter-csala |
I did not know that. Thanks for calling it out! |
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?
This will interfere with the Better?
|
We can handle this on Polly level, the question is whether we want to take a perf hit @martincostello, @peter-csala ? |
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. |
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 Even though the use of opt-in vs opt-outI would personally vote for opt-out instead of opt-in. The default behavior should work with |
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. |
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 thecallback
does not throw any exceptions. Instead, it converts them toOutcome<TResult>
."When I jump through the source code, I end up in the
RetryResilienceStrategy
which uses theStrategyHelper
. Within theStrategyHelper
, it has a try/catch around the call to the callback method. Thus, when usingExecuteOutcomeAsync
, a user isn't required to catchExceptions
and callOutcome.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
withException
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
The text was updated successfully, but these errors were encountered: