Merge pull request #11577 from bdach/ongoing-tracker-disposal

Make ongoing operation tracker disposals idempotent

osu.Game.Tests/NonVisual/OngoingOperationTrackerTest.cs

@@ -0,0 +1,62 @@
+// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
+// See the LICENCE file in the repository root for full licence text.
+
+using System;
+using NUnit.Framework;
+using osu.Framework.Bindables;
+using osu.Framework.Testing;
+using osu.Game.Screens.OnlinePlay;
+using osu.Game.Tests.Visual;
+
+namespace osu.Game.Tests.NonVisual
+{
+    [HeadlessTest]
+    public class OngoingOperationTrackerTest : OsuTestScene
+    {
+        private OngoingOperationTracker tracker;
+        private IBindable<bool> operationInProgress;
+
+        [SetUpSteps]
+        public void SetUp()
+        {
+            AddStep("create tracker", () => Child = tracker = new OngoingOperationTracker());
+            AddStep("bind to operation status", () => operationInProgress = tracker.InProgress.GetBoundCopy());
+        }
+
+        [Test]
+        public void TestOperationTracking()
+        {
+            IDisposable firstOperation = null;
+            IDisposable secondOperation = null;
+
+            AddStep("begin first operation", () => firstOperation = tracker.BeginOperation());
+            AddAssert("first operation in progress", () => operationInProgress.Value);
+
+            AddStep("cannot start another operation",
+                () => Assert.Throws<InvalidOperationException>(() => tracker.BeginOperation()));
+
+            AddStep("end first operation", () => firstOperation.Dispose());
+            AddAssert("first operation is ended", () => !operationInProgress.Value);
+
+            AddStep("start second operation", () => secondOperation = tracker.BeginOperation());
+            AddAssert("second operation in progress", () => operationInProgress.Value);
+
+            AddStep("dispose first operation again", () => firstOperation.Dispose());
+            AddAssert("second operation still in progress", () => operationInProgress.Value);
+
+            AddStep("dispose second operation", () => secondOperation.Dispose());
+            AddAssert("second operation is ended", () => !operationInProgress.Value);
+        }
+
+        [Test]
+        public void TestOperationDisposalAfterTracker()
+        {
+            IDisposable operation = null;
+
+            AddStep("begin operation", () => operation = tracker.BeginOperation());
+            AddStep("dispose tracker", () => tracker.Expire());
+            AddStep("end operation", () => operation.Dispose());
+            AddAssert("operation is ended", () => !operationInProgress.Value);
+        }
+    }
+}

osu.Game/Screens/OnlinePlay/OngoingOperationTracker.cs

@@ -2,7 +2,6 @@
 // See the LICENCE file in the repository root for full licence text.
 
 using System;
-using osu.Framework.Allocation;
 using osu.Framework.Bindables;
 using osu.Framework.Graphics;
 
@@ -43,17 +42,46 @@ namespace osu.Game.Screens.OnlinePlay
             leasedInProgress = inProgress.BeginLease(true);
             leasedInProgress.Value = true;
 
-            // for extra safety, marshal the end of operation back to the update thread if necessary.
+            return new OngoingOperation(this, leasedInProgress);
-            return new InvokeOnDisposal(() => Scheduler.Add(endOperation, false));
         }
 
-        private void endOperation()
+        private void endOperationWithKnownLease(LeasedBindable<bool> lease)
         {
-            if (leasedInProgress == null)
+            if (lease != leasedInProgress)
-                throw new InvalidOperationException("Cannot end operation multiple times.");
+                return;
 
-            leasedInProgress.Return();
+            // for extra safety, marshal the end of operation back to the update thread if necessary.
+            Scheduler.Add(() =>
+            {
+                leasedInProgress?.Return();
+                leasedInProgress = null;
+            }, false);
+        }
+
+        protected override void Dispose(bool isDisposing)
+        {
+            base.Dispose(isDisposing);
+
+            // base call does an UnbindAllBindables().
+            // clean up the leased reference here so that it doesn't get returned twice.
             leasedInProgress = null;
         }
+
+        private class OngoingOperation : IDisposable
+        {
+            private readonly OngoingOperationTracker tracker;
+            private readonly LeasedBindable<bool> lease;
+
+            public OngoingOperation(OngoingOperationTracker tracker, LeasedBindable<bool> lease)
+            {
+                this.tracker = tracker;
+                this.lease = lease;
+            }
+
+            public void Dispose()
+            {
+                tracker.endOperationWithKnownLease(lease);
+            }
+        }
     }
 }