From 45841673f6704da52cd689836950c8967e0cae57 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 30 May 2022 15:44:16 +0900 Subject: [PATCH 1/7] Update `OnlineLookupCache` to use async version of `Perform` call --- osu.Game/Database/OnlineLookupCache.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/osu.Game/Database/OnlineLookupCache.cs b/osu.Game/Database/OnlineLookupCache.cs index 2f98aef58a..506103a2c0 100644 --- a/osu.Game/Database/OnlineLookupCache.cs +++ b/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>>(); @@ -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(); From a2a057440e87e5f494a8248f07f851e3a505d460 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 30 May 2022 16:05:32 +0900 Subject: [PATCH 2/7] Fail requests taretting the fake API with a more deliberate exception I think this feels better than relying on some other method to throw an exception. --- osu.Game/Online/API/DummyAPIAccess.cs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/osu.Game/Online/API/DummyAPIAccess.cs b/osu.Game/Online/API/DummyAPIAccess.cs index f292e95bd1..8614cc79cb 100644 --- a/osu.Game/Online/API/DummyAPIAccess.cs +++ b/osu.Game/Online/API/DummyAPIAccess.cs @@ -65,9 +65,7 @@ namespace osu.Game.Online.API { if (HandleRequest?.Invoke(request) != true) { - // this will fail due to not receiving an APIAccess, and trigger a failure on the request. - // 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.")); } } From c18dd8c8fb29fb84c9459a3bed7bb58e429e257e Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 30 May 2022 16:32:44 +0900 Subject: [PATCH 3/7] Ensure `Queue` operations on `DummyAPIAccess` are performed on the update thread --- osu.Game/Online/API/DummyAPIAccess.cs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/osu.Game/Online/API/DummyAPIAccess.cs b/osu.Game/Online/API/DummyAPIAccess.cs index 8614cc79cb..d3707d977c 100644 --- a/osu.Game/Online/API/DummyAPIAccess.cs +++ b/osu.Game/Online/API/DummyAPIAccess.cs @@ -63,10 +63,13 @@ namespace osu.Game.Online.API public virtual void Queue(APIRequest request) { - if (HandleRequest?.Invoke(request) != true) + Schedule(() => { - request.Fail(new InvalidOperationException($@"{nameof(DummyAPIAccess)} cannot process this request.")); - } + if (HandleRequest?.Invoke(request) != true) + { + request.Fail(new InvalidOperationException($@"{nameof(DummyAPIAccess)} cannot process this request.")); + } + }); } public void Perform(APIRequest request) => HandleRequest?.Invoke(request); From f935f034c2a573bb5002b11d5f70b0987b2786a1 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 30 May 2022 16:33:07 +0900 Subject: [PATCH 4/7] Ensure request handling for `OnlinePlayTestScene` runs in a scheduled fashion --- .../Visual/OnlinePlay/OnlinePlayTestScene.cs | 20 ++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/osu.Game/Tests/Visual/OnlinePlay/OnlinePlayTestScene.cs b/osu.Game/Tests/Visual/OnlinePlay/OnlinePlayTestScene.cs index b6a347a896..32e42a945d 100644 --- a/osu.Game/Tests/Visual/OnlinePlay/OnlinePlayTestScene.cs +++ b/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,24 @@ 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(); - ((DummyAPIAccess)API).HandleRequest = request => handler.HandleRequest(request, API.LocalUser.Value, beatmapManager); + ((DummyAPIAccess)API).HandleRequest = request => + { + TaskCompletionSource tcs = new TaskCompletionSource(); + + // 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. + return tcs.Task.Result; +#pragma warning restore RS0030 + }; }); /// From 0981d415a1fe5ed295b21049cc451f5817b36e68 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 30 May 2022 17:57:48 +0900 Subject: [PATCH 5/7] Select correct channel regardless of load order --- osu.Game.Tests/Visual/Online/TestSceneChatOverlay.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/osu.Game.Tests/Visual/Online/TestSceneChatOverlay.cs b/osu.Game.Tests/Visual/Online/TestSceneChatOverlay.cs index 4edbb9f215..2cf1114f30 100644 --- a/osu.Game.Tests/Visual/Online/TestSceneChatOverlay.cs +++ b/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(); From 17174a7b099b753a2d6c8e7c90754f2060c8f163 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 30 May 2022 19:29:13 +0900 Subject: [PATCH 6/7] Ensure some results have been loaded in playlist results screen tests Basically, the failing delayed test would fire two web requests during the proceedings. In unfortunate timing, the first would succeed and the test would think "everything is okay", but the actual request loading results has not yet run. This check ensures *something* is loaded, which seems to be enough to make things reliable. --- .../Visual/Playlists/TestScenePlaylistsResultsScreen.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/osu.Game.Tests/Visual/Playlists/TestScenePlaylistsResultsScreen.cs b/osu.Game.Tests/Visual/Playlists/TestScenePlaylistsResultsScreen.cs index f5fe00458a..c532e8bc05 100644 --- a/osu.Game.Tests/Visual/Playlists/TestScenePlaylistsResultsScreen.cs +++ b/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); From 477e520766ef2924e4bbc37ab5cb33cf085debfe Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 31 May 2022 12:24:44 +0900 Subject: [PATCH 7/7] Add comment regarding deadlock avoidance --- osu.Game/Tests/Visual/OnlinePlay/OnlinePlayTestScene.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/osu.Game/Tests/Visual/OnlinePlay/OnlinePlayTestScene.cs b/osu.Game/Tests/Visual/OnlinePlay/OnlinePlayTestScene.cs index 32e42a945d..df3974664e 100644 --- a/osu.Game/Tests/Visual/OnlinePlay/OnlinePlayTestScene.cs +++ b/osu.Game/Tests/Visual/OnlinePlay/OnlinePlayTestScene.cs @@ -83,6 +83,7 @@ namespace osu.Game.Tests.Visual.OnlinePlay #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 };