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.

Sign up for GitHub

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

Jump to bottom

Drop DelegatingResilienceStrategy #1063

Merged
merged 3 commits into from
Mar 20, 2023
Merged

Drop DelegatingResilienceStrategy #1063

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2333fe9
Drop DelegatingResilienceStrategy
martintmk Mar 18, 2023
2ccde13
Cleanup
martintmk Mar 20, 2023
48afa85
PR comments
martintmk Mar 20, 2023
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
9 changes: 2 additions & 7 deletions src/Polly.Core.Benchmarks/Benchmarks/PipelineBenchmark.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,12 @@ public class PipelineBenchmark
{
private object? _strategyV7;
private object? _strategyV8;
private object? _strategyV8Delegating;

[GlobalSetup]
public void Setup()
{
_strategyV7 = Helper.CreatePipeline(PollyVersion.V7, Components, delegating: false);
_strategyV8 = Helper.CreatePipeline(PollyVersion.V8, Components, delegating: false);
_strategyV8Delegating = Helper.CreatePipeline(PollyVersion.V8, Components, delegating: true);
_strategyV7 = Helper.CreatePipeline(PollyVersion.V7, Components);
_strategyV8 = Helper.CreatePipeline(PollyVersion.V8, Components);
}

[Params(1, 2, 5, 10)]
Expand All @@ -25,7 +23,4 @@ public void Setup()

[Benchmark]
public ValueTask ExecutePipeline_V8() => _strategyV8!.ExecuteAsync(PollyVersion.V8);

[Benchmark]
public ValueTask ExecutePipeline_V8Delegating() => _strategyV8Delegating!.ExecuteAsync(PollyVersion.V8);
}
9 changes: 0 additions & 9 deletions src/Polly.Core.Benchmarks/Internals/EmptyDelegatingResilienceStrategy.cs
View file
Open in desktop

This file was deleted.

4 changes: 2 additions & 2 deletions src/Polly.Core.Benchmarks/Internals/Helper.Pipeline.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,15 @@ namespace Polly.Core.Benchmarks;

internal static partial class Helper
{
public static object CreatePipeline(PollyVersion technology, int count, bool delegating) => technology switch
public static object CreatePipeline(PollyVersion technology, int count) => technology switch
{
PollyVersion.V7 => count == 1 ? Policy.NoOpAsync<int>() : Policy.WrapAsync(Enumerable.Repeat(0, count).Select(_ => Policy.NoOpAsync<int>()).ToArray()),

PollyVersion.V8 => CreateStrategy(builder =>
{
for (var i = 0; i < count; i++)
{
builder.AddStrategy(delegating ? new EmptyDelegatingResilienceStrategy() : new EmptyResilienceStrategy());
builder.AddStrategy(new EmptyResilienceStrategy());
}
}),
_ => throw new NotSupportedException()
Expand Down
30 changes: 13 additions & 17 deletions src/Polly.Core.Benchmarks/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,20 +12,16 @@ LaunchCount=2 WarmupCount=10

## PIPELINES

| Method | Components | Mean | Error | StdDev | Median | Ratio | RatioSD | Gen0 | Allocated | Alloc Ratio |
|----------------------------- |----------- |------------:|----------:|----------:|------------:|------:|--------:|-------:|----------:|------------:|
| ExecutePipeline_V7 | 1 | 74.84 ns | 1.279 ns | 1.835 ns | 75.81 ns | 1.00 | 0.00 | 0.0362 | 304 B | 1.00 |
| ExecutePipeline_V8 | 1 | 72.61 ns | 0.584 ns | 0.819 ns | 72.28 ns | 0.97 | 0.02 | 0.0048 | 40 B | 0.13 |
| ExecutePipeline_V8Delegating | 1 | 90.04 ns | 0.753 ns | 1.104 ns | 89.76 ns | 1.20 | 0.02 | 0.0048 | 40 B | 0.13 |
| | | | | | | | | | | |
| ExecutePipeline_V7 | 2 | 156.67 ns | 2.810 ns | 4.119 ns | 154.12 ns | 1.00 | 0.00 | 0.0658 | 552 B | 1.00 |
| ExecutePipeline_V8 | 2 | 185.87 ns | 1.932 ns | 2.580 ns | 185.02 ns | 1.19 | 0.04 | 0.0048 | 40 B | 0.07 |
| ExecutePipeline_V8Delegating | 2 | 127.92 ns | 1.264 ns | 1.772 ns | 129.00 ns | 0.82 | 0.02 | 0.0048 | 40 B | 0.07 |
| | | | | | | | | | | |
| ExecutePipeline_V7 | 5 | 543.85 ns | 19.540 ns | 29.247 ns | 530.00 ns | 1.00 | 0.00 | 0.1545 | 1296 B | 1.00 |
| ExecutePipeline_V8 | 5 | 335.67 ns | 3.903 ns | 5.598 ns | 336.22 ns | 0.62 | 0.04 | 0.0048 | 40 B | 0.03 |
| ExecutePipeline_V8Delegating | 5 | 189.99 ns | 4.045 ns | 5.929 ns | 189.05 ns | 0.35 | 0.02 | 0.0048 | 40 B | 0.03 |
| | | | | | | | | | | |
| ExecutePipeline_V7 | 10 | 1,112.84 ns | 8.585 ns | 12.036 ns | 1,111.96 ns | 1.00 | 0.00 | 0.3014 | 2536 B | 1.00 |
| ExecutePipeline_V8 | 10 | 543.85 ns | 6.205 ns | 9.095 ns | 545.30 ns | 0.49 | 0.01 | 0.0048 | 40 B | 0.02 |
| ExecutePipeline_V8Delegating | 10 | 258.11 ns | 0.881 ns | 1.318 ns | 258.30 ns | 0.23 | 0.00 | 0.0048 | 40 B | 0.02 |
| Method | Components | Mean | Error | StdDev | Ratio | RatioSD | Gen0 | Allocated | Alloc Ratio |
|------------------- |----------- |------------:|----------:|----------:|------:|--------:|-------:|----------:|------------:|
| ExecutePipeline_V7 | 1 | 81.25 ns | 1.304 ns | 1.870 ns | 1.00 | 0.00 | 0.0362 | 304 B | 1.00 |
| ExecutePipeline_V8 | 1 | 82.25 ns | 1.414 ns | 2.073 ns | 1.01 | 0.04 | 0.0048 | 40 B | 0.13 |
| | | | | | | | | | |
| ExecutePipeline_V7 | 2 | 166.04 ns | 2.875 ns | 4.215 ns | 1.00 | 0.00 | 0.0658 | 552 B | 1.00 |
| ExecutePipeline_V8 | 2 | 108.29 ns | 1.504 ns | 2.251 ns | 0.65 | 0.02 | 0.0048 | 40 B | 0.07 |
| | | | | | | | | | |
| ExecutePipeline_V7 | 5 | 531.04 ns | 4.728 ns | 6.930 ns | 1.00 | 0.00 | 0.1545 | 1296 B | 1.00 |
| ExecutePipeline_V8 | 5 | 245.50 ns | 2.344 ns | 3.509 ns | 0.46 | 0.01 | 0.0048 | 40 B | 0.03 |
| | | | | | | | | | |
| ExecutePipeline_V7 | 10 | 1,128.82 ns | 10.838 ns | 15.886 ns | 1.00 | 0.00 | 0.3014 | 2536 B | 1.00 |
| ExecutePipeline_V8 | 10 | 449.31 ns | 2.926 ns | 4.379 ns | 0.40 | 0.01 | 0.0048 | 40 B | 0.02 |
51 changes: 19 additions & 32 deletions src/Polly.Core.Tests/Builder/ResilienceStrategyPipelineTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,52 +9,40 @@ namespace Polly.Core.Tests.Builder;
public class ResilienceStrategyPipelineTests
{
[Fact]
public void CreateAndFreezeStrategies_ArgValidation()
public void CreatePipeline_ArgValidation()
{
Assert.Throws<ArgumentNullException>(() => ResilienceStrategyPipeline.CreatePipelineAndFreezeStrategies(null!));
Assert.Throws<InvalidOperationException>(() => ResilienceStrategyPipeline.CreatePipelineAndFreezeStrategies(Array.Empty<ResilienceStrategy>()));
Assert.Throws<InvalidOperationException>(() => ResilienceStrategyPipeline.CreatePipelineAndFreezeStrategies(new ResilienceStrategy[] { new TestResilienceStrategy() }));
Assert.Throws<InvalidOperationException>(() => ResilienceStrategyPipeline.CreatePipelineAndFreezeStrategies(new ResilienceStrategy[]
Assert.Throws<ArgumentNullException>(() => ResilienceStrategyPipeline.CreatePipeline(null!));
Assert.Throws<InvalidOperationException>(() => ResilienceStrategyPipeline.CreatePipeline(Array.Empty<ResilienceStrategy>()));
Assert.Throws<InvalidOperationException>(() => ResilienceStrategyPipeline.CreatePipeline(new ResilienceStrategy[] { new TestResilienceStrategy() }));
Assert.Throws<InvalidOperationException>(() => ResilienceStrategyPipeline.CreatePipeline(new ResilienceStrategy[]
{
NullResilienceStrategy.Instance,
NullResilienceStrategy.Instance
}));
}

[Fact]
public void CreateAndFreezeStrategies_EnsureStrategiesLinked()
public void CreatePipeline_EnsureOriginalStrategiesPreserved()
{
var s1 = new TestResilienceStrategy();
var s2 = new TestResilienceStrategy();
var s3 = new TestResilienceStrategy();

var pipeline = ResilienceStrategyPipeline.CreatePipelineAndFreezeStrategies(new[] { s1, s2, s3 });

s1.Next.Should().Be(s2);
s2.Next.Should().Be(s3);
s3.Next.Should().Be(NullResilienceStrategy.Instance);
}

[Fact]
public void Create_EnsureStrategiesFrozen()
{
var strategies = new[]
var strategies = new ResilienceStrategy[]
{
new TestResilienceStrategy(),
new TestResilienceStrategy(),
new Strategy(),
new TestResilienceStrategy(),
};

var pipeline = ResilienceStrategyPipeline.CreatePipelineAndFreezeStrategies(strategies);
var pipeline = ResilienceStrategyPipeline.CreatePipeline(strategies);

foreach (var s in strategies)
for (int i = 0; i < strategies.Length; i++)
{
Assert.Throws<InvalidOperationException>(() => s.Next = NullResilienceStrategy.Instance);
pipeline.Strategies[i].Should().BeSameAs(strategies[i]);
}

pipeline.Strategies.SequenceEqual(strategies).Should().BeTrue();
}

[Fact]
public void Create_EnsureOriginalStrategiesPreserved()
public void CreatePipeline_EnsurePipelineReusableAcrossDifferentPipelines()
{
var strategies = new ResilienceStrategy[]
{
Expand All @@ -63,14 +51,13 @@ public void Create_EnsureOriginalStrategiesPreserved()
new TestResilienceStrategy(),
};

var pipeline = ResilienceStrategyPipeline.CreatePipelineAndFreezeStrategies(strategies);
var pipeline = ResilienceStrategyPipeline.CreatePipeline(strategies);

for (int i = 0; i < strategies.Length; i++)
{
pipeline.Strategies[i].Should().BeSameAs(strategies[i]);
}
ResilienceStrategyPipeline.CreatePipeline(new ResilienceStrategy[] { NullResilienceStrategy.Instance, pipeline });

pipeline.Strategies.SequenceEqual(strategies).Should().BeTrue();
this.Invoking(_ => ResilienceStrategyPipeline.CreatePipeline(new ResilienceStrategy[] { NullResilienceStrategy.Instance, pipeline }))
.Should()
.NotThrow();
}

private class Strategy : ResilienceStrategy
Expand Down
66 changes: 0 additions & 66 deletions src/Polly.Core.Tests/DelegatingResilienceStrategyTests.cs
View file
Open in desktop

This file was deleted.

4 changes: 2 additions & 2 deletions src/Polly.Core.Tests/Utils/TestResilienceStrategy.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
namespace Polly.Core.Tests.Utils;

public class TestResilienceStrategy : DelegatingResilienceStrategy
public class TestResilienceStrategy : ResilienceStrategy
{
public Action<ResilienceContext, object?>? Before { get; set; }

Expand All @@ -19,7 +19,7 @@ protected internal override async ValueTask<TResult> ExecuteCoreAsync<TResult, T
await OnExecute(context, state);
}

var result = await base.ExecuteCoreAsync(callback, context, state);
var result = await callback(context, state);

After?.Invoke(result, null);

Expand Down
2 changes: 1 addition & 1 deletion src/Polly.Core/Builder/ResilienceStrategyBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ public ResilienceStrategy Build()

var strategies = _entries.Select(CreateResilienceStrategy).ToList();

return ResilienceStrategyPipeline.CreatePipelineAndFreezeStrategies(strategies);
return ResilienceStrategyPipeline.CreatePipeline(strategies);
}

private ResilienceStrategy CreateResilienceStrategy(Entry entry)
Expand Down
56 changes: 25 additions & 31 deletions src/Polly.Core/Builder/ResilienceStrategyPipeline.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,55 +1,50 @@
using System;
using static System.Net.Mime.MediaTypeNames;

namespace Polly.Builder;

#pragma warning disable S2302 // "nameof" should be used

/// <summary>
/// A pipeline of strategies.
/// </summary>
internal sealed class ResilienceStrategyPipeline : DelegatingResilienceStrategy
internal sealed class ResilienceStrategyPipeline : ResilienceStrategy
{
private readonly ResilienceStrategy _pipeline;

public static ResilienceStrategyPipeline CreatePipelineAndFreezeStrategies(IReadOnlyList<ResilienceStrategy> strategies)
public static ResilienceStrategyPipeline CreatePipeline(IReadOnlyList<ResilienceStrategy> strategies)
{
Guard.NotNull(strategies);

if (strategies.Count < 2)
{
#pragma warning disable S2302 // "nameof" should be used
throw new InvalidOperationException("The resilience pipeline must contain at least two resilience strategies.");
#pragma warning restore S2302 // "nameof" should be used
}

if (strategies.Distinct().Count() != strategies.Count)
{
#pragma warning disable S2302 // "nameof" should be used
throw new InvalidOperationException("The resilience pipeline must contain unique resilience strategies.");
#pragma warning restore S2302 // "nameof" should be used
}

var delegatingStrategies = strategies.Select(strategy =>
{
if (strategy is DelegatingResilienceStrategy delegatingStrategy)
{
return delegatingStrategy;
}
else
{
return new DelegatingStrategyWrapper(strategy);
}
}).ToList();
// convert all strategies to delegating ones (except the last one as it's not required)
var delegatingStrategies = strategies
.Take(strategies.Count - 1)
.Select(strategy => new DelegatingResilienceStrategy(strategy))
.ToList();

#if NET6_0_OR_GREATER
// link the last one
delegatingStrategies[^1].Next = strategies[^1];
#else
delegatingStrategies[delegatingStrategies.Count - 1].Next = strategies[strategies.Count - 1];
#endif

// link the remaining ones
for (var i = 0; i < delegatingStrategies.Count - 1; i++)
{
delegatingStrategies[i].Next = delegatingStrategies[i + 1];
}

// now, freeze the strategies so any further modifications are not allowed
foreach (var strategy in delegatingStrategies)
{
strategy.Freeze();
}

return new ResilienceStrategyPipeline(delegatingStrategies[0], strategies);
}

Expand All @@ -63,25 +58,24 @@ private ResilienceStrategyPipeline(ResilienceStrategy pipeline, IReadOnlyList<Re

protected internal override ValueTask<TResult> ExecuteCoreAsync<TResult, TState>(Func<ResilienceContext, TState, ValueTask<TResult>> callback, ResilienceContext context, TState state)
{
return _pipeline.ExecuteCoreAsync(
static (context, state) => state.Next.ExecuteCoreAsync(state.callback, context, state.state),
context,
(Next, callback, state));
return _pipeline.ExecuteCoreAsync(callback, context, state);
}

/// <summary>
/// A wrapper that converts a <see cref="ResilienceStrategy"/> into a <see cref="DelegatingResilienceStrategy"/>.
/// A resilience strategy that delegates the execution to the next strategy in the chain.
/// </summary>
private sealed class DelegatingStrategyWrapper : DelegatingResilienceStrategy
private sealed class DelegatingResilienceStrategy : ResilienceStrategy
{
private readonly ResilienceStrategy _strategy;

public DelegatingStrategyWrapper(ResilienceStrategy strategy) => _strategy = strategy;
public DelegatingResilienceStrategy(ResilienceStrategy strategy) => _strategy = strategy;

public ResilienceStrategy? Next { get; set; }

protected internal override ValueTask<TResult> ExecuteCoreAsync<TResult, TState>(Func<ResilienceContext, TState, ValueTask<TResult>> callback, ResilienceContext context, TState state)
{
return _strategy.ExecuteCoreAsync(
static (context, state) => state.Next.ExecuteCoreAsync(state.callback, context, state.state),
static (context, state) => state.Next!.ExecuteCoreAsync(state.callback, context, state.state),
context,
(Next, callback, state));
}
Expand Down
Loading