diff --git a/src/Polly.Core/PublicAPI.Unshipped.txt b/src/Polly.Core/PublicAPI.Unshipped.txt index 29c53389c40..23e982d2b47 100644 --- a/src/Polly.Core/PublicAPI.Unshipped.txt +++ b/src/Polly.Core/PublicAPI.Unshipped.txt @@ -158,7 +158,7 @@ Polly.PredicateBuilder.HandleResult(TResult result, System.Collections. Polly.PredicateBuilder.PredicateBuilder() -> void Polly.PredicateResult Polly.Registry.ConfigureBuilderContext -Polly.Registry.ConfigureBuilderContext.EnableReloads(System.Func!>! tokenProducerFactory) -> void +Polly.Registry.ConfigureBuilderContext.AddReloadToken(System.Threading.CancellationToken cancellationToken) -> void Polly.Registry.ConfigureBuilderContext.OnPipelineDisposed(System.Action! callback) -> void Polly.Registry.ConfigureBuilderContext.PipelineKey.get -> TKey Polly.Registry.ResiliencePipelineProvider diff --git a/src/Polly.Core/Registry/ConfigureBuilderContext.cs b/src/Polly.Core/Registry/ConfigureBuilderContext.cs index 4edf96c6014..431483258ca 100644 --- a/src/Polly.Core/Registry/ConfigureBuilderContext.cs +++ b/src/Polly.Core/Registry/ConfigureBuilderContext.cs @@ -31,24 +31,26 @@ internal ConfigureBuilderContext(TKey strategyKey, string builderName, string? b /// internal string? BuilderInstanceName { get; } - internal Func>? ReloadTokenProducer { get; private set; } + internal List ReloadTokens { get; } = new(); internal List DisposeCallbacks { get; } = new(); /// - /// Enables dynamic reloading of the strategy retrieved from . + /// Reloads the pipeline when is canceled. /// - /// The producer of that is triggered when change occurs. + /// The cancellation token that triggers the pipeline reload when cancelled. /// - /// The should always return function that returns a new instance when invoked otherwise - /// the reload infrastructure will stop listening for changes. The is called only once for each strategy. + /// You can add multiple reload tokens to the context. Non-cancelable or already canceled tokens are ignored. /// [EditorBrowsable(EditorBrowsableState.Never)] - public void EnableReloads(Func> tokenProducerFactory) + public void AddReloadToken(CancellationToken cancellationToken) { - Guard.NotNull(tokenProducerFactory); + if (!cancellationToken.CanBeCanceled || cancellationToken.IsCancellationRequested) + { + return; + } - ReloadTokenProducer = tokenProducerFactory; + ReloadTokens.Add(cancellationToken); } /// diff --git a/src/Polly.Core/Registry/RegistryPipelineComponentBuilder.cs b/src/Polly.Core/Registry/RegistryPipelineComponentBuilder.cs index 17431e37d2e..42ee13b5086 100644 --- a/src/Polly.Core/Registry/RegistryPipelineComponentBuilder.cs +++ b/src/Polly.Core/Registry/RegistryPipelineComponentBuilder.cs @@ -37,17 +37,20 @@ internal PipelineComponent CreateComponent() new ResilienceTelemetrySource(_builderName, _instanceName, null), builder.Listener); - var initialPipeline = builder.ComponentFactory(); + var component = builder.ComponentFactory(); - if (builder.ReloadTokenProducer is null) + if (builder.ReloadTokens.Count == 0) { - return initialPipeline; + return component; } return PipelineComponentFactory.CreateReloadable( - initialPipeline, - builder.ReloadTokenProducer(), - () => CreateBuilder().ComponentFactory(), + new ReloadableComponent.Entry(component, builder.ReloadTokens), + () => + { + var builder = CreateBuilder(); + return new ReloadableComponent.Entry(builder.ComponentFactory(), builder.ReloadTokens); + }, telemetry); } @@ -63,14 +66,14 @@ private Builder CreateBuilder() () => PipelineComponentFactory.WithDisposableCallbacks( builder.BuildPipelineComponent(), context.DisposeCallbacks), - context.ReloadTokenProducer, + context.ReloadTokens, context.DisposeCallbacks, builder.TelemetryListener); } private record Builder( Func ComponentFactory, - Func>? ReloadTokenProducer, + List ReloadTokens, List DisposeCallbacks, TelemetryListener? Listener); } diff --git a/src/Polly.Core/Utils/Pipeline/PipelineComponentFactory.cs b/src/Polly.Core/Utils/Pipeline/PipelineComponentFactory.cs index 3818b027f99..7579af518f1 100644 --- a/src/Polly.Core/Utils/Pipeline/PipelineComponentFactory.cs +++ b/src/Polly.Core/Utils/Pipeline/PipelineComponentFactory.cs @@ -30,8 +30,7 @@ public static PipelineComponent CreateComposite( TimeProvider timeProvider) => CompositeComponent.Create(components, telemetry, timeProvider); public static PipelineComponent CreateReloadable( - PipelineComponent initialComponent, - Func onReload, - Func factory, - ResilienceStrategyTelemetry telemetry) => new ReloadableComponent(initialComponent, onReload, factory, telemetry); + ReloadableComponent.Entry initial, + Func factory, + ResilienceStrategyTelemetry telemetry) => new ReloadableComponent(initial, factory, telemetry); } diff --git a/src/Polly.Core/Utils/Pipeline/ReloadableComponent.cs b/src/Polly.Core/Utils/Pipeline/ReloadableComponent.cs index 98f5cfde2e7..0380b57e164 100644 --- a/src/Polly.Core/Utils/Pipeline/ReloadableComponent.cs +++ b/src/Polly.Core/Utils/Pipeline/ReloadableComponent.cs @@ -10,24 +10,21 @@ internal sealed class ReloadableComponent : PipelineComponent public const string OnReloadEvent = "OnReload"; - private readonly Func _onReload; - private readonly Func _factory; + private readonly Func _factory; private readonly ResilienceStrategyTelemetry _telemetry; + private CancellationTokenSource _tokenSource = null!; private CancellationTokenRegistration _registration; + private List _reloadTokens; - public ReloadableComponent( - PipelineComponent initialComponent, - Func onReload, - Func factory, - ResilienceStrategyTelemetry telemetry) + public ReloadableComponent(Entry entry, Func factory, ResilienceStrategyTelemetry telemetry) { - Component = initialComponent; + Component = entry.Component; - _onReload = onReload; + _reloadTokens = entry.ReloadTokens; _factory = factory; _telemetry = telemetry; - RegisterOnReload(default); + TryRegisterOnReload(); } public PipelineComponent Component { get; private set; } @@ -52,15 +49,15 @@ public override ValueTask DisposeAsync() return Component.DisposeAsync(); } - private void RegisterOnReload(CancellationToken previousToken) + private void TryRegisterOnReload() { - var token = _onReload(); - if (token == previousToken) + if (_reloadTokens.Count == 0) { return; } - _registration = token.Register(() => + _tokenSource = CancellationTokenSource.CreateLinkedTokenSource(_reloadTokens.ToArray()); + _registration = _tokenSource.Token.Register(() => { var context = ResilienceContextPool.Shared.Get().Initialize(isSynchronous: true); PipelineComponent previousComponent = Component; @@ -68,27 +65,34 @@ private void RegisterOnReload(CancellationToken previousToken) try { _telemetry.Report(new(ResilienceEventSeverity.Information, OnReloadEvent), context, new OnReloadArguments()); - Component = _factory(); + (Component, _reloadTokens) = _factory(); previousComponent.Dispose(); } catch (Exception e) { + _reloadTokens = new List(); var args = new OutcomeArguments(context, Outcome.FromException(e), new ReloadFailedArguments(e)); _telemetry.Report(new(ResilienceEventSeverity.Error, ReloadFailedEvent), args); ResilienceContextPool.Shared.Return(context); } DisposeRegistration(); - RegisterOnReload(token); + TryRegisterOnReload(); }); } #pragma warning disable S2952 // Classes should "Dispose" of members from the classes' own "Dispose" methods - private void DisposeRegistration() => _registration.Dispose(); + private void DisposeRegistration() + { + _registration.Dispose(); + _tokenSource.Dispose(); + } #pragma warning restore S2952 // Classes should "Dispose" of members from the classes' own "Dispose" methods - internal readonly record struct ReloadFailedArguments(Exception Exception); + internal record ReloadFailedArguments(Exception Exception); + + internal record OnReloadArguments(); - internal readonly record struct OnReloadArguments(); + internal record Entry(PipelineComponent Component, List ReloadTokens); } diff --git a/src/Polly.Extensions/Registry/ConfigureBuilderContextExtensions.cs b/src/Polly.Extensions/Registry/ConfigureBuilderContextExtensions.cs index c1df328ac88..c9948be89e0 100644 --- a/src/Polly.Extensions/Registry/ConfigureBuilderContextExtensions.cs +++ b/src/Polly.Extensions/Registry/ConfigureBuilderContextExtensions.cs @@ -1,4 +1,5 @@ using System.Diagnostics.CodeAnalysis; +using System.Threading; using Microsoft.Extensions.Options; using Polly.Utils; @@ -33,6 +34,24 @@ public static class ConfigureBuilderContextExtensions Guard.NotNull(context); Guard.NotNull(optionsMonitor); - context.EnableReloads(() => new OptionsReloadHelper(optionsMonitor, name ?? Options.DefaultName).GetCancellationToken); + name ??= string.Empty; + +#pragma warning disable CA2000 // Dispose objects before losing scope + var source = new CancellationTokenSource(); +#pragma warning restore CA2000 // Dispose objects before losing scope + var registration = optionsMonitor.OnChange((_, changedNamed) => + { + if (name == changedNamed) + { + source.Cancel(); + } + }); + + context.AddReloadToken(source.Token); + context.OnPipelineDisposed(() => + { + registration?.Dispose(); + source.Dispose(); + }); } } diff --git a/src/Polly.Extensions/Utils/OptionsReloadHelper.cs b/src/Polly.Extensions/Utils/OptionsReloadHelper.cs deleted file mode 100644 index 80a3acbbb76..00000000000 --- a/src/Polly.Extensions/Utils/OptionsReloadHelper.cs +++ /dev/null @@ -1,30 +0,0 @@ -using System.Diagnostics.CodeAnalysis; -using Microsoft.Extensions.Options; - -namespace Polly.Utils; - -#pragma warning disable CA1001 // we can get away of not disposing this class because it's active for a lifetime of app -#pragma warning disable S2931 - -internal sealed class OptionsReloadHelper<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] T> -{ - private CancellationTokenSource _cancellation = new(); - - public OptionsReloadHelper(IOptionsMonitor monitor, string name) => monitor.OnChange((_, changedNamed) => - { - if (name == changedNamed) - { - HandleChange(); - } - }); - - public CancellationToken GetCancellationToken() => _cancellation.Token; - - private void HandleChange() - { - var oldCancellation = _cancellation; - _cancellation = new CancellationTokenSource(); - oldCancellation.Cancel(); - oldCancellation.Dispose(); - } -} diff --git a/test/Polly.Core.Tests/Registry/ConfigureBuilderContextTests.cs b/test/Polly.Core.Tests/Registry/ConfigureBuilderContextTests.cs new file mode 100644 index 00000000000..72c78d10143 --- /dev/null +++ b/test/Polly.Core.Tests/Registry/ConfigureBuilderContextTests.cs @@ -0,0 +1,21 @@ +using Polly.Registry; + +namespace Polly.Core.Tests.Registry; + +public class ConfigureBuilderContextTests +{ + [Fact] + public void AddReloadToken_Ok() + { + var context = new ConfigureBuilderContext(0, "dummy", "dummy"); + using var source = new CancellationTokenSource(); + + context.AddReloadToken(CancellationToken.None); + context.AddReloadToken(source.Token); + + source.Cancel(); + context.AddReloadToken(source.Token); + + context.ReloadTokens.Should().HaveCount(1); + } +} diff --git a/test/Polly.Core.Tests/Registry/ResiliencePipelineRegistryTests.cs b/test/Polly.Core.Tests/Registry/ResiliencePipelineRegistryTests.cs index e21f6f34f13..a18a2034663 100644 --- a/test/Polly.Core.Tests/Registry/ResiliencePipelineRegistryTests.cs +++ b/test/Polly.Core.Tests/Registry/ResiliencePipelineRegistryTests.cs @@ -249,18 +249,22 @@ public void TryGet_GenericNoBuilder_Null() strategy.Should().BeNull(); } - [Fact] - public void EnableReloads_Ok() + [InlineData(true)] + [InlineData(false)] + [Theory] + public void EnableReloads_Ok(bool firstOne) { // arrange var retryCount = 2; using var registry = new ResiliencePipelineRegistry(); - using var changeSource = new CancellationTokenSource(); + using var token1 = new CancellationTokenSource(); + using var token2 = new CancellationTokenSource(); registry.TryAddBuilder("dummy", (builder, context) => { // this call enables dynamic reloads for the dummy strategy - context.EnableReloads(() => () => changeSource.Token); + context.AddReloadToken(token1.Token); + context.AddReloadToken(token2.Token); builder.AddRetry(new RetryStrategyOptions { @@ -280,7 +284,16 @@ public void EnableReloads_Ok() tries = 0; retryCount = 5; - changeSource.Cancel(); + + if (firstOne) + { + token1.Cancel(); + } + else + { + token2.Cancel(); + } + strategy.Execute(() => tries++); tries.Should().Be(retryCount + 1); } @@ -296,7 +309,7 @@ public void EnableReloads_EnsureDisposedCallbackCalled() registry.TryAddBuilder("dummy", (builder, context) => { // this call enables dynamic reloads for the dummy strategy - context.EnableReloads(() => () => changeSource.Token); + context.AddReloadToken(changeSource.Token); context.OnPipelineDisposed(() => disposedCalls++); builder.AddTimeout(TimeSpan.FromSeconds(1)); }); @@ -327,7 +340,7 @@ public void EnableReloads_Generic_Ok() registry.TryAddBuilder("dummy", (builder, context) => { // this call enables dynamic reloads for the dummy strategy - context.EnableReloads(() => () => changeSource.Token); + context.AddReloadToken(changeSource.Token); builder.AddRetry(new RetryStrategyOptions { diff --git a/test/Polly.Core.Tests/Utils/Pipeline/ReloadablePipelineComponentTests.cs b/test/Polly.Core.Tests/Utils/Pipeline/ReloadablePipelineComponentTests.cs index 55b8dc98ac7..883e97cc9d1 100644 --- a/test/Polly.Core.Tests/Utils/Pipeline/ReloadablePipelineComponentTests.cs +++ b/test/Polly.Core.Tests/Utils/Pipeline/ReloadablePipelineComponentTests.cs @@ -55,24 +55,19 @@ public void ChangeTriggered_StrategyReloaded() var component = Substitute.For(); using var sut = CreateSut(component); - for (var i = 0; i < 10; i++) - { - var src = _cancellationTokenSource; - _cancellationTokenSource = new CancellationTokenSource(); - src.Cancel(); - - sut.Component.Should().NotBe(component); - } + sut.Component.Should().Be(component); + _cancellationTokenSource.Cancel(); + sut.Component.Should().NotBe(component); _events.Where(e => e.Event.EventName == "ReloadFailed").Should().HaveCount(0); - _events.Where(e => e.Event.EventName == "OnReload").Should().HaveCount(10); + _events.Where(e => e.Event.EventName == "OnReload").Should().HaveCount(1); } [Fact] public void ChangeTriggered_EnsureOldStrategyDisposed() { var component = Substitute.For(); - using var sut = CreateSut(component, () => Substitute.For()); + using var sut = CreateSut(component, () => new(Substitute.For(), new List())); for (var i = 0; i < 10; i++) { @@ -109,12 +104,12 @@ public void ChangeTriggered_FactoryError_LastStrategyUsedAndErrorReported() args.Exception.Should().BeOfType(); } - private ReloadableComponent CreateSut(PipelineComponent? initial = null, Func? factory = null) + private ReloadableComponent CreateSut(PipelineComponent? initial = null, Func? factory = null) { - factory ??= () => PipelineComponent.Empty; + factory ??= () => new ReloadableComponent.Entry(PipelineComponent.Empty, new List()); - return (ReloadableComponent)PipelineComponentFactory.CreateReloadable(initial ?? PipelineComponent.Empty, - () => _cancellationTokenSource.Token, + return (ReloadableComponent)PipelineComponentFactory.CreateReloadable( + new ReloadableComponent.Entry(initial ?? PipelineComponent.Empty, new List { _cancellationTokenSource.Token }), factory, _telemetry); } diff --git a/test/Polly.Extensions.Tests/Registry/ConfigureBuilderContextExtensionsTEsts.cs b/test/Polly.Extensions.Tests/Registry/ConfigureBuilderContextExtensionsTEsts.cs new file mode 100644 index 00000000000..a362da31640 --- /dev/null +++ b/test/Polly.Extensions.Tests/Registry/ConfigureBuilderContextExtensionsTEsts.cs @@ -0,0 +1,33 @@ +using Microsoft.Extensions.Options; +using NSubstitute; +using NSubstitute.ReturnsExtensions; +using Polly.Registry; + +namespace Polly.Extensions.Tests.Registry; + +public class ConfigureBuilderContextExtensionsTests +{ + [Fact] + public void ConfigureReloads_MonitorRegistrationReturnsNull_DoesNotThrow() + { + var monitor = Substitute.For>(); + monitor.OnChange(default!).ReturnsNullForAnyArgs(); + + var listener = new FakeTelemetryListener(); + var registry = new ResiliencePipelineRegistry(); + var pipeline = registry.GetOrAddPipeline("pipeline", (builder, context) => + { + builder.TelemetryListener = listener; + builder.AddConcurrencyLimiter(1); + context.EnableReloads(monitor); + }); + + registry.Dispose(); + + listener.Events.Should().BeEmpty(); + } + + public class Options + { + } +} diff --git a/test/Polly.Extensions.Tests/Utils/OptionsReloadHelperTests.cs b/test/Polly.Extensions.Tests/Utils/OptionsReloadHelperTests.cs deleted file mode 100644 index bc410917824..00000000000 --- a/test/Polly.Extensions.Tests/Utils/OptionsReloadHelperTests.cs +++ /dev/null @@ -1,22 +0,0 @@ -using Microsoft.Extensions.Options; -using NSubstitute; -using Polly.Utils; - -namespace Polly.Extensions.Tests.Utils; - -public class OptionsReloadHelperTests -{ - [Fact] - public void Ctor_NamedOptions() - { - var disposable = Substitute.For(); - var monitor = Substitute.For>(); - - monitor.OnChange(Arg.Any>()) - .Returns(disposable); - - var helper = new OptionsReloadHelper(monitor, "name"); - - monitor.Received().OnChange(Arg.Any>()); - } -} diff --git a/test/Polly.Testing.Tests/ResiliencePipelineExtensionsTests.cs b/test/Polly.Testing.Tests/ResiliencePipelineExtensionsTests.cs index b609337e93a..2018ba08824 100644 --- a/test/Polly.Testing.Tests/ResiliencePipelineExtensionsTests.cs +++ b/test/Polly.Testing.Tests/ResiliencePipelineExtensionsTests.cs @@ -112,11 +112,13 @@ public void GetPipelineDescriptor_SingleStrategy_Ok() public void GetPipelineDescriptor_Reloadable_Ok() { // arrange + using var source = new CancellationTokenSource(); using var registry = new ResiliencePipelineRegistry(); var strategy = registry.GetOrAddPipeline("dummy", (builder, context) => { - context.EnableReloads(() => () => CancellationToken.None); context.OnPipelineDisposed(() => { }); + context.AddReloadToken(source.Token); + builder .AddConcurrencyLimiter(10) .AddStrategy(_ => new CustomStrategy(), new TestOptions());