Merge pull request #18494 from peppy/fix-dummmy-api-request-firing-2

Fix `OnlinePlayTestScene` request handlers potentially running requests post-disposal

osu.Game.Tests/Visual/Online/TestSceneChatOverlay.cs

@@ -469,6 +469,8 @@ namespace osu.Game.Tests.Visual.Online
                 chatOverlay.Show();
             });
 
+            AddStep("Select channel 1", () => clickDrawable(getChannelListItem(testChannel1)));
+
             waitForChannel1Visible();
             AddStep("Press document next keys", () => InputManager.Keys(PlatformAction.DocumentNext));
             waitForChannel2Visible();

osu.Game.Tests/Visual/Playlists/TestScenePlaylistsResultsScreen.cs

@@ -173,6 +173,8 @@ namespace osu.Game.Tests.Visual.Playlists
         {
             AddUntilStep("wait for scores loaded", () =>
                 requestComplete
+                // request handler may need to fire more than once to get scores.
+                && totalCount > 0
                 && resultsScreen.ScorePanelList.GetScorePanels().Count() == totalCount
                 && resultsScreen.ScorePanelList.AllPanelsVisible);
             AddWaitStep("wait for display", 5);

osu.Game/Database/OnlineLookupCache.cs

@@ -91,7 +91,7 @@ namespace osu.Game.Database
             }
         }
 
-        private void performLookup()
+        private async Task performLookup()
         {
             // contains at most 50 unique IDs from tasks, which is used to perform the lookup.
             var nextTaskBatch = new Dictionary<TLookup, List<TaskCompletionSource<TValue>>>();
@@ -127,7 +127,7 @@ namespace osu.Game.Database
 
             // rather than queueing, we maintain our own single-threaded request stream.
             // todo: we probably want retry logic here.
-            api.Perform(request);
+            await api.PerformAsync(request).ConfigureAwait(false);
 
             finishPendingTask();
 

osu.Game/Online/API/DummyAPIAccess.cs

@@ -63,12 +63,13 @@ namespace osu.Game.Online.API
 
         public virtual void Queue(APIRequest request)
         {
-            if (HandleRequest?.Invoke(request) != true)
+            Schedule(() =>
             {
-                // this will fail due to not receiving an APIAccess, and trigger a failure on the request.
+                if (HandleRequest?.Invoke(request) != true)
-                // this is intended - any request in testing that needs non-failures should use HandleRequest.
+                {
-                request.Perform(this);
+                    request.Fail(new InvalidOperationException($@"{nameof(DummyAPIAccess)} cannot process this request."));
-            }
+                }
+            });
         }
 
         public void Perform(APIRequest request) => HandleRequest?.Invoke(request);

osu.Game/Tests/Visual/OnlinePlay/OnlinePlayTestScene.cs

@@ -2,6 +2,7 @@
 // See the LICENCE file in the repository root for full licence text.
 
 using System;
+using System.Threading.Tasks;
 using NUnit.Framework;
 using osu.Framework.Allocation;
 using osu.Framework.Bindables;
@@ -67,7 +68,25 @@ namespace osu.Game.Tests.Visual.OnlinePlay
             // To get around this, the BeatmapManager is looked up from the dependencies provided to the children of the test scene instead.
             var beatmapManager = dependencies.Get<BeatmapManager>();
 
-            ((DummyAPIAccess)API).HandleRequest = request => handler.HandleRequest(request, API.LocalUser.Value, beatmapManager);
+            ((DummyAPIAccess)API).HandleRequest = request =>
+            {
+                TaskCompletionSource<bool> tcs = new TaskCompletionSource<bool>();
+
+                // Because some of the handlers use realm, we need to ensure the game is still alive when firing.
+                // If we don't, a stray `PerformAsync` could hit an `ObjectDisposedException` if running too late.
+                Scheduler.Add(() =>
+                {
+                    bool result = handler.HandleRequest(request, API.LocalUser.Value, beatmapManager);
+                    tcs.SetResult(result);
+                }, false);
+
+#pragma warning disable RS0030
+                // We can't GetResultSafely() here (will fail with "Can't use GetResultSafely from inside an async operation."), but Wait is safe enough due to
+                // the task being a TaskCompletionSource.
+                // Importantly, this doesn't deadlock because of the scheduler call above running inline where feasible (see the `false` argument).
+                return tcs.Task.Result;
+#pragma warning restore RS0030
+            };
         });
 
         /// <summary>