Merge pull request #13932 from peppy/fix-multiple-request-completion-states

Fix API request potentially firing failed events after completion

osu.Game/Online/API/APIException.cs

@@ -0,0 +1,15 @@
+// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. 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)
+        {
+        }
+    }
+}

osu.Game/Online/API/APIRequest.cs

@@ -79,7 +79,13 @@ namespace osu.Game.Online.API
         /// </summary>
         public event APIFailureHandler Failure;
 
-        private bool cancelled;
+        private readonly object completionStateLock = new object();
+
+        /// <summary>
+        /// The state of this request, from an outside perspective.
+        /// This is used to ensure correct notification events are fired.
+        /// </summary>
+        private APIRequestCompletionState completionState;
 
         private Action pendingFailure;
 
@@ -116,12 +122,7 @@ namespace osu.Game.Online.API
 
             PostProcess();
 
-            API.Schedule(delegate
+            API.Schedule(TriggerSuccess);
-            {
-                if (cancelled) return;
-
-                TriggerSuccess();
-            });
         }
 
         /// <summary>
@@ -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)
+            lock (completionStateLock)
-                return;
+            {
+                // 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
         /// <returns>Whether we are in a failed or cancelled state.</returns>
         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();

osu.Game/Online/API/APIRequestCompletionState.cs

@@ -0,0 +1,23 @@
+// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. 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
+    {
+        /// <summary>
+        /// Not yet run or currently waiting on response.
+        /// </summary>
+        Waiting,
+
+        /// <summary>
+        /// Ran to completion.
+        /// </summary>
+        Completed,
+
+        /// <summary>
+        /// Cancelled or failed due to error.
+        /// </summary>
+        Failed
+    }
+}

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;
         }