Chaos Outcome - Exception handling

[peter-csala]

Based on the discussion what we have started in the #2071 we have an agreement that the Outcome chaos strategy does not throw an exception rather returns the Result of the Outcome.

var pipeline = new ResiliencePipelineBuilder<int>()
.AddRetry(new RetryStrategyOptions<int>
{
	ShouldHandle = new PredicateBuilder<int>().Handle<CustomException>(),
	MaxRetryAttempts = 2,
	OnRetry = static args =>
	{
		Console.WriteLine("OnRetry");
		return default;
	}
})
.AddChaosOutcome(new ChaosOutcomeStrategyOptions<int>()
{
	InjectionRate = 1,
	Enabled = true,
	OutcomeGenerator = static args =>
    {
		//throw new Exception("test");
		return ValueTask.FromResult<Outcome<int>?>(Outcome.FromException<int>(new CustomException()));
    },
	//OutcomeGenerator =  new OutcomeGenerator<int>().AddException<Exception>(),
	OnOutcomeInjected = static args =>
	{
		Console.WriteLine($"OnOutcomeInjected, Outcome: {args.Outcome.Result}");
		Console.WriteLine($"OnOutcomeInjected, Exception: {args.Outcome.Exception}");
		return default;
	}
})
.ConfigureTelemetry(new TelemetryOptions
{
	LoggerFactory = LoggerFactory.Create(builder => builder.AddConsole())
}) 
.Build();

var resilienceContext = ResilienceContextPool.Shared.Get();
try
{
	var result = await pipeline.ExecuteAsync<int>(ctx => { return ValueTask.FromResult<int>(1); }, resilienceContext);
	//var result = await pipeline.ExecuteAsync<int>(ctx => { return ValueTask.FromException<int>(new CustomException()); }, resilienceContext);
	result.Dump();
}
catch(Exception ex)
{
	Console.WriteLine(ex.Message);
}
ResilienceContextPool.Shared.Return(resilienceContext);

info: Polly[0]
      Resilience event occurred. EventName: 'Chaos.OnOutcome', Source: '(null)/(null)/Chaos.Outcome', Operation Key: '', Result: ''
OnOutcomeInjected, Outcome: 0
OnOutcomeInjected, Exception: CustomException: Exception of type 'CustomException' was thrown.
info: Polly[3]
      Execution attempt. Source: '(null)/(null)/Retry', Operation Key: '', Result: '0', Handled: 'False', Attempt: '0', Execution Time: '23.4007'
Dumping object(Int32)
 0

Dotnet fiddle: https://dotnetfiddle.net/82Chpg

cc: @vany0114 , @martincostello , @martintmk

[martintmk]

My opinion is different here. I do not see this as an problem nor anti-pattern. Chaos outcome allows returning Outcome (surprisingly :D) and that is valid for both results and exceptions.

Why is that a problem?

FaultStrategy is used for non-generic pipelines and therefore is not possible to inject results, this does not applies to generic pipelines.

[peter-csala]

My opinion is different here. I do not see this as an problem nor anti-pattern. Chaos outcome allows returning Outcome (surprisingly :D) and that is valid for both results and exceptions.

That's semi-true. You can setup an Outcome in the OutcomeGenerator but that Outcome object is only visible inside the OnOutcomeInjected delegate. If you call the ExecuteAsync on a pipeline which only consists of an Outcome chaos strategy then the Result will be used always. Regardless your Outcome contains an exception or a value.

If you have multiple strategies then yet again the other strategies will see only the Result. In the above example the ShouldHandle's args.Outcome will contain a 0 for the Result and null for the Exception.

Why is that a problem?

You can't use the Outcome strategy to throw an exception. You have to use the Fault chaos for that.

Currently the Outcome strategy either returns a specified value or default(T). IMHO if we can't use the Exception part of the Outcome structure then it does not make sense to return an Outcome ...

[vany0114]

I'm surprised that no one has reported that issue yet as it is stated in the docs and examples (since the initial release) that the Outcome strategy is also able to inject exceptions as results (it turns out it's not working 🙃), which tells me that it seems users are not really using the Outcome strategy to inject exceptions, which is a good sign either way I'd say.

With that said, I lean toward the third option: Make a clear separation between the Outcome and Fault chaos strategies and, update the docs, and validate in the Outcome strategy that T is not an Exception type?

[peter-csala]

Will you create a PR to make a clear separation between Outcome and Fault?

[vany0114]

@peter-csala feel free to do so if you want, I can also help with it but I'm afraid it will take me some time as I'm kinda busy lately.

[peter-csala]

@martintmk What do you think? Shall we fix the Outcome chaos to be able to return with Exceptions as well? Or shall we alter the Outcome strategy to return only TResult?

[martintmk]

My vote is not to do any breaking changes and rather to fix chaos outcome strategy. Internally, we have recommended it for injecting exceptions anyway if results are also injected. It's one less strategy in the pipeline so it's also slightly faster.

[peter-csala]

Okay, I will go with approach then.