Merge pull request #13 from imurashka/ivan/composite-disposable-fix

ControllerCompositeDisposable: safer post-dispose behavior

Author: perepechenko

src/ControllersTree/Core/Utils/ControllerCompositeDisposable.cs

@@ -11,14 +11,22 @@ namespace Playtika.Controllers
     public class ControllerCompositeDisposable : IDisposable
     {
         private readonly List<IDisposable> _disposables = ListPool<IDisposable>.Get();
+        private bool _disposed = false;
 
         /// <summary>
         /// Adds a disposable object to the internal list of disposables.
         /// </summary>
         /// <param name="disposable">The disposable object to add to the list.</param>
         public void Add(IDisposable disposable)
         {
-            _disposables.Add(disposable);
+            if (_disposed)
+            {
+                disposable?.Dispose();
+            }
+            else if (disposable != null)
+            {
+                _disposables.Add(disposable);
+            }
         }
 
         /// <summary>
@@ -27,33 +35,63 @@ public void Add(IDisposable disposable)
         /// <param name="collection">The collection of disposable objects to add to the list.</param>
         public void AddRange(IEnumerable<IDisposable> collection)
         {
-            _disposables.AddRange(collection);
+            if (collection == null)
+            {
+                return;
+            }
+
+            if (_disposed)
+            {
+                using var pooledObject = ListPool<IDisposable>.Get(out var disposablesList);
+                disposablesList.AddRange(collection.Where(c => c != null));
+                DisposeMany(disposablesList);
+            }
+            else
+            {
+                _disposables.AddRange(collection);
+            }
         }
 
         public void Dispose()
         {
-            using var pooledObject = ListPool<Exception>.Get(out var exceptionList);
+            if (_disposed)
+            {
+                return;
+            }
+
+            _disposed = true;
+
+            try
+            {
+                DisposeMany(_disposables);
+            }
+            finally
+            {
+                ListPool<IDisposable>.Release(_disposables);
+            }
+        }
 
-            foreach (var disposable in _disposables)
+        private static void DisposeMany(IEnumerable<IDisposable> disposables)
+        {
+            using var pooledObject = ListPool<Exception>.Get(out var exceptionList);
+            foreach (var disposable in disposables)
             {
                 try
                 {
-                    disposable.Dispose();
+                    disposable?.Dispose();
                 }
                 catch (Exception e)
                 {
                     exceptionList.Add(e);
                 }
             }
 
-            _disposables.Clear();
-
-            ListPool<IDisposable>.Release(_disposables);
-
-            if (exceptionList.Any())
+            switch (exceptionList.Count)
             {
-                throw new AggregateException(exceptionList);
+                case 0: return;
+                case 1: throw exceptionList[0];
+                default: throw new AggregateException(exceptionList.ToList());
             }
         }
     }
-}
+}

src/ControllersTree/Tests/ControllerCompositeDisposableTests.cs

@@ -0,0 +1,267 @@
+using System;
+using NUnit.Framework;
+using Playtika.Controllers;
+
+namespace UnitTests.Controllers
+{
+    [TestFixture]
+    public class ControllerCompositeDisposableTests
+    {
+        [Test]
+        public void ControllerCompositeDisposable_SingleDisposable_Disposes()
+        {
+            // Arrange
+            var composite = new ControllerCompositeDisposable();
+            var disposable = new TestDisposable();
+
+            composite.Add(disposable);
+
+            // Act
+            composite.Dispose();
+
+            // Assert
+            Assert.AreEqual(1, disposable.DisposeCallCount, "Disposable should be disposed exactly once");
+        }
+
+        [Test]
+        public void ControllerCompositeDisposable_DisposeEmpty_DoesNotThrow()
+        {
+            // Arrange
+            var composite = new ControllerCompositeDisposable();
+
+            // Act / Assert
+            Assert.DoesNotThrow(
+                () =>
+                {
+                    composite.Dispose();
+                });
+        }
+
+        [Test]
+        public void ControllerCompositeDisposable_DisposeTwice_IsIdempotent()
+        {
+            // Arrange
+            var composite = new ControllerCompositeDisposable();
+            var disposable = new TestDisposable();
+            composite.Add(disposable);
+
+            // Act
+            composite.Dispose();
+            composite.Dispose();
+
+            // Assert
+            Assert.AreEqual(1, disposable.DisposeCallCount, "Disposable should still be disposed exactly once");
+        }
+
+        [Test]
+        public void ControllerCompositeDisposable_AddAfterDisposed_DisposesImmediately()
+        {
+            // Arrange
+            var composite = new ControllerCompositeDisposable();
+            var disposable = new TestDisposable();
+
+            composite.Dispose();
+
+            // Act
+            composite.Add(disposable);
+
+            // Assert
+            Assert.AreEqual(1, disposable.DisposeCallCount, "Disposable added after composite dispose should be disposed immediately");
+        }
+
+        [Test]
+        public void ControllerCompositeDisposable_AddNull_IgnoredAndOtherDisposablesWork()
+        {
+            // Arrange
+            var composite = new ControllerCompositeDisposable();
+            var disposable = new TestDisposable();
+
+            composite.Add(null);
+            composite.Add(disposable);
+            composite.Add(null);
+
+            // Act
+            composite.Dispose();
+
+            // Assert
+            Assert.AreEqual(1, disposable.DisposeCallCount, "Non-null disposable should be disposed exactly once");
+        }
+
+        [Test]
+        public void ControllerCompositeDisposable_AddRangeBeforeDispose_DisposesAll()
+        {
+            // Arrange
+            var composite = new ControllerCompositeDisposable();
+            var d1 = new TestDisposable();
+            var d2 = new TestDisposable();
+            var collection = new[] { d1, d2 };
+
+            composite.AddRange(collection);
+
+            // Act
+            composite.Dispose();
+
+            // Assert
+            Assert.AreEqual(1, d1.DisposeCallCount, "First disposable should be disposed exactly once");
+            Assert.AreEqual(1, d2.DisposeCallCount, "Second disposable should be disposed exactly once");
+        }
+
+        [Test]
+        public void ControllerCompositeDisposable_AddRangeNullCollection_DoesNotThrow()
+        {
+            // Arrange
+            var composite = new ControllerCompositeDisposable();
+
+            // Act / Assert
+            Assert.DoesNotThrow(
+                () =>
+                {
+                    composite.AddRange(null);
+                    composite.Dispose();
+                });
+        }
+
+        [Test]
+        public void ControllerCompositeDisposable_AddRangeWithNullItems_DisposesNonNullOnly()
+        {
+            // Arrange
+            var composite = new ControllerCompositeDisposable();
+            var d1 = new TestDisposable();
+            var d3 = new TestDisposable();
+            var collection = new[] { d1, null, d3 };
+
+            composite.AddRange(collection);
+
+            // Act
+            composite.Dispose();
+
+            // Assert
+            Assert.AreEqual(1, d1.DisposeCallCount, "First non-null disposable should be disposed exactly once");
+            Assert.AreEqual(1, d3.DisposeCallCount, "Second non-null disposable should be disposed exactly once");
+        }
+
+        [Test]
+        public void ControllerCompositeDisposable_AddRangeAfterDisposed_DisposesAllImmediately()
+        {
+            // Arrange
+            var composite = new ControllerCompositeDisposable();
+            var d1 = new TestDisposable();
+            var d2 = new TestDisposable();
+            var collection = new[] { d1, d2 };
+
+            composite.Dispose();
+
+            // Act
+            composite.AddRange(collection);
+
+            // Assert
+            Assert.AreEqual(1, d1.DisposeCallCount, "First disposable should be disposed immediately");
+            Assert.AreEqual(1, d2.DisposeCallCount, "Second disposable should be disposed immediately");
+        }
+
+        [Test]
+        public void ControllerCompositeDisposable_OneDisposableThrows_ThrowsSingleExceptionAndDisposesOthers()
+        {
+            // Arrange
+            var composite = new ControllerCompositeDisposable();
+            var throwingDisposable = new TestDisposable { ThrowOnDispose = true };
+            var normalDisposable = new TestDisposable();
+
+            composite.Add(throwingDisposable);
+            composite.Add(normalDisposable);
+
+            // Act
+            var exception = Assert.Throws<TestControllersException>(
+                () => composite.Dispose(),
+                "Expected TestControllersException from throwing disposable");
+
+            // Assert
+            Assert.IsNotNull(exception, "Exception should not be null");
+            Assert.AreEqual(1, throwingDisposable.DisposeCallCount, "Throwing disposable should be disposed exactly once");
+            Assert.AreEqual(1, normalDisposable.DisposeCallCount, "Normal disposable should still be disposed exactly once");
+        }
+
+        [Test]
+        public void ControllerCompositeDisposable_MultipleDisposablesThrow_ThrowsAggregateExceptionWithAllInner()
+        {
+            // Arrange
+            var composite = new ControllerCompositeDisposable();
+            var d1 = new TestDisposable("d1") { ThrowOnDispose = true };
+            var d2 = new TestDisposable("d2") { ThrowOnDispose = true };
+            var d3 = new TestDisposable("d3");
+
+            composite.Add(d1);
+            composite.Add(d2);
+            composite.Add(d3);
+
+            // Act
+            var aggregate = Assert.Throws<AggregateException>(
+                () => composite.Dispose(),
+                "Expected AggregateException when multiple disposables throw");
+
+            // Assert
+            Assert.IsNotNull(aggregate, "AggregateException should not be null");
+            Assert.AreEqual(2, aggregate.InnerExceptions.Count, "AggregateException should contain all throwing disposables");
+            foreach (var inner in aggregate.InnerExceptions)
+            {
+                Assert.IsInstanceOf<TestControllersException>(inner, "Inner exception should be TestControllersException");
+            }
+
+            Assert.AreEqual(1, d1.DisposeCallCount, "First throwing disposable should be disposed exactly once");
+            Assert.AreEqual(1, d2.DisposeCallCount, "Second throwing disposable should be disposed exactly once");
+            Assert.AreEqual(1, d3.DisposeCallCount, "Non-throwing disposable should be disposed exactly once");
+        }
+
+        [Test]
+        public void ControllerCompositeDisposable_AddRangeAfterDisposed_MultipleDisposablesThrow_ThrowsAggregateException()
+        {
+            // Arrange
+            var composite = new ControllerCompositeDisposable();
+            var d1 = new TestDisposable("d1") { ThrowOnDispose = true };
+            var d2 = new TestDisposable("d2") { ThrowOnDispose = true };
+            var d3 = new TestDisposable("d3"); // does not throw
+            var collection = new[] { d1, d2, d3 };
+
+            composite.Dispose();
+
+            // Act
+            var aggregate = Assert.Throws<AggregateException>(
+                () => composite.AddRange(collection),
+                "Expected AggregateException when multiple disposables added after dispose throw");
+
+            // Assert
+            Assert.IsNotNull(aggregate, "AggregateException should not be null");
+            Assert.AreEqual(2, aggregate.InnerExceptions.Count, "AggregateException should contain all throwing disposables");
+            foreach (var inner in aggregate.InnerExceptions)
+            {
+                Assert.IsInstanceOf<TestControllersException>(inner, "Inner exception should be TestControllersException");
+            }
+
+            Assert.AreEqual(1, d1.DisposeCallCount, "First throwing disposable should be disposed exactly once");
+            Assert.AreEqual(1, d2.DisposeCallCount, "Second throwing disposable should be disposed exactly once");
+            Assert.AreEqual(1, d3.DisposeCallCount, "Non-throwing disposable should be disposed exactly once");
+        }
+
+        private sealed class TestDisposable : IDisposable
+        {
+            public int DisposeCallCount { get; private set; }
+            public bool ThrowOnDispose { get; set; }
+            public string Id { get; }
+
+            public TestDisposable(string id = null)
+            {
+                Id = id;
+            }
+
+            public void Dispose()
+            {
+                DisposeCallCount++;
+
+                if (ThrowOnDispose)
+                {
+                    throw new TestControllersException($"Dispose:{Id ?? "unknown"}");
+                }
+            }
+        }
+    }
+}

src/ControllersTree/Tests/ControllerCompositeDisposableTests.cs.meta

@@ -215,7 +215,7 @@ public async Task ControllerWithResultBase_Exception_Dispose()
                         .LaunchAsync<ActionModelTestControllerWithResult_CompleteOnStart, TestControllerArgs, TestEmptyControllerResult>(
                             args, _controllerFactory, CancellationToken);
             }
-            catch (AggregateException)
+            catch (TestControllersException)
             {
                 exceptionThrown = true;
             }