Merge pull request #3872 from peppy/simplify-api-requests

Remove all retry logic and simplify overall handling of API requests

osu.Game/Online/API/APIAccess.cs

@@ -102,7 +102,7 @@ namespace osu.Game.Online.API
                         if (queue.Count == 0)
                         {
                             log.Add(@"Queueing a ping request");
-                            Queue(new ListChannelsRequest { Timeout = 5000 });
+                            Queue(new GetUserRequest());
                         }
 
                         break;
@@ -141,7 +141,7 @@ namespace osu.Game.Online.API
                             State = APIState.Online;
                         };
 
-                        if (!handleRequest(userReq, out _))
+                        if (!handleRequest(userReq))
                         {
                             Thread.Sleep(500);
                             continue;
@@ -170,15 +170,10 @@ namespace osu.Game.Online.API
                     lock (queue)
                     {
                         if (queue.Count == 0) break;
-                        req = queue.Peek();
+                        req = queue.Dequeue();
                     }
 
-                    // TODO: handle failures better
+                    handleRequest(req);
-                    handleRequest(req, out var removeFromQueue);
-
-                    if (removeFromQueue)
-                        lock (queue)
-                            queue.Dequeue();
                 }
 
                 Thread.Sleep(50);
@@ -195,46 +190,29 @@ namespace osu.Game.Online.API
 
         /// <summary>
         /// Handle a single API request.
+        /// Ensures all exceptions are caught and dealt with correctly.
         /// </summary>
         /// <param name="req">The request.</param>
         /// <returns>true if the request succeeded.</returns>
-        private bool handleRequest(APIRequest req, out bool removeFromQueue)
+        private bool handleRequest(APIRequest req)
         {
-            removeFromQueue = true;
-
             try
             {
-                Logger.Log($@"Performing request {req}", LoggingTarget.Network);
-                req.Failure += ex =>
-                {
-                    if (ex is WebException we)
-                        handleWebException(we);
-                };
-
                 req.Perform(this);
 
                 //we could still be in initialisation, at which point we don't want to say we're Online yet.
-                if (IsLoggedIn)
+                if (IsLoggedIn) State = APIState.Online;
-                    State = APIState.Online;
 
                 failureCount = 0;
                 return true;
             }
             catch (WebException we)
             {
-                removeFromQueue = handleWebException(we);
+                handleWebException(we);
-
-                if (removeFromQueue)
-                    req.Fail(we);
-
                 return false;
             }
             catch (Exception e)
             {
-                if (e is TimeoutException)
-                    log.Add(@"API level timeout exception was hit");
-
-                req.Fail(e);
                 return false;
             }
         }
@@ -290,8 +268,12 @@ namespace osu.Game.Online.API
                         //we might try again at an api level.
                         return false;
 
-                    State = APIState.Failing;
+                    if (State == APIState.Online)
-                    flushQueue();
+                    {
+                        State = APIState.Failing;
+                        flushQueue();
+                    }
+
                     return true;
             }
 

osu.Game/Online/API/APIRequest.cs

@@ -3,6 +3,7 @@
 
 using System;
 using osu.Framework.IO.Network;
+using osu.Framework.Logging;
 
 namespace osu.Game.Online.API
 {
@@ -35,23 +36,12 @@ namespace osu.Game.Online.API
     /// </summary>
     public abstract class APIRequest
     {
-        /// <summary>
-        /// The maximum amount of time before this request will fail.
-        /// </summary>
-        public int Timeout = WebRequest.DEFAULT_TIMEOUT;
-
         protected virtual string Target => string.Empty;
 
         protected virtual WebRequest CreateWebRequest() => new WebRequest(Uri);
 
         protected virtual string Uri => $@"{API.Endpoint}/api/v2/{Target}";
 
-        private double remainingTime => Math.Max(0, Timeout - (DateTimeOffset.UtcNow - (startTime ?? DateTimeOffset.MinValue)).TotalMilliseconds);
-
-        public bool ExceededTimeout => remainingTime == 0;
-
-        private DateTimeOffset? startTime;
-
         protected APIAccess API;
         protected WebRequest WebRequest;
 
@@ -75,27 +65,24 @@ namespace osu.Game.Online.API
         {
             API = api;
 
-            if (checkAndProcessFailure())
+            if (checkAndScheduleFailure())
                 return;
 
-            if (startTime == null)
-                startTime = DateTimeOffset.UtcNow;
-
-            if (remainingTime <= 0)
-                throw new TimeoutException(@"API request timeout hit");
-
             WebRequest = CreateWebRequest();
             WebRequest.Failed += Fail;
             WebRequest.AllowRetryOnTimeout = false;
             WebRequest.AddHeader("Authorization", $"Bearer {api.AccessToken}");
 
-            if (checkAndProcessFailure())
+            if (checkAndScheduleFailure())
                 return;
 
             if (!WebRequest.Aborted) //could have been aborted by a Cancel() call
+            {
+                Logger.Log($@"Performing request {this}", LoggingTarget.Network);
                 WebRequest.Perform();
+            }
 
-            if (checkAndProcessFailure())
+            if (checkAndScheduleFailure())
                 return;
 
             api.Schedule(delegate { Success?.Invoke(); });
@@ -105,19 +92,21 @@ namespace osu.Game.Online.API
 
         public void Fail(Exception e)
         {
-            cancelled = true;
+            if (cancelled) return;
 
+            cancelled = true;
             WebRequest?.Abort();
 
+            Logger.Log($@"Failing request {this} ({e})", LoggingTarget.Network);
             pendingFailure = () => Failure?.Invoke(e);
-            checkAndProcessFailure();
+            checkAndScheduleFailure();
         }
 
         /// <summary>
         /// Checked for cancellation or error. Also queues up the Failed event if we can.
         /// </summary>
         /// <returns>Whether we are in a failed or cancelled state.</returns>
-        private bool checkAndProcessFailure()
+        private bool checkAndScheduleFailure()
         {
             if (API == null || pendingFailure == null) return cancelled;
 

osu.Game/osu.Game.csproj

@@ -18,7 +18,7 @@
     <PackageReference Include="Microsoft.EntityFrameworkCore.Sqlite" Version="2.1.4" />
     <PackageReference Include="Microsoft.EntityFrameworkCore.Sqlite.Core" Version="2.2.0" />
     <PackageReference Include="Newtonsoft.Json" Version="12.0.1" />
-    <PackageReference Include="ppy.osu.Framework" Version="2018.1214.0" />
+    <PackageReference Include="ppy.osu.Framework" Version="2018.1219.0" />
     <PackageReference Include="SharpCompress" Version="0.22.0" />
     <PackageReference Include="NUnit" Version="3.11.0" />
     <PackageReference Include="SharpRaven" Version="2.4.0" />