From 67b01d5972723860cc9c8fb89fa59314fadbc532 Mon Sep 17 00:00:00 2001 From: Martin Tomka Date: Wed, 23 Aug 2023 08:04:44 +0200 Subject: [PATCH 1/2] Clenaup rate limiter API --- src/Polly.RateLimiting/DisposeWrapper.cs | 14 +++ .../PublicAPI.Unshipped.txt | 11 ++- .../RateLimiterArguments.cs | 20 +++++ ...iterResiliencePipelineBuilderExtensions.cs | 16 +++- .../RateLimiterResilienceStrategy.cs | 24 +++-- .../RateLimiterStrategyOptions.cs | 2 +- .../ResilienceRateLimiter.cs | 71 --------------- ...IssuesTests.PartitionedRateLimiter_1365.cs | 2 +- ...esiliencePipelineBuilderExtensionsTests.cs | 17 ++-- .../RateLimiterResilienceStrategyTests.cs | 40 ++++++++- .../ResilienceRateLimiterTests.cs | 89 ------------------- 11 files changed, 123 insertions(+), 183 deletions(-) create mode 100644 src/Polly.RateLimiting/DisposeWrapper.cs create mode 100644 src/Polly.RateLimiting/RateLimiterArguments.cs delete mode 100644 src/Polly.RateLimiting/ResilienceRateLimiter.cs delete mode 100644 test/Polly.RateLimiting.Tests/ResilienceRateLimiterTests.cs diff --git a/src/Polly.RateLimiting/DisposeWrapper.cs b/src/Polly.RateLimiting/DisposeWrapper.cs new file mode 100644 index 00000000000..2a062747eb2 --- /dev/null +++ b/src/Polly.RateLimiting/DisposeWrapper.cs @@ -0,0 +1,14 @@ +using System.Threading.RateLimiting; + +namespace Polly.RateLimiting; + +internal sealed class DisposeWrapper : IDisposable, IAsyncDisposable +{ + internal DisposeWrapper(RateLimiter limiter) => Limiter = limiter; + + public RateLimiter Limiter { get; } + + public ValueTask DisposeAsync() => Limiter.DisposeAsync(); + + public void Dispose() => Limiter.Dispose(); +} diff --git a/src/Polly.RateLimiting/PublicAPI.Unshipped.txt b/src/Polly.RateLimiting/PublicAPI.Unshipped.txt index ad59951f733..675b43b0b2e 100644 --- a/src/Polly.RateLimiting/PublicAPI.Unshipped.txt +++ b/src/Polly.RateLimiting/PublicAPI.Unshipped.txt @@ -5,6 +5,10 @@ Polly.RateLimiting.OnRateLimiterRejectedArguments.Context.get -> Polly.Resilienc Polly.RateLimiting.OnRateLimiterRejectedArguments.Lease.get -> System.Threading.RateLimiting.RateLimitLease! Polly.RateLimiting.OnRateLimiterRejectedArguments.OnRateLimiterRejectedArguments() -> void Polly.RateLimiting.OnRateLimiterRejectedArguments.OnRateLimiterRejectedArguments(Polly.ResilienceContext! context, System.Threading.RateLimiting.RateLimitLease! lease) -> void +Polly.RateLimiting.RateLimiterArguments +Polly.RateLimiting.RateLimiterArguments.Context.get -> Polly.ResilienceContext! +Polly.RateLimiting.RateLimiterArguments.RateLimiterArguments() -> void +Polly.RateLimiting.RateLimiterArguments.RateLimiterArguments(Polly.ResilienceContext! context) -> void Polly.RateLimiting.RateLimiterRejectedException Polly.RateLimiting.RateLimiterRejectedException.RateLimiterRejectedException() -> void Polly.RateLimiting.RateLimiterRejectedException.RateLimiterRejectedException(string! message) -> void @@ -18,15 +22,10 @@ Polly.RateLimiting.RateLimiterStrategyOptions.DefaultRateLimiterOptions.get -> S Polly.RateLimiting.RateLimiterStrategyOptions.DefaultRateLimiterOptions.set -> void Polly.RateLimiting.RateLimiterStrategyOptions.OnRejected.get -> System.Func? Polly.RateLimiting.RateLimiterStrategyOptions.OnRejected.set -> void -Polly.RateLimiting.RateLimiterStrategyOptions.RateLimiter.get -> Polly.RateLimiting.ResilienceRateLimiter? +Polly.RateLimiting.RateLimiterStrategyOptions.RateLimiter.get -> System.Func>? Polly.RateLimiting.RateLimiterStrategyOptions.RateLimiter.set -> void Polly.RateLimiting.RateLimiterStrategyOptions.RateLimiterStrategyOptions() -> void -Polly.RateLimiting.ResilienceRateLimiter static Polly.RateLimiterResiliencePipelineBuilderExtensions.AddConcurrencyLimiter(this TBuilder! builder, int permitLimit, int queueLimit = 0) -> TBuilder! static Polly.RateLimiterResiliencePipelineBuilderExtensions.AddConcurrencyLimiter(this TBuilder! builder, System.Threading.RateLimiting.ConcurrencyLimiterOptions! options) -> TBuilder! static Polly.RateLimiterResiliencePipelineBuilderExtensions.AddRateLimiter(this TBuilder! builder, Polly.RateLimiting.RateLimiterStrategyOptions! options) -> TBuilder! static Polly.RateLimiterResiliencePipelineBuilderExtensions.AddRateLimiter(this TBuilder! builder, System.Threading.RateLimiting.RateLimiter! limiter) -> TBuilder! -Polly.RateLimiting.ResilienceRateLimiter.Dispose() -> void -Polly.RateLimiting.ResilienceRateLimiter.DisposeAsync() -> System.Threading.Tasks.ValueTask -static Polly.RateLimiting.ResilienceRateLimiter.Create(System.Threading.RateLimiting.PartitionedRateLimiter! rateLimiter) -> Polly.RateLimiting.ResilienceRateLimiter! -static Polly.RateLimiting.ResilienceRateLimiter.Create(System.Threading.RateLimiting.RateLimiter! rateLimiter) -> Polly.RateLimiting.ResilienceRateLimiter! diff --git a/src/Polly.RateLimiting/RateLimiterArguments.cs b/src/Polly.RateLimiting/RateLimiterArguments.cs new file mode 100644 index 00000000000..cd5b98bb7ac --- /dev/null +++ b/src/Polly.RateLimiting/RateLimiterArguments.cs @@ -0,0 +1,20 @@ +namespace Polly.RateLimiting; + +#pragma warning disable CA1815 // Override equals and operator equals on value types + +/// +/// The arguments used by the delegate. +/// +public readonly struct RateLimiterArguments +{ + /// + /// Initializes a new instance of the struct. + /// + /// Context associated with the execution of a user-provided callback. + public RateLimiterArguments(ResilienceContext context) => Context = context; + + /// + /// Gets the context associated with the execution of a user-provided callback. + /// + public ResilienceContext Context { get; } +} diff --git a/src/Polly.RateLimiting/RateLimiterResiliencePipelineBuilderExtensions.cs b/src/Polly.RateLimiting/RateLimiterResiliencePipelineBuilderExtensions.cs index c9dc9aeb86c..994b81d9192 100644 --- a/src/Polly.RateLimiting/RateLimiterResiliencePipelineBuilderExtensions.cs +++ b/src/Polly.RateLimiting/RateLimiterResiliencePipelineBuilderExtensions.cs @@ -79,7 +79,7 @@ public static TBuilder AddRateLimiter( return builder.AddRateLimiter(new RateLimiterStrategyOptions { - RateLimiter = ResilienceRateLimiter.Create(limiter), + RateLimiter = args => limiter.AcquireAsync(1, args.Context.CancellationToken), }); } @@ -109,10 +109,20 @@ public static TBuilder AddRateLimiter( return builder.AddStrategy( context => { + DisposeWrapper? wrapper = default; + var limiter = options.RateLimiter; + if (limiter is null) + { + var defaultLimiter = new ConcurrencyLimiter(options.DefaultRateLimiterOptions); + wrapper = new DisposeWrapper(defaultLimiter); + limiter = args => defaultLimiter.AcquireAsync(1, args.Context.CancellationToken); + } + return new RateLimiterResilienceStrategy( - options.RateLimiter ?? ResilienceRateLimiter.Create(new ConcurrencyLimiter(options.DefaultRateLimiterOptions)), + limiter, options.OnRejected, - context.Telemetry); + context.Telemetry, + wrapper); }, options); } diff --git a/src/Polly.RateLimiting/RateLimiterResilienceStrategy.cs b/src/Polly.RateLimiting/RateLimiterResilienceStrategy.cs index 80301b9e430..ec29f4c0ee1 100644 --- a/src/Polly.RateLimiting/RateLimiterResilienceStrategy.cs +++ b/src/Polly.RateLimiting/RateLimiterResilienceStrategy.cs @@ -8,30 +8,42 @@ internal sealed class RateLimiterResilienceStrategy : ResilienceStrategy, IDispo private readonly ResilienceStrategyTelemetry _telemetry; public RateLimiterResilienceStrategy( - ResilienceRateLimiter limiter, + Func> limiter, Func? onRejected, - ResilienceStrategyTelemetry telemetry) + ResilienceStrategyTelemetry telemetry, + DisposeWrapper? wrapper) { Limiter = limiter; OnLeaseRejected = onRejected; _telemetry = telemetry; + Wrapper = wrapper; } - public ResilienceRateLimiter Limiter { get; } + public Func> Limiter { get; } public Func? OnLeaseRejected { get; } - public void Dispose() => Limiter.Dispose(); + public DisposeWrapper? Wrapper { get; } - public ValueTask DisposeAsync() => Limiter.DisposeAsync(); + public void Dispose() => Wrapper?.Dispose(); + + public ValueTask DisposeAsync() + { + if (Wrapper is not null) + { + return Wrapper.DisposeAsync(); + } + + return default; + } protected override async ValueTask> ExecuteCore( Func>> callback, ResilienceContext context, TState state) { - using var lease = await Limiter.AcquireAsync(context).ConfigureAwait(context.ContinueOnCapturedContext); + using var lease = await Limiter(new RateLimiterArguments(context)).ConfigureAwait(context.ContinueOnCapturedContext); if (lease.IsAcquired) { diff --git a/src/Polly.RateLimiting/RateLimiterStrategyOptions.cs b/src/Polly.RateLimiting/RateLimiterStrategyOptions.cs index 050b6de14c2..4cc3c793213 100644 --- a/src/Polly.RateLimiting/RateLimiterStrategyOptions.cs +++ b/src/Polly.RateLimiting/RateLimiterStrategyOptions.cs @@ -45,5 +45,5 @@ public class RateLimiterStrategyOptions : ResilienceStrategyOptions /// The default value is . If this property is , then the strategy /// will use a created using . /// - public ResilienceRateLimiter? RateLimiter { get; set; } + public Func>? RateLimiter { get; set; } } diff --git a/src/Polly.RateLimiting/ResilienceRateLimiter.cs b/src/Polly.RateLimiting/ResilienceRateLimiter.cs deleted file mode 100644 index a3f79ef2580..00000000000 --- a/src/Polly.RateLimiting/ResilienceRateLimiter.cs +++ /dev/null @@ -1,71 +0,0 @@ -using System.Threading.RateLimiting; - -namespace Polly.RateLimiting; - -/// -/// This class is just a simple adapter for the built-in limiters in the System.Threading.RateLimiting namespace. -/// -public sealed class ResilienceRateLimiter : IDisposable, IAsyncDisposable -{ - private ResilienceRateLimiter(RateLimiter? limiter, PartitionedRateLimiter? partitionedLimiter) - { - Limiter = limiter; - PartitionedLimiter = partitionedLimiter; - } - - /// - /// Creates an instance of from . - /// - /// The rate limiter instance. - /// An instance of . - public static ResilienceRateLimiter Create(RateLimiter rateLimiter) => new(Guard.NotNull(rateLimiter), null); - - /// - /// Creates an instance of from partitioned . - /// - /// The rate limiter instance. - /// An instance of . - public static ResilienceRateLimiter Create(PartitionedRateLimiter rateLimiter) => new(null, Guard.NotNull(rateLimiter)); - - internal RateLimiter? Limiter { get; } - - internal PartitionedRateLimiter? PartitionedLimiter { get; } - - internal ValueTask AcquireAsync(ResilienceContext context) - { - if (PartitionedLimiter is not null) - { - return PartitionedLimiter.AcquireAsync(context, permitCount: 1, context.CancellationToken); - } - else - { - return Limiter!.AcquireAsync(permitCount: 1, context.CancellationToken); - } - } - - /// - public ValueTask DisposeAsync() - { - if (PartitionedLimiter is not null) - { - return PartitionedLimiter.DisposeAsync(); - } - else - { - return Limiter!.DisposeAsync(); - } - } - - /// - public void Dispose() - { - if (PartitionedLimiter is not null) - { - PartitionedLimiter.Dispose(); - } - else - { - Limiter!.Dispose(); - } - } -} diff --git a/test/Polly.Extensions.Tests/Issues/IssuesTests.PartitionedRateLimiter_1365.cs b/test/Polly.Extensions.Tests/Issues/IssuesTests.PartitionedRateLimiter_1365.cs index 84248d4629f..ffc0555bbb3 100644 --- a/test/Polly.Extensions.Tests/Issues/IssuesTests.PartitionedRateLimiter_1365.cs +++ b/test/Polly.Extensions.Tests/Issues/IssuesTests.PartitionedRateLimiter_1365.cs @@ -36,7 +36,7 @@ public async void PartitionedRateLimiter_EnsureUserLimited_1365() builder.AddRateLimiter(new RateLimiterStrategyOptions { - RateLimiter = ResilienceRateLimiter.Create(partitionedLimiter) + RateLimiter = args => partitionedLimiter.AcquireAsync(args.Context, 1, args.Context.CancellationToken) }); }); diff --git a/test/Polly.RateLimiting.Tests/RateLimiterResiliencePipelineBuilderExtensionsTests.cs b/test/Polly.RateLimiting.Tests/RateLimiterResiliencePipelineBuilderExtensionsTests.cs index d359555ec44..33b53caf8b9 100644 --- a/test/Polly.RateLimiting.Tests/RateLimiterResiliencePipelineBuilderExtensionsTests.cs +++ b/test/Polly.RateLimiting.Tests/RateLimiterResiliencePipelineBuilderExtensionsTests.cs @@ -14,7 +14,7 @@ public class RateLimiterResiliencePipelineBuilderExtensionsTests builder => { builder.AddConcurrencyLimiter(2, 2); - AssertRateLimiterStrategy(builder, strategy => strategy.Limiter.Limiter.Should().BeOfType()); + AssertRateLimiterStrategy(builder, strategy => strategy.Wrapper!.Limiter.Should().BeOfType()); }, builder => { @@ -25,13 +25,20 @@ public class RateLimiterResiliencePipelineBuilderExtensionsTests QueueLimit = 2 }); - AssertRateLimiterStrategy(builder, strategy => strategy.Limiter.Limiter.Should().BeOfType()); + AssertRateLimiterStrategy(builder, strategy => strategy.Wrapper!.Limiter.Should().BeOfType()); }, builder => { var expected = Substitute.For(); builder.AddRateLimiter(expected); - AssertRateLimiterStrategy(builder, strategy => strategy.Limiter.Limiter.Should().Be(expected)); + AssertRateLimiterStrategy(builder, strategy => strategy.Wrapper.Should().BeNull()); + }, + builder => + { + var limiter = new ConcurrencyLimiter(new ConcurrencyLimiterOptions { PermitLimit = 1 }); + builder.AddRateLimiter(limiter); + builder.Build().Execute(() => { }); + AssertRateLimiterStrategy(builder, strategy => strategy.Wrapper.Should().BeNull()); } }; @@ -85,7 +92,7 @@ public void AddRateLimiter_Ok() new ResiliencePipelineBuilder() .AddRateLimiter(new RateLimiterStrategyOptions { - RateLimiter = ResilienceRateLimiter.Create(limiter) + RateLimiter = args => limiter.AcquireAsync(1, args.Context.CancellationToken) }) .Build() .GetPipelineDescriptor() @@ -129,7 +136,7 @@ public void AddRateLimiter_Options_Ok() var strategy = new ResiliencePipelineBuilder() .AddRateLimiter(new RateLimiterStrategyOptions { - RateLimiter = ResilienceRateLimiter.Create(Substitute.For()) + RateLimiter = args => new ValueTask(Substitute.For()) }) .Build() .GetPipelineDescriptor() diff --git a/test/Polly.RateLimiting.Tests/RateLimiterResilienceStrategyTests.cs b/test/Polly.RateLimiting.Tests/RateLimiterResilienceStrategyTests.cs index d9580a03dc9..e8380d1569e 100644 --- a/test/Polly.RateLimiting.Tests/RateLimiterResilienceStrategyTests.cs +++ b/test/Polly.RateLimiting.Tests/RateLimiterResilienceStrategyTests.cs @@ -88,6 +88,44 @@ public async Task Execute_LeaseRejected(bool hasEvents, bool hasRetryAfter) _listener.GetArgs().Should().HaveCount(1); } + [InlineData(true)] + [InlineData(false)] + [Theory] + public async Task Dispose_DisposableResourcesShouldBeDisposed(bool isAsync) + { + using var limiter = new ConcurrencyLimiter(new ConcurrencyLimiterOptions { PermitLimit = 1 }); + using var wrapper = new DisposeWrapper(limiter); + var strategy = new RateLimiterResilienceStrategy(null!, null, null!, wrapper); + + if (isAsync) + { + await strategy.DisposeAsync(); + } + else + { + strategy.Dispose(); + } + + await limiter.Invoking(l => l.AcquireAsync(1).AsTask()).Should().ThrowAsync(); + } + + [InlineData(true)] + [InlineData(false)] + [Theory] + public async Task Dispose_NoDisposableResources_ShouldNotThrow(bool isAsync) + { + using var strategy = new RateLimiterResilienceStrategy(null!, null, null!, null); + + if (isAsync) + { + await strategy.Invoking(s => s.DisposeAsync().AsTask()).Should().NotThrowAsync(); + } + else + { + strategy.Invoking(s => s.Dispose()).Should().NotThrow(); + } + } + private void SetupLimiter(CancellationToken token) { var result = new ValueTask(_lease); @@ -107,7 +145,7 @@ private ResiliencePipeline Create() return builder.AddRateLimiter(new RateLimiterStrategyOptions { - RateLimiter = ResilienceRateLimiter.Create(_limiter), + RateLimiter = args => _limiter.AcquireAsync(1, args.Context.CancellationToken), OnRejected = _event }) .Build(); diff --git a/test/Polly.RateLimiting.Tests/ResilienceRateLimiterTests.cs b/test/Polly.RateLimiting.Tests/ResilienceRateLimiterTests.cs deleted file mode 100644 index 428e89f7b8a..00000000000 --- a/test/Polly.RateLimiting.Tests/ResilienceRateLimiterTests.cs +++ /dev/null @@ -1,89 +0,0 @@ -using System.Threading.RateLimiting; -using NSubstitute; - -namespace Polly.RateLimiting.Tests; - -public class ResilienceRateLimiterTests -{ - [Fact] - public async Task Create_RateLimiter_Ok() - { - var lease = Substitute.For(); - var leaseTask = new ValueTask(lease); - - var rateLimiter = Substitute.For(); - rateLimiter - .GetType() - .GetMethod("AcquireAsyncCore", BindingFlags.NonPublic | BindingFlags.Instance)! - .Invoke(rateLimiter, new object[] { 1, default(CancellationToken) }) - .Returns(leaseTask); - - using var limiter = ResilienceRateLimiter.Create(rateLimiter); - - (await limiter.AcquireAsync(ResilienceContextPool.Shared.Get())).Should().Be(lease); - limiter.Limiter.Should().NotBeNull(); - } - - [Fact] - public async Task Create_PartitionedRateLimiter_Ok() - { - var context = ResilienceContextPool.Shared.Get(); - - var lease = Substitute.For(); - var leaseTask = new ValueTask(lease); - - var rateLimiter = Substitute.For>(); - rateLimiter - .GetType() - .GetMethod("AcquireAsyncCore", BindingFlags.NonPublic | BindingFlags.Instance)! - .Invoke(rateLimiter, new object[] { context, 1, default(CancellationToken) }) - .Returns(leaseTask); - - using var limiter = ResilienceRateLimiter.Create(rateLimiter); - - (await limiter.AcquireAsync(context)).Should().Be(lease); - limiter.PartitionedLimiter.Should().NotBeNull(); - } - - [InlineData(true)] - [InlineData(false)] - [Theory] - public async Task RateLimiter_Dispose_EnsureDisposed(bool isAsync) - { - using var concurrencyLimiter = new ConcurrencyLimiter(new ConcurrencyLimiterOptions { PermitLimit = 10, QueueLimit = 10 }); - var limiter = ResilienceRateLimiter.Create(concurrencyLimiter); - - if (isAsync) - { - await limiter.DisposeAsync(); - } - else - { - limiter.Dispose(); - } - - await limiter.Invoking(l => l.AcquireAsync(ResilienceContextPool.Shared.Get()).AsTask()).Should().ThrowAsync(); - } - - [InlineData(true)] - [InlineData(false)] - [Theory] - public async Task PartitionedRateLimiter_Dispose_EnsureDisposed(bool isAsync) - { - using var partitioned = PartitionedRateLimiter.Create( - c => RateLimitPartition.GetConcurrencyLimiter("a", - _ => new ConcurrencyLimiterOptions { PermitLimit = 10, QueueLimit = 10 })); - var limiter = ResilienceRateLimiter.Create(partitioned); - - if (isAsync) - { - await limiter.DisposeAsync(); - } - else - { - limiter.Dispose(); - } - - await limiter.Invoking(l => l.AcquireAsync(ResilienceContextPool.Shared.Get()).AsTask()).Should().ThrowAsync(); - } -} From 86932c56f1448c98c604e2a9ac000513fd016a08 Mon Sep 17 00:00:00 2001 From: Martin Tomka Date: Wed, 23 Aug 2023 10:34:19 +0200 Subject: [PATCH 2/2] PR comments --- src/Polly.RateLimiting/RateLimiterStrategyOptions.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Polly.RateLimiting/RateLimiterStrategyOptions.cs b/src/Polly.RateLimiting/RateLimiterStrategyOptions.cs index 4cc3c793213..f35c83c84dc 100644 --- a/src/Polly.RateLimiting/RateLimiterStrategyOptions.cs +++ b/src/Polly.RateLimiting/RateLimiterStrategyOptions.cs @@ -39,7 +39,7 @@ public class RateLimiterStrategyOptions : ResilienceStrategyOptions public Func? OnRejected { get; set; } /// - /// Gets or sets a rate limiter used by the strategy. + /// Gets or sets a rate limiter delegate that produces . /// /// /// The default value is . If this property is , then the strategy