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

Cleanup OutcomeResilienceStrategy #1430

Merged
merged 2 commits into from
Jul 26, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,7 @@ public CircuitBreakerResilienceStrategy(
Func<OutcomeArguments<T, CircuitBreakerPredicateArguments>, ValueTask<bool>> handler,
CircuitStateController<T> controller,
CircuitBreakerStateProvider? stateProvider,
CircuitBreakerManualControl? manualControl,
bool isGeneric)
: base(isGeneric)
CircuitBreakerManualControl? manualControl)
{
_handler = handler;
_controller = controller;
Expand All @@ -23,7 +21,7 @@ public CircuitBreakerResilienceStrategy(
_controller.Dispose);
}

protected override async ValueTask<Outcome<T>> ExecuteCallbackAsync<TState>(Func<ResilienceContext, TState, ValueTask<Outcome<T>>> callback, ResilienceContext context, TState state)
protected override async ValueTask<Outcome<T>> ExecuteCore<TState>(Func<ResilienceContext, TState, ValueTask<Outcome<T>>> callback, ResilienceContext context, TState state)
{
if (await _controller.OnActionPreExecuteAsync(context).ConfigureAwait(context.ContinueOnCapturedContext) is Outcome<T> outcome)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -156,8 +156,7 @@ private static TBuilder AddSimpleCircuitBreakerCore<TBuilder, TResult>(this TBui
options.ShouldHandle!,
controller,
options.StateProvider,
options.ManualControl,
context.IsGenericBuilder);
options.ManualControl);
}
}

5 changes: 2 additions & 3 deletions src/Polly.Core/Fallback/FallbackResilienceStrategy.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,14 @@ internal sealed class FallbackResilienceStrategy<T> : OutcomeResilienceStrategy<
private readonly Func<OutcomeArguments<T, OnFallbackArguments>, ValueTask>? _onFallback;
private readonly ResilienceStrategyTelemetry _telemetry;

public FallbackResilienceStrategy(FallbackHandler<T> handler, Func<OutcomeArguments<T, OnFallbackArguments>, ValueTask>? onFallback, ResilienceStrategyTelemetry telemetry, bool isGeneric)
: base(isGeneric)
public FallbackResilienceStrategy(FallbackHandler<T> handler, Func<OutcomeArguments<T, OnFallbackArguments>, ValueTask>? onFallback, ResilienceStrategyTelemetry telemetry)
{
_handler = handler;
_onFallback = onFallback;
_telemetry = telemetry;
}

protected override async ValueTask<Outcome<T>> ExecuteCallbackAsync<TState>(Func<ResilienceContext, TState, ValueTask<Outcome<T>>> callback, ResilienceContext context, TState state)
protected override async ValueTask<Outcome<T>> ExecuteCore<TState>(Func<ResilienceContext, TState, ValueTask<Outcome<T>>> callback, ResilienceContext context, TState state)
{
var outcome = await ExecuteCallbackSafeAsync(callback, context, state).ConfigureAwait(context.ContinueOnCapturedContext);
var handleFallbackArgs = new OutcomeArguments<T, FallbackPredicateArguments>(context, outcome, default);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,7 @@ internal static ResilienceStrategyBuilder AddFallback(this ResilienceStrategyBui
return new FallbackResilienceStrategy<TResult>(
handler,
options.OnFallback,
context.Telemetry,
context.IsGenericBuilder);
context.Telemetry);
},
options);
}
Expand Down
6 changes: 2 additions & 4 deletions src/Polly.Core/Hedging/HedgingResilienceStrategy.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,7 @@ public HedgingResilienceStrategy(
Func<OutcomeArguments<T, OnHedgingArguments>, ValueTask>? onHedging,
Func<HedgingDelayArguments, ValueTask<TimeSpan>>? hedgingDelayGenerator,
TimeProvider timeProvider,
ResilienceStrategyTelemetry telemetry,
bool isGeneric)
: base(isGeneric)
ResilienceStrategyTelemetry telemetry)
{
HedgingDelay = hedgingDelay;
MaxHedgedAttempts = maxHedgedAttempts;
Expand All @@ -43,7 +41,7 @@ public HedgingResilienceStrategy(
public Func<OutcomeArguments<T, OnHedgingArguments>, ValueTask>? OnHedging { get; }

[ExcludeFromCodeCoverage] // coverlet issue
protected override async ValueTask<Outcome<T>> ExecuteCallbackAsync<TState>(
protected override async ValueTask<Outcome<T>> ExecuteCore<TState>(
Func<ResilienceContext, TState, ValueTask<Outcome<T>>> callback,
ResilienceContext context,
TState state)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,7 @@ internal static ResilienceStrategyBuilder AddHedging(this ResilienceStrategyBuil
options.OnHedging,
options.HedgingDelayGenerator,
context.TimeProvider,
context.Telemetry,
context.IsGenericBuilder);
context.Telemetry);
},
options);
}
Expand Down
4 changes: 1 addition & 3 deletions src/Polly.Core/Retry/RetryResilienceStrategy.cs
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,9 @@ internal sealed class RetryResilienceStrategy<T> : OutcomeResilienceStrategy<T>

public RetryResilienceStrategy(
RetryStrategyOptions<T> options,
bool isGeneric,
TimeProvider timeProvider,
ResilienceStrategyTelemetry telemetry,
Func<double> randomizer)
: base(isGeneric)
{
ShouldHandle = options.ShouldHandle;
BaseDelay = options.BaseDelay;
Expand All @@ -41,7 +39,7 @@ public RetryResilienceStrategy(

public Func<OutcomeArguments<T, OnRetryArguments>, ValueTask>? OnRetry { get; }

protected override async ValueTask<Outcome<T>> ExecuteCallbackAsync<TState>(Func<ResilienceContext, TState, ValueTask<Outcome<T>>> callback, ResilienceContext context, TState state)
protected override async ValueTask<Outcome<T>> ExecuteCore<TState>(Func<ResilienceContext, TState, ValueTask<Outcome<T>>> callback, ResilienceContext context, TState state)
{
double retryState = 0;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ public static ResilienceStrategyBuilder<TResult> AddRetry<TResult>(this Resilien
builder.AddStrategy(context =>
new RetryResilienceStrategy<TResult>(
options,
context.IsGenericBuilder,
context.TimeProvider,
context.Telemetry,
context.Randomizer),
Expand Down
48 changes: 7 additions & 41 deletions src/Polly.Core/Utils/OutcomeResilienceStrategy.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10,71 +10,37 @@
/// </remarks>
internal abstract class OutcomeResilienceStrategy<T> : ResilienceStrategy
{
private readonly bool _isGeneric;

protected OutcomeResilienceStrategy(bool isGeneric)
{
if (!isGeneric && typeof(T) != typeof(object))
{
throw new NotSupportedException("For non-generic strategies the generic parameter should be of type 'object'.");
}

_isGeneric = isGeneric;
}

protected internal sealed override ValueTask<Outcome<TResult>> ExecuteCore<TResult, TState>(
Func<ResilienceContext, TState, ValueTask<Outcome<TResult>>> callback,
ResilienceContext context,
TState state)
{
if (_isGeneric)
// Check if we can cast directly, thus saving some cycles and improving the performance
if (context.ResultType == typeof(T))
{
if (typeof(TResult) != typeof(T))
{
return callback(context, state);
}

// cast is safe here, because TResult and T are the same type
var callbackCasted = (Func<ResilienceContext, TState, ValueTask<Outcome<T>>>)(object)callback;
var valueTask = ExecuteCallbackAsync(callbackCasted, context, state);
var valueTask = ExecuteCore(callbackCasted, context, state);

return ConvertValueTask<TResult>(valueTask, context);
return TaskHelper.ConvertValueTask<T, TResult>(valueTask, context);
}
else
{
var valueTask = ExecuteCallbackAsync(
var valueTask = ExecuteCore(
static async (context, state) =>
{
var outcome = await state.callback(context, state.state).ConfigureAwait(context.ContinueOnCapturedContext);

// cast the outcome to "object" based on (T)
return outcome.AsOutcome<T>();
},
context,
(callback, state));

return ConvertValueTask<TResult>(valueTask, context);
return TaskHelper.ConvertValueTask<T, TResult>(valueTask, context);
}
}

protected abstract ValueTask<Outcome<T>> ExecuteCallbackAsync<TState>(
protected abstract ValueTask<Outcome<T>> ExecuteCore<TState>(
Func<ResilienceContext, TState, ValueTask<Outcome<T>>> callback,
ResilienceContext context,
TState state);

private static ValueTask<Outcome<TResult>> ConvertValueTask<TResult>(ValueTask<Outcome<T>> valueTask, ResilienceContext resilienceContext)
{
if (valueTask.IsCompletedSuccessfully)
{
return new ValueTask<Outcome<TResult>>(valueTask.Result.AsOutcome<TResult>());
}

return ConvertValueTaskAsync(valueTask, resilienceContext);

static async ValueTask<Outcome<TResult>> ConvertValueTaskAsync(ValueTask<Outcome<T>> valueTask, ResilienceContext resilienceContext)
{
var outcome = await valueTask.ConfigureAwait(resilienceContext.ContinueOnCapturedContext);
return outcome.AsOutcome<TResult>();
}
}
}
22 changes: 22 additions & 0 deletions src/Polly.Core/Utils/TaskHelper.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
using System.Threading.Tasks;

namespace Polly.Utils;

internal static class TaskHelper
{
public static ValueTask<Outcome<TTo>> ConvertValueTask<TFrom, TTo>(ValueTask<Outcome<TFrom>> valueTask, ResilienceContext resilienceContext)
{
if (valueTask.IsCompletedSuccessfully)
{
return new ValueTask<Outcome<TTo>>(valueTask.Result.AsOutcome<TTo>());
}

return ConvertValueTaskAsync(valueTask, resilienceContext);

static async ValueTask<Outcome<TTo>> ConvertValueTaskAsync(ValueTask<Outcome<TFrom>> valueTask, ResilienceContext resilienceContext)
{
var outcome = await valueTask.ConfigureAwait(resilienceContext.ContinueOnCapturedContext);
return outcome.AsOutcome<TTo>();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -127,11 +127,8 @@ public void Execute_Ok()
_options.ShouldHandle = _ => PredicateResult.False;
_behavior.Setup(v => v.OnActionSuccess(CircuitState.Closed));

Create().Invoking(s => s.Execute(_ => { })).Should().NotThrow();
Create().Invoking(s => s.Execute(_ => 0)).Should().NotThrow();
}

private CircuitBreakerResilienceStrategy<int> Create()
{
return new(_options.ShouldHandle!, _controller, _options.StateProvider, _options.ManualControl, true);
}
private CircuitBreakerResilienceStrategy<int> Create() => new(_options.ShouldHandle!, _controller, _options.StateProvider, _options.ManualControl);
}
15 changes: 2 additions & 13 deletions test/Polly.Core.Tests/Fallback/FallbackResilienceStrategyTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,6 @@ public void Ctor_Ok()
Create().Should().NotBeNull();
}

[Fact]
public void DoesntHandle_Skips()
{
SetHandler(_ => true, () => Outcome.FromResult("dummy"), true);

Create().Execute(() => -1).Should().Be(-1);

_args.Should().BeEmpty();
}

[Fact]
public void Handle_Result_Ok()
{
Expand Down Expand Up @@ -70,7 +60,7 @@ public void Handle_UnhandledException_Ok()
_options.OnFallback = _ => { called = true; return default; };
SetHandler(outcome => outcome.Exception is InvalidOperationException, () => { fallbackActionCalled = true; return Outcome.FromResult("secondary"); });

Create().Invoking(s => s.Execute<int>(_ => throw new ArgumentException())).Should().Throw<ArgumentException>();
Create().Invoking(s => s.Execute<string>(_ => throw new ArgumentException())).Should().Throw<ArgumentException>();

_args.Should().BeEmpty();
called.Should().BeFalse();
Expand Down Expand Up @@ -103,6 +93,5 @@ private void SetHandler(
private FallbackResilienceStrategy<string> Create() => new(
_handler!,
_options.OnFallback,
_telemetry,
true);
_telemetry);
}
12 changes: 1 addition & 11 deletions test/Polly.Core.Tests/Hedging/HedgingResilienceStrategyTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -54,15 +54,6 @@ public void Ctor_EnsureDefaults()
strategy.HedgingHandler.Should().NotBeNull();
}

[Fact]
public void Execute_Skipped_Ok()
{
ConfigureHedging();
var strategy = Create();

strategy.Execute(_ => 10).Should().Be(10);
}

[Fact]
public async Task Execute_CancellationRequested_Throws()
{
Expand Down Expand Up @@ -973,6 +964,5 @@ private HedgingResilienceStrategy<T> Create<T>(
onHedging,
_options.HedgingDelayGenerator,
_timeProvider,
_telemetry,
true);
_telemetry);
}
Original file line number Diff line number Diff line change
Expand Up @@ -352,7 +352,6 @@ private async ValueTask<int> ExecuteAndAdvance(RetryResilienceStrategy<object> s

private RetryResilienceStrategy<object> CreateSut(TimeProvider? timeProvider = null) =>
new(_options,
false,
timeProvider ?? _timeProvider,
_telemetry,
() => 1.0);
Expand Down
40 changes: 20 additions & 20 deletions test/Polly.Core.Tests/Utils/OutcomeResilienceStrategyTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,7 @@ public class OutcomeResilienceStrategyTests
[Fact]
public void Ctor_Ok()
{
new Strategy<string>(args => { }, true).Should().NotBeNull();
new Strategy<object>(args => { }, false).Should().NotBeNull();
new Strategy<object>(args => { }, true).Should().NotBeNull();
}

[Fact]
public void Ctor_InvalidArgs_Throws()
{
this.Invoking(_ => new Strategy<string>(_ => { }, false)).Should().Throw<NotSupportedException>();
new Strategy<string>(args => { }).Should().NotBeNull();
}

[Fact]
Expand All @@ -26,8 +18,7 @@ public void Execute_NonGeneric_Ok()
var strategy = new Strategy<object>(outcome =>
{
values.Add(outcome.Result);
},
false);
});

strategy.Execute(args => "dummy");
strategy.Execute(args => 0);
Expand All @@ -48,31 +39,40 @@ public void Execute_Generic_Ok()
var strategy = new Strategy<string>(outcome =>
{
values.Add(outcome.Result);
},
true);
});

strategy.Execute(args => "dummy");
strategy.Execute(args => 0);
strategy.Execute<object?>(args => null);
strategy.Execute(args => true);

values.Should().HaveCount(1);
values[0].Should().Be("dummy");
}

[Fact]
public void Pipeline_TypeCheck_Ok()
{
var called = false;
var strategy = new Strategy<object>(o =>
{
o.Result.Should().Be(-1);
called = true;
});

strategy.Execute(() => -1);

called.Should().BeTrue();
}

private class Strategy<T> : OutcomeResilienceStrategy<T>
{
private readonly Action<Outcome<T>> _onOutcome;

public Strategy(Action<Outcome<T>> onOutcome, bool isGeneric)
: base(isGeneric) => _onOutcome = onOutcome;
public Strategy(Action<Outcome<T>> onOutcome) => _onOutcome = onOutcome;

protected override async ValueTask<Outcome<T>> ExecuteCallbackAsync<TState>(Func<ResilienceContext, TState, ValueTask<Outcome<T>>> callback, ResilienceContext context, TState state)
protected override async ValueTask<Outcome<T>> ExecuteCore<TState>(Func<ResilienceContext, TState, ValueTask<Outcome<T>>> callback, ResilienceContext context, TState state)
{
var outcome = await callback(context, state);
_onOutcome(outcome);
return outcome;
}
}

}