From 80c2b1449bb2ae088104a37712b17829a32893d4 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 19 Jul 2021 20:27:00 +0900 Subject: [PATCH] Fix API request potentially firing failed events after completion Specifically, `Cancel()` calls were not thread safe. Due to a series of events, `ListPollingComponent` could call `Cancel` from a non-update thread, leading to a race condition where both a `Success` and `Fail` event can be fired. This is intended to be the simplest fix possible, locking and guarding specifically on the callbacks. Further work could be done in the future to improve the flow surrounding `pendingFailure`, potentially reducing redundant work and cleaning up the code, but that's not happening here. Closes https://github.com/ppy/osu/issues/13632. --- osu.Game/Online/API/APIException.cs | 15 +++++ osu.Game/Online/API/APIRequest.cs | 58 ++++++++++++------- .../Online/API/APIRequestCompletionState.cs | 23 ++++++++ osu.Game/Online/PollingComponent.cs | 2 +- 4 files changed, 75 insertions(+), 23 deletions(-) create mode 100644 osu.Game/Online/API/APIException.cs create mode 100644 osu.Game/Online/API/APIRequestCompletionState.cs diff --git a/osu.Game/Online/API/APIException.cs b/osu.Game/Online/API/APIException.cs new file mode 100644 index 0000000000..97786bced9 --- /dev/null +++ b/osu.Game/Online/API/APIException.cs @@ -0,0 +1,15 @@ +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. +// See the LICENCE file in the repository root for full licence text. + +using System; + +namespace osu.Game.Online.API +{ + public class APIException : InvalidOperationException + { + public APIException(string messsage, Exception innerException) + : base(messsage, innerException) + { + } + } +} diff --git a/osu.Game/Online/API/APIRequest.cs b/osu.Game/Online/API/APIRequest.cs index 1a6868cfa4..8d816d3975 100644 --- a/osu.Game/Online/API/APIRequest.cs +++ b/osu.Game/Online/API/APIRequest.cs @@ -79,7 +79,13 @@ namespace osu.Game.Online.API /// public event APIFailureHandler Failure; - private bool cancelled; + private readonly object completionStateLock = new object(); + + /// + /// The state of this request, from an outside perspective. + /// This is used to ensure correct notification events are fired. + /// + private APIRequestCompletionState completionState; private Action pendingFailure; @@ -116,12 +122,7 @@ namespace osu.Game.Online.API PostProcess(); - API.Schedule(delegate - { - if (cancelled) return; - - TriggerSuccess(); - }); + API.Schedule(TriggerSuccess); } /// @@ -131,16 +132,29 @@ namespace osu.Game.Online.API { } - private bool succeeded; - internal virtual void TriggerSuccess() { - succeeded = true; + lock (completionStateLock) + { + if (completionState != APIRequestCompletionState.Waiting) + return; + + completionState = APIRequestCompletionState.Completed; + } + Success?.Invoke(); } internal void TriggerFailure(Exception e) { + lock (completionStateLock) + { + if (completionState != APIRequestCompletionState.Waiting) + return; + + completionState = APIRequestCompletionState.Failed; + } + Failure?.Invoke(e); } @@ -148,10 +162,14 @@ namespace osu.Game.Online.API public void Fail(Exception e) { - if (succeeded || cancelled) - return; + lock (completionStateLock) + { + // while it doesn't matter if code following this check is run more than once, + // this avoids unnecessarily performing work where we are already sure the user has been informed. + if (completionState != APIRequestCompletionState.Waiting) + return; + } - cancelled = true; WebRequest?.Abort(); string responseString = WebRequest?.GetResponseString(); @@ -181,7 +199,11 @@ namespace osu.Game.Online.API /// Whether we are in a failed or cancelled state. private bool checkAndScheduleFailure() { - if (pendingFailure == null) return cancelled; + lock (completionStateLock) + { + if (pendingFailure == null) + return completionState == APIRequestCompletionState.Failed; + } if (API == null) pendingFailure(); @@ -199,14 +221,6 @@ namespace osu.Game.Online.API } } - public class APIException : InvalidOperationException - { - public APIException(string messsage, Exception innerException) - : base(messsage, innerException) - { - } - } - public delegate void APIFailureHandler(Exception e); public delegate void APISuccessHandler(); diff --git a/osu.Game/Online/API/APIRequestCompletionState.cs b/osu.Game/Online/API/APIRequestCompletionState.cs new file mode 100644 index 0000000000..84c9974dd8 --- /dev/null +++ b/osu.Game/Online/API/APIRequestCompletionState.cs @@ -0,0 +1,23 @@ +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. +// See the LICENCE file in the repository root for full licence text. + +namespace osu.Game.Online.API +{ + public enum APIRequestCompletionState + { + /// + /// Not yet run or currently waiting on response. + /// + Waiting, + + /// + /// Ran to completion. + /// + Completed, + + /// + /// Cancelled or failed due to error. + /// + Failed + } +} diff --git a/osu.Game/Online/PollingComponent.cs b/osu.Game/Online/PollingComponent.cs index 3d19f2ab09..806c0047e7 100644 --- a/osu.Game/Online/PollingComponent.cs +++ b/osu.Game/Online/PollingComponent.cs @@ -70,7 +70,7 @@ namespace osu.Game.Online return true; } - // not ennough time has passed since the last poll. we do want to schedule a poll to happen, though. + // not enough time has passed since the last poll. we do want to schedule a poll to happen, though. scheduleNextPoll(); return false; }