diff --git a/osu.Game.Tests/NonVisual/OngoingOperationTrackerTest.cs b/osu.Game.Tests/NonVisual/OngoingOperationTrackerTest.cs index eef9582af9..10216c3339 100644 --- a/osu.Game.Tests/NonVisual/OngoingOperationTrackerTest.cs +++ b/osu.Game.Tests/NonVisual/OngoingOperationTrackerTest.cs @@ -3,8 +3,12 @@ using System; using NUnit.Framework; +using osu.Framework.Allocation; using osu.Framework.Bindables; +using osu.Framework.Graphics; +using osu.Framework.Screens; using osu.Framework.Testing; +using osu.Game.Screens; using osu.Game.Screens.OnlinePlay; using osu.Game.Tests.Visual; @@ -58,5 +62,45 @@ namespace osu.Game.Tests.NonVisual AddStep("end operation", () => operation.Dispose()); AddAssert("operation is ended", () => !operationInProgress.Value); } + + [Test] + public void TestOperationDisposalAfterScreenExit() + { + TestScreenWithTracker screen = null; + OsuScreenStack stack; + IDisposable operation = null; + + AddStep("create screen with tracker", () => + { + Child = stack = new OsuScreenStack + { + RelativeSizeAxes = Axes.Both + }; + + stack.Push(screen = new TestScreenWithTracker()); + }); + AddUntilStep("wait for loaded", () => screen.IsLoaded); + + AddStep("begin operation", () => operation = screen.OngoingOperationTracker.BeginOperation()); + AddAssert("operation in progress", () => screen.OngoingOperationTracker.InProgress.Value); + + AddStep("dispose after screen exit", () => + { + screen.Exit(); + operation.Dispose(); + }); + AddAssert("operation ended", () => !screen.OngoingOperationTracker.InProgress.Value); + } + + private class TestScreenWithTracker : OsuScreen + { + public OngoingOperationTracker OngoingOperationTracker { get; private set; } + + [BackgroundDependencyLoader] + private void load() + { + InternalChild = OngoingOperationTracker = new OngoingOperationTracker(); + } + } } } diff --git a/osu.Game/Screens/OnlinePlay/OngoingOperationTracker.cs b/osu.Game/Screens/OnlinePlay/OngoingOperationTracker.cs index b7ee84eb9e..aabeafe460 100644 --- a/osu.Game/Screens/OnlinePlay/OngoingOperationTracker.cs +++ b/osu.Game/Screens/OnlinePlay/OngoingOperationTracker.cs @@ -47,26 +47,21 @@ namespace osu.Game.Screens.OnlinePlay private void endOperationWithKnownLease(LeasedBindable lease) { - if (lease != leasedInProgress) - return; - // for extra safety, marshal the end of operation back to the update thread if necessary. Scheduler.Add(() => { - leasedInProgress?.Return(); + if (lease != leasedInProgress) + return; + + // UnbindAll() is purposefully used instead of Return() - the two do roughly the same thing, with one difference: + // the former won't throw if the lease has already been returned before. + // this matters because framework can unbind the lease via the internal UnbindAllBindables(), which is not always detectable + // (it is in the case of disposal, but not in the case of screen exit - at least not cleanly). + leasedInProgress?.UnbindAll(); 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;