From 5836f497ac13d168ab077946a67f8d349079794f Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 30 Aug 2024 18:03:30 +0900 Subject: [PATCH 1/6] Provide API context earlier to api requests in order to fix missing schedules Closes https://github.com/ppy/osu/issues/29546. --- osu.Game/Online/API/APIAccess.cs | 8 +++++-- osu.Game/Online/API/APIRequest.cs | 37 +++++++++++++++++++++---------- 2 files changed, 31 insertions(+), 14 deletions(-) diff --git a/osu.Game/Online/API/APIAccess.cs b/osu.Game/Online/API/APIAccess.cs index 716d1e4466..a9ad561163 100644 --- a/osu.Game/Online/API/APIAccess.cs +++ b/osu.Game/Online/API/APIAccess.cs @@ -385,7 +385,8 @@ namespace osu.Game.Online.API { try { - request.Perform(this); + request.AttachAPI(this); + request.Perform(); } catch (Exception e) { @@ -483,7 +484,8 @@ namespace osu.Game.Online.API { try { - req.Perform(this); + req.AttachAPI(this); + req.Perform(); if (req.CompletionState != APIRequestCompletionState.Completed) return false; @@ -568,6 +570,8 @@ namespace osu.Game.Online.API { lock (queue) { + request.AttachAPI(this); + if (state.Value == APIState.Offline) { request.Fail(new WebException(@"User not logged in")); diff --git a/osu.Game/Online/API/APIRequest.cs b/osu.Game/Online/API/APIRequest.cs index 6b6b222043..d062b8f3de 100644 --- a/osu.Game/Online/API/APIRequest.cs +++ b/osu.Game/Online/API/APIRequest.cs @@ -4,6 +4,7 @@ #nullable disable using System; +using System.Diagnostics; using System.Globalization; using JetBrains.Annotations; using Newtonsoft.Json; @@ -74,6 +75,7 @@ namespace osu.Game.Online.API protected virtual string Uri => $@"{API.APIEndpointUrl}/api/v2/{Target}"; protected APIAccess API; + protected WebRequest WebRequest; /// @@ -101,16 +103,29 @@ namespace osu.Game.Online.API /// public APIRequestCompletionState CompletionState { get; private set; } - public void Perform(IAPIProvider api) + /// + /// Should be called before to give API context. + /// + /// + /// This allows scheduling of operations back to the correct thread (which may be required before is called). + /// + public void AttachAPI(APIAccess apiAccess) { - if (!(api is APIAccess apiAccess)) + if (API != null && API != apiAccess) + throw new InvalidOperationException("Attached API cannot be changed after initial set."); + + API = apiAccess; + } + + public void Perform() + { + if (API == null) { Fail(new NotSupportedException($"A {nameof(APIAccess)} is required to perform requests.")); return; } - API = apiAccess; - User = apiAccess.LocalUser.Value; + User = API.LocalUser.Value; if (isFailing) return; @@ -153,6 +168,8 @@ namespace osu.Game.Online.API internal void TriggerSuccess() { + Debug.Assert(API != null); + lock (completionStateLock) { if (CompletionState != APIRequestCompletionState.Waiting) @@ -161,14 +178,13 @@ namespace osu.Game.Online.API CompletionState = APIRequestCompletionState.Completed; } - if (API == null) - Success?.Invoke(); - else - API.Schedule(() => Success?.Invoke()); + API.Schedule(() => Success?.Invoke()); } internal void TriggerFailure(Exception e) { + Debug.Assert(API != null); + lock (completionStateLock) { if (CompletionState != APIRequestCompletionState.Waiting) @@ -177,10 +193,7 @@ namespace osu.Game.Online.API CompletionState = APIRequestCompletionState.Failed; } - if (API == null) - Failure?.Invoke(e); - else - API.Schedule(() => Failure?.Invoke(e)); + API.Schedule(() => Failure?.Invoke(e)); } public void Cancel() => Fail(new OperationCanceledException(@"Request cancelled")); From 07611bd8f5f30617a78649cc9eb513c89844a552 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 30 Aug 2024 18:10:33 +0900 Subject: [PATCH 2/6] Use `IAPIProvider` interface and correctly support scheduling from `DummyAPIAccess` --- osu.Game/Online/API/APIAccess.cs | 2 +- osu.Game/Online/API/APIRequest.cs | 4 ++-- osu.Game/Online/API/DummyAPIAccess.cs | 13 ++++++++++++- osu.Game/Online/API/IAPIProvider.cs | 5 +++++ 4 files changed, 20 insertions(+), 4 deletions(-) diff --git a/osu.Game/Online/API/APIAccess.cs b/osu.Game/Online/API/APIAccess.cs index a9ad561163..a9ccbf9b18 100644 --- a/osu.Game/Online/API/APIAccess.cs +++ b/osu.Game/Online/API/APIAccess.cs @@ -159,7 +159,7 @@ namespace osu.Game.Online.API private void onTokenChanged(ValueChangedEvent e) => config.SetValue(OsuSetting.Token, config.Get(OsuSetting.SavePassword) ? authentication.TokenString : string.Empty); - internal new void Schedule(Action action) => base.Schedule(action); + void IAPIProvider.Schedule(Action action) => base.Schedule(action); public string AccessToken => authentication.RequestAccessToken(); diff --git a/osu.Game/Online/API/APIRequest.cs b/osu.Game/Online/API/APIRequest.cs index d062b8f3de..37ad5fff0e 100644 --- a/osu.Game/Online/API/APIRequest.cs +++ b/osu.Game/Online/API/APIRequest.cs @@ -74,7 +74,7 @@ namespace osu.Game.Online.API protected virtual string Uri => $@"{API.APIEndpointUrl}/api/v2/{Target}"; - protected APIAccess API; + protected IAPIProvider API; protected WebRequest WebRequest; @@ -109,7 +109,7 @@ namespace osu.Game.Online.API /// /// This allows scheduling of operations back to the correct thread (which may be required before is called). /// - public void AttachAPI(APIAccess apiAccess) + public void AttachAPI(IAPIProvider apiAccess) { if (API != null && API != apiAccess) throw new InvalidOperationException("Attached API cannot be changed after initial set."); diff --git a/osu.Game/Online/API/DummyAPIAccess.cs b/osu.Game/Online/API/DummyAPIAccess.cs index 0af76537cd..7ac5c45fad 100644 --- a/osu.Game/Online/API/DummyAPIAccess.cs +++ b/osu.Game/Online/API/DummyAPIAccess.cs @@ -82,6 +82,8 @@ namespace osu.Game.Online.API public virtual void Queue(APIRequest request) { + request.AttachAPI(this); + Schedule(() => { if (HandleRequest?.Invoke(request) != true) @@ -98,10 +100,17 @@ namespace osu.Game.Online.API }); } - public void Perform(APIRequest request) => HandleRequest?.Invoke(request); + void IAPIProvider.Schedule(Action action) => base.Schedule(action); + + public void Perform(APIRequest request) + { + request.AttachAPI(this); + HandleRequest?.Invoke(request); + } public Task PerformAsync(APIRequest request) { + request.AttachAPI(this); HandleRequest?.Invoke(request); return Task.CompletedTask; } @@ -155,6 +164,8 @@ namespace osu.Game.Online.API state.Value = APIState.Connecting; LastLoginError = null; + request.AttachAPI(this); + // if no handler installed / handler can't handle verification, just assume that the server would verify for simplicity. if (HandleRequest?.Invoke(request) != true) onSuccessfulLogin(); diff --git a/osu.Game/Online/API/IAPIProvider.cs b/osu.Game/Online/API/IAPIProvider.cs index d8194dc32b..eccfb36546 100644 --- a/osu.Game/Online/API/IAPIProvider.cs +++ b/osu.Game/Online/API/IAPIProvider.cs @@ -134,6 +134,11 @@ namespace osu.Game.Online.API /// void UpdateStatistics(UserStatistics newStatistics); + /// + /// Schedule a callback to run on the update thread. + /// + internal void Schedule(Action action); + /// /// Constructs a new . May be null if not supported. /// From dd7133657dbe57c3aa99ef8266b52ca6bebf62a9 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 30 Aug 2024 18:14:10 +0900 Subject: [PATCH 3/6] Fix weird test critical failure if exception happens too early in execution Noticed in passing. ``` Exit code is 134 (Unhandled exception. System.NullReferenceException: Object reference not set to an instance of an object. at osu.Game.OsuGameBase.onExceptionThrown(Exception ex) in /Users/dean/Projects/osu/osu.Game/OsuGameBase.cs:line 695 at osu.Framework.Platform.GameHost.abortExecutionFromException(Object sender, Exception exception, Boolean isTerminating) at osu.Framework.Platform.GameHost.unobservedExceptionHandler(Object sender, UnobservedTaskExceptionEventArgs args) at System.Threading.Tasks.TaskExceptionHolder.Finalize()) ``` --- osu.Game/OsuGameBase.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/OsuGameBase.cs b/osu.Game/OsuGameBase.cs index 1988a06503..ce0c288934 100644 --- a/osu.Game/OsuGameBase.cs +++ b/osu.Game/OsuGameBase.cs @@ -692,7 +692,7 @@ namespace osu.Game if (Interlocked.Decrement(ref allowableExceptions) < 0) { Logger.Log("Too many unhandled exceptions, crashing out."); - RulesetStore.TryDisableCustomRulesetsCausing(ex); + RulesetStore?.TryDisableCustomRulesetsCausing(ex); return false; } From 2d745fb67e9210ae4eb7d0b5a702729ca4ac8ce3 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 30 Aug 2024 18:21:30 +0900 Subject: [PATCH 4/6] Apply NRT to `APIRequest` --- osu.Game/Online/API/APIRequest.cs | 26 ++++++++----------- .../Online/API/Requests/JoinChannelRequest.cs | 2 +- .../API/Requests/LeaveChannelRequest.cs | 2 +- osu.Game/Online/Chat/WebSocketChatClient.cs | 2 +- osu.Game/Online/Rooms/JoinRoomRequest.cs | 2 +- osu.Game/Online/Rooms/PartRoomRequest.cs | 2 +- osu.Game/Tests/PollingChatClient.cs | 2 +- 7 files changed, 17 insertions(+), 21 deletions(-) diff --git a/osu.Game/Online/API/APIRequest.cs b/osu.Game/Online/API/APIRequest.cs index 37ad5fff0e..45ebbcd76d 100644 --- a/osu.Game/Online/API/APIRequest.cs +++ b/osu.Game/Online/API/APIRequest.cs @@ -1,12 +1,9 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. -#nullable disable - using System; using System.Diagnostics; using System.Globalization; -using JetBrains.Annotations; using Newtonsoft.Json; using osu.Framework.Extensions.TypeExtensions; using osu.Framework.IO.Network; @@ -27,18 +24,17 @@ namespace osu.Game.Online.API /// /// The deserialised response object. May be null if the request or deserialisation failed. /// - [CanBeNull] - public T Response { get; private set; } + public T? Response { get; private set; } /// /// Invoked on successful completion of an API request. /// This will be scheduled to the API's internal scheduler (run on update thread automatically). /// - public new event APISuccessHandler Success; + public new event APISuccessHandler? Success; protected APIRequest() { - base.Success += () => Success?.Invoke(Response); + base.Success += () => Success?.Invoke(Response!); } protected override void PostProcess() @@ -72,28 +68,28 @@ namespace osu.Game.Online.API protected virtual WebRequest CreateWebRequest() => new OsuWebRequest(Uri); - protected virtual string Uri => $@"{API.APIEndpointUrl}/api/v2/{Target}"; + protected virtual string Uri => $@"{API!.APIEndpointUrl}/api/v2/{Target}"; - protected IAPIProvider API; + protected IAPIProvider? API; - protected WebRequest WebRequest; + protected WebRequest? WebRequest; /// /// The currently logged in user. Note that this will only be populated during . /// - protected APIUser User { get; private set; } + protected APIUser? User { get; private set; } /// /// Invoked on successful completion of an API request. /// This will be scheduled to the API's internal scheduler (run on update thread automatically). /// - public event APISuccessHandler Success; + public event APISuccessHandler? Success; /// /// Invoked on failure to complete an API request. /// This will be scheduled to the API's internal scheduler (run on update thread automatically). /// - public event APIFailureHandler Failure; + public event APIFailureHandler? Failure; private readonly object completionStateLock = new object(); @@ -210,7 +206,7 @@ namespace osu.Game.Online.API // in the case of a cancellation we don't care about whether there's an error in the response. if (!(e is OperationCanceledException)) { - string responseString = WebRequest?.GetResponseString(); + string? responseString = WebRequest?.GetResponseString(); // naive check whether there's an error in the response to avoid unnecessary JSON deserialisation. if (!string.IsNullOrEmpty(responseString) && responseString.Contains(@"""error""")) @@ -248,7 +244,7 @@ namespace osu.Game.Online.API private class DisplayableError { [JsonProperty("error")] - public string ErrorMessage { get; set; } + public string ErrorMessage { get; set; } = string.Empty; } } diff --git a/osu.Game/Online/API/Requests/JoinChannelRequest.cs b/osu.Game/Online/API/Requests/JoinChannelRequest.cs index 33eab7e355..0109e653d9 100644 --- a/osu.Game/Online/API/Requests/JoinChannelRequest.cs +++ b/osu.Game/Online/API/Requests/JoinChannelRequest.cs @@ -23,6 +23,6 @@ namespace osu.Game.Online.API.Requests return req; } - protected override string Target => $@"chat/channels/{channel.Id}/users/{User.Id}"; + protected override string Target => $@"chat/channels/{channel.Id}/users/{User!.Id}"; } } diff --git a/osu.Game/Online/API/Requests/LeaveChannelRequest.cs b/osu.Game/Online/API/Requests/LeaveChannelRequest.cs index 7dfc9a0aed..36cfd79c60 100644 --- a/osu.Game/Online/API/Requests/LeaveChannelRequest.cs +++ b/osu.Game/Online/API/Requests/LeaveChannelRequest.cs @@ -23,6 +23,6 @@ namespace osu.Game.Online.API.Requests return req; } - protected override string Target => $@"chat/channels/{channel.Id}/users/{User.Id}"; + protected override string Target => $@"chat/channels/{channel.Id}/users/{User!.Id}"; } } diff --git a/osu.Game/Online/Chat/WebSocketChatClient.cs b/osu.Game/Online/Chat/WebSocketChatClient.cs index 37774a1f5d..a74f0222f2 100644 --- a/osu.Game/Online/Chat/WebSocketChatClient.cs +++ b/osu.Game/Online/Chat/WebSocketChatClient.cs @@ -80,7 +80,7 @@ namespace osu.Game.Online.Chat fetchReq.Success += updates => { - if (updates?.Presence != null) + if (updates.Presence != null) { foreach (var channel in updates.Presence) joinChannel(channel); diff --git a/osu.Game/Online/Rooms/JoinRoomRequest.cs b/osu.Game/Online/Rooms/JoinRoomRequest.cs index 8645f2a2c0..9a73104b60 100644 --- a/osu.Game/Online/Rooms/JoinRoomRequest.cs +++ b/osu.Game/Online/Rooms/JoinRoomRequest.cs @@ -27,6 +27,6 @@ namespace osu.Game.Online.Rooms return req; } - protected override string Target => $@"rooms/{Room.RoomID.Value}/users/{User.Id}"; + protected override string Target => $@"rooms/{Room.RoomID.Value}/users/{User!.Id}"; } } diff --git a/osu.Game/Online/Rooms/PartRoomRequest.cs b/osu.Game/Online/Rooms/PartRoomRequest.cs index 09ba6f65c3..2416833a1e 100644 --- a/osu.Game/Online/Rooms/PartRoomRequest.cs +++ b/osu.Game/Online/Rooms/PartRoomRequest.cs @@ -23,6 +23,6 @@ namespace osu.Game.Online.Rooms return req; } - protected override string Target => $"rooms/{room.RoomID.Value}/users/{User.Id}"; + protected override string Target => $"rooms/{room.RoomID.Value}/users/{User!.Id}"; } } diff --git a/osu.Game/Tests/PollingChatClient.cs b/osu.Game/Tests/PollingChatClient.cs index eb29b35c1d..75975c716b 100644 --- a/osu.Game/Tests/PollingChatClient.cs +++ b/osu.Game/Tests/PollingChatClient.cs @@ -48,7 +48,7 @@ namespace osu.Game.Tests fetchReq.Success += updates => { - if (updates?.Presence != null) + if (updates.Presence != null) { foreach (var channel in updates.Presence) handleChannelJoined(channel); From 8ffd4aa82c5e1afd4b4fe56773d6043248b54a12 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Fri, 30 Aug 2024 13:41:34 +0200 Subject: [PATCH 5/6] Fix NRT inspections --- .../TestSceneOnlinePlayBeatmapAvailabilityTracker.cs | 2 +- osu.Game/Online/API/APIDownloadRequest.cs | 7 ++++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/osu.Game.Tests/Online/TestSceneOnlinePlayBeatmapAvailabilityTracker.cs b/osu.Game.Tests/Online/TestSceneOnlinePlayBeatmapAvailabilityTracker.cs index 585fd516bd..ae3451c3e0 100644 --- a/osu.Game.Tests/Online/TestSceneOnlinePlayBeatmapAvailabilityTracker.cs +++ b/osu.Game.Tests/Online/TestSceneOnlinePlayBeatmapAvailabilityTracker.cs @@ -257,7 +257,7 @@ namespace osu.Game.Tests.Online { } - protected override string Target => null; + protected override string Target => string.Empty; } } } diff --git a/osu.Game/Online/API/APIDownloadRequest.cs b/osu.Game/Online/API/APIDownloadRequest.cs index c48372278a..f8db52139d 100644 --- a/osu.Game/Online/API/APIDownloadRequest.cs +++ b/osu.Game/Online/API/APIDownloadRequest.cs @@ -4,6 +4,7 @@ #nullable disable using System; +using System.Diagnostics; using System.IO; using osu.Framework.IO.Network; @@ -34,7 +35,11 @@ namespace osu.Game.Online.API return request; } - private void request_Progress(long current, long total) => API.Schedule(() => Progressed?.Invoke(current, total)); + private void request_Progress(long current, long total) + { + Debug.Assert(API != null); + API.Schedule(() => Progressed?.Invoke(current, total)); + } protected void TriggerSuccess(string filename) { From 8b04455c29fd41dfab49974f8883acb7ef60d8fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Fri, 30 Aug 2024 14:57:15 +0200 Subject: [PATCH 6/6] Fix chat overlay tests Not entirely sure why they were failing previously, but the most likely explanation is that by freak accident some mock requests would previously execute immediately rather than be scheduled on the API thread, which would change execution ordering and ensure that `ChannelManager.CurrentChannel` would become the joined channel, rather than remaining at the channel listing. --- osu.Game.Tests/Visual/Online/TestSceneChatOverlay.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/osu.Game.Tests/Visual/Online/TestSceneChatOverlay.cs b/osu.Game.Tests/Visual/Online/TestSceneChatOverlay.cs index a47205094e..b6445dec6b 100644 --- a/osu.Game.Tests/Visual/Online/TestSceneChatOverlay.cs +++ b/osu.Game.Tests/Visual/Online/TestSceneChatOverlay.cs @@ -446,7 +446,7 @@ namespace osu.Game.Tests.Visual.Online { AddStep("Show overlay with channel 1", () => { - channelManager.JoinChannel(testChannel1); + channelManager.CurrentChannel.Value = channelManager.JoinChannel(testChannel1); chatOverlay.Show(); }); waitForChannel1Visible(); @@ -462,7 +462,7 @@ namespace osu.Game.Tests.Visual.Online { AddStep("Show overlay with channel 1", () => { - channelManager.JoinChannel(testChannel1); + channelManager.CurrentChannel.Value = channelManager.JoinChannel(testChannel1); chatOverlay.Show(); }); waitForChannel1Visible();