Clean duplications around disposing the pipelines (#1530)

src/Polly.Core/Registry/ResiliencePipelineRegistry.TResult.cs

@@ -5,7 +5,7 @@ namespace Polly.Registry;
 public sealed partial class ResiliencePipelineRegistry<TKey> : ResiliencePipelineProvider<TKey>
     where TKey : notnull
 {
-    private sealed class GenericRegistry<TResult> : IDisposable, IAsyncDisposable
+    private sealed class GenericRegistry<TResult> : IAsyncDisposable
     {
         private readonly Func<ResiliencePipelineBuilder<TResult>> _activator;
         private readonly ConcurrentDictionary<TKey, Action<ResiliencePipelineBuilder<TResult>, ConfigureBuilderContext<TKey>>> _builders;
@@ -64,16 +64,6 @@ public ResiliencePipeline<TResult> GetOrAdd(TKey key, Action<ResiliencePipelineB
 
         public bool TryAddBuilder(TKey key, Action<ResiliencePipelineBuilder<TResult>, ConfigureBuilderContext<TKey>> configure) => _builders.TryAdd(key, configure);
 
-        public void Dispose()
-        {
-            foreach (var strategy in _pipelines.Values)
-            {
-                strategy.DisposeHelper.ForceDispose();
-            }
-
-            _pipelines.Clear();
-        }
-
         public async ValueTask DisposeAsync()
         {
             foreach (var strategy in _pipelines.Values)

src/Polly.Core/Registry/ResiliencePipelineRegistry.cs

@@ -218,19 +218,7 @@ public bool TryAddBuilder<TResult>(TKey key, Action<ResiliencePipelineBuilder<TR
     /// After the disposal, all resilience pipelines still used outside of the builder are disposed
     /// and cannot be used anymore.
     /// </remarks>
-    public void Dispose()
-    {
-        _disposed = true;
-
-        var pipelines = _pipelines.Values.ToList();
-        _pipelines.Clear();
-
-        var registries = _genericRegistry.Values.Cast<IDisposable>().ToList();
-        _genericRegistry.Clear();
-
-        pipelines.ForEach(p => p.DisposeHelper.ForceDispose());
-        registries.ForEach(p => p.Dispose());
-    }
+    public void Dispose() => DisposeAsync().AsTask().GetAwaiter().GetResult();
 
     /// <summary>
     /// Disposes all resources that are held by the resilience pipelines created by this builder.

src/Polly.Core/Utils/Pipeline/BridgeComponentBase.cs

@@ -6,28 +6,17 @@ internal abstract class BridgeComponentBase : PipelineComponent
 
     protected BridgeComponentBase(object strategy) => _strategy = strategy;
 
-    public override void Dispose()
-    {
-        if (_strategy is IDisposable disposable)
-        {
-            disposable.Dispose();
-        }
-        else if (_strategy is IAsyncDisposable asyncDisposable)
-        {
-            asyncDisposable.DisposeAsync().AsTask().GetAwaiter().GetResult();
-        }
-    }
-
     public override ValueTask DisposeAsync()
     {
         if (_strategy is IAsyncDisposable asyncDisposable)
         {
             return asyncDisposable.DisposeAsync();
         }
-        else
+        else if (_strategy is IDisposable disposable)
         {
-            Dispose();
-            return default;
+            disposable.Dispose();
         }
+
+        return default;
     }
 }

src/Polly.Core/Utils/Pipeline/ComponentDisposeHelper.cs

@@ -1,6 +1,6 @@
 namespace Polly.Utils.Pipeline;
 
-internal sealed class ComponentDisposeHelper : IDisposable, IAsyncDisposable
+internal sealed class ComponentDisposeHelper : IAsyncDisposable
 {
     private readonly PipelineComponent _component;
     private readonly DisposeBehavior _disposeBehavior;
@@ -12,14 +12,6 @@ public ComponentDisposeHelper(PipelineComponent component, DisposeBehavior dispo
         _disposeBehavior = disposeBehavior;
     }
 
-    public void Dispose()
-    {
-        if (EnsureDisposable())
-        {
-            ForceDispose();
-        }
-    }
-
     public ValueTask DisposeAsync()
     {
         if (EnsureDisposable())
@@ -38,14 +30,6 @@ public void EnsureNotDisposed()
         }
     }
 
-    public void ForceDispose()
-    {
-        _disposed = true;
-#pragma warning disable S2952 // Classes should "Dispose" of members from the classes' own "Dispose" methods
-        _component.Dispose();
-#pragma warning restore S2952 // Classes should "Dispose" of members from the classes' own "Dispose" methods
-    }
-
     public ValueTask ForceDisposeAsync()
     {
         _disposed = true;

src/Polly.Core/Utils/Pipeline/ComponentWithDisposeCallbacks.cs

@@ -12,13 +12,6 @@ public ComponentWithDisposeCallbacks(PipelineComponent component, List<Action> c
 
     internal PipelineComponent Component { get; }
 
-    public override void Dispose()
-    {
-        ExecuteCallbacks();
-
-        Component.Dispose();
-    }
-
     public override ValueTask DisposeAsync()
     {
         ExecuteCallbacks();

src/Polly.Core/Utils/Pipeline/CompositeComponent.cs

@@ -61,14 +61,6 @@ public static PipelineComponent Create(
 
     public IReadOnlyList<PipelineComponent> Components { get; }
 
-    public override void Dispose()
-    {
-        foreach (var component in Components)
-        {
-            component.Dispose();
-        }
-    }
-
     public override async ValueTask DisposeAsync()
     {
         foreach (var component in Components)
@@ -155,10 +147,6 @@ internal override ValueTask<Outcome<TResult>> ExecuteCore<TResult, TState>(
                 (Next, callback, state));
         }
 
-        public override void Dispose()
-        {
-        }
-
         public override ValueTask DisposeAsync() => default;
     }
 }

src/Polly.Core/Utils/Pipeline/ExternalComponent.cs

@@ -15,11 +15,6 @@ internal override ValueTask<Outcome<TResult>> ExecuteCore<TResult, TState>(
         ResilienceContext context,
         TState state) => Component.ExecuteCore(callback, context, state);
 
-    public override void Dispose()
-    {
-        // don't dispose component that is external
-    }
-
     public override ValueTask DisposeAsync()
     {
         // don't dispose component that is external

src/Polly.Core/Utils/Pipeline/PipelineComponent.cs

@@ -6,7 +6,7 @@
 /// <remarks>
 /// The component of the pipeline can be either a strategy, a generic strategy or a whole pipeline.
 /// </remarks>
-internal abstract class PipelineComponent : IDisposable, IAsyncDisposable
+internal abstract class PipelineComponent : IAsyncDisposable
 {
     public static PipelineComponent Empty { get; } = new NullComponent();
 
@@ -33,19 +33,13 @@ internal Outcome<TResult> ExecuteCoreSync<TResult, TState>(
             (callback, state)).GetResult();
     }
 
-    public abstract void Dispose();
-
     public abstract ValueTask DisposeAsync();
 
     private class NullComponent : PipelineComponent
     {
         internal override ValueTask<Outcome<TResult>> ExecuteCore<TResult, TState>(Func<ResilienceContext, TState, ValueTask<Outcome<TResult>>> callback, ResilienceContext context, TState state)
             => callback(context, state);
 
-        public override void Dispose()
-        {
-        }
-
         public override ValueTask DisposeAsync() => default;
     }
 }

src/Polly.Core/Utils/Pipeline/ReloadableComponent.cs

@@ -8,6 +8,8 @@ internal sealed class ReloadableComponent : PipelineComponent
 {
     public const string ReloadFailedEvent = "ReloadFailed";
 
+    public const string DisposeFailedEvent = "DisposeFailed";
+
     public const string OnReloadEvent = "OnReload";
 
     private readonly Func<Entry> _factory;
@@ -37,12 +39,6 @@ internal override ValueTask<Outcome<TResult>> ExecuteCore<TResult, TState>(
         return Component.ExecuteCore(callback, context, state);
     }
 
-    public override void Dispose()
-    {
-        DisposeRegistration();
-        Component.Dispose();
-    }
-
     public override ValueTask DisposeAsync()
     {
         DisposeRegistration();
@@ -60,14 +56,12 @@ private void TryRegisterOnReload()
         _registration = _tokenSource.Token.Register(() =>
         {
             var context = ResilienceContextPool.Shared.Get().Initialize<VoidResult>(isSynchronous: true);
-            PipelineComponent previousComponent = Component;
+            var previousComponent = Component;
 
             try
             {
                 _telemetry.Report(new(ResilienceEventSeverity.Information, OnReloadEvent), context, new OnReloadArguments());
                 (Component, _reloadTokens) = _factory();
-
-                previousComponent.Dispose();
             }
             catch (Exception e)
             {
@@ -78,9 +72,27 @@ private void TryRegisterOnReload()
 
             DisposeRegistration();
             TryRegisterOnReload();
+
+            _ = DisposeDiscardedComponentSafeAsync(previousComponent);
         });
     }
 
+    private async Task DisposeDiscardedComponentSafeAsync(PipelineComponent component)
+    {
+        var context = ResilienceContextPool.Shared.Get().Initialize<VoidResult>(isSynchronous: false);
+
+        try
+        {
+            await component.DisposeAsync().ConfigureAwait(false);
+        }
+        catch (Exception e)
+        {
+            _telemetry.Report(new(ResilienceEventSeverity.Error, DisposeFailedEvent), context, Outcome.FromException(e), new DisposedFailedArguments(e));
+        }
+
+        ResilienceContextPool.Shared.Return(context);
+    }
+
 #pragma warning disable S2952 // Classes should "Dispose" of members from the classes' own "Dispose" methods
     private void DisposeRegistration()
     {
@@ -91,6 +103,8 @@ private void DisposeRegistration()
 
     internal record ReloadFailedArguments(Exception Exception);
 
+    internal record DisposedFailedArguments(Exception Exception);
+
     internal record OnReloadArguments();
 
     internal record Entry(PipelineComponent Component, List<CancellationToken> ReloadTokens);

test/Polly.Core.Tests/CircuitBreaker/CircuitBreakerResiliencePipelineBuilderTests.cs

@@ -131,12 +131,10 @@ public async Task AddCircuitBreakers_WithIsolatedManualControl_ShouldBeIsolated(
         strategy2.Execute(() => { });
     }
 
-    [InlineData(false, false)]
-    [InlineData(true, false)]
-    [InlineData(false, true)]
-    [InlineData(true, true)]
+    [InlineData(false)]
+    [InlineData(true)]
     [Theory]
-    public async Task DisposePipeline_EnsureCircuitBreakerDisposed(bool isAsync, bool attachManualControl)
+    public async Task DisposePipeline_EnsureCircuitBreakerDisposed(bool attachManualControl)
     {
         var manualControl = attachManualControl ? new CircuitBreakerManualControl() : null;
         var pipeline = new ResiliencePipelineBuilder()
@@ -153,14 +151,7 @@ public async Task DisposePipeline_EnsureCircuitBreakerDisposed(bool isAsync, boo
 
         var strategy = (ResilienceStrategy<object>)pipeline.GetPipelineDescriptor().FirstStrategy.StrategyInstance;
 
-        if (isAsync)
-        {
-            await pipeline.DisposeHelper.DisposeAsync();
-        }
-        else
-        {
-            pipeline.DisposeHelper.Dispose();
-        }
+        await pipeline.DisposeHelper.DisposeAsync();
 
         strategy.AsPipeline().Invoking(s => s.Execute(() => 1)).Should().Throw<ObjectDisposedException>();
 

test/Polly.Core.Tests/Registry/ResiliencePipelineRegistryTests.cs

@@ -424,22 +424,14 @@ public async Task Dispose_EnsureDisposed(bool isAsync)
         pipeline4.Invoking(p => p.Execute(() => "dummy")).Should().Throw<ObjectDisposedException>();
     }
 
-    [InlineData(true)]
-    [InlineData(false)]
-    [Theory]
-    public async Task DisposePipeline_NotAllowed(bool isAsync)
+    [Fact]
+    public async Task DisposePipeline_NotAllowed()
     {
         using var registry = CreateRegistry();
         var pipeline = registry.GetOrAddPipeline(StrategyId.Create("A"), builder => { builder.AddTimeout(TimeSpan.FromSeconds(1)); });
 
-        if (isAsync)
-        {
-            await pipeline.Invoking(p => p.DisposeHelper.DisposeAsync().AsTask()).Should().ThrowAsync<InvalidOperationException>();
-        }
-        else
-        {
-            pipeline.Invoking(p => p.DisposeHelper.Dispose()).Should().Throw<InvalidOperationException>();
-        }
+        await pipeline.Invoking(p => p.DisposeHelper.DisposeAsync().AsTask()).Should().ThrowAsync<InvalidOperationException>();
+
     }
 
     [InlineData(true)]

test/Polly.Core.Tests/ResiliencePipelineBuilderTests.cs

@@ -232,10 +232,8 @@ public void AddPipeline_NullFactory_Throws()
             .Be("factory");
     }
 
-    [InlineData(true)]
-    [InlineData(false)]
-    [Theory]
-    public async Task AddPipeline_EnsureNotDisposed(bool isAsync)
+    [Fact]
+    public async Task AddPipeline_EnsureNotDisposed()
     {
         var externalComponent = Substitute.For<PipelineComponent>();
         var externalBuilder = new ResiliencePipelineBuilder();
@@ -249,24 +247,13 @@ public async Task AddPipeline_EnsureNotDisposed(bool isAsync)
             .AddPipelineComponent(_ => internalComponent, new TestResilienceStrategyOptions());
         var pipeline = builder.Build();
 
-        if (isAsync)
-        {
-            await pipeline.DisposeHelper.DisposeAsync();
-            await externalComponent.Received(0).DisposeAsync();
-            await internalComponent.Received(1).DisposeAsync();
-        }
-        else
-        {
-            pipeline.DisposeHelper.Dispose();
-            externalComponent.Received(0).Dispose();
-            internalComponent.Received(1).Dispose();
-        }
+        await pipeline.DisposeHelper.DisposeAsync();
+        await externalComponent.Received(0).DisposeAsync();
+        await internalComponent.Received(1).DisposeAsync();
     }
 
-    [InlineData(true)]
-    [InlineData(false)]
-    [Theory]
-    public async Task AddPipeline_Generic_EnsureNotDisposed(bool isAsync)
+    [Fact]
+    public async Task AddPipeline_Generic_EnsureNotDisposed()
     {
         var externalComponent = Substitute.For<PipelineComponent>();
         var externalBuilder = new ResiliencePipelineBuilder<string>();
@@ -282,18 +269,9 @@ public async Task AddPipeline_Generic_EnsureNotDisposed(bool isAsync)
 
         pipeline.Execute(_ => string.Empty);
 
-        if (isAsync)
-        {
-            await pipeline.DisposeHelper.DisposeAsync();
-            await externalComponent.Received(0).DisposeAsync();
-            await internalComponent.Received(1).DisposeAsync();
-        }
-        else
-        {
-            pipeline.DisposeHelper.Dispose();
-            externalComponent.Received(0).Dispose();
-            internalComponent.Received(1).Dispose();
-        }
+        await pipeline.DisposeHelper.DisposeAsync();
+        await externalComponent.Received(0).DisposeAsync();
+        await internalComponent.Received(1).DisposeAsync();
     }
 
     [Fact]