From bf6fb1c38048f8a7f44cfc3bc525cab22b56591b Mon Sep 17 00:00:00 2001
From: Dean Herbert <pe@ppy.sh>
Date: Sat, 1 Sep 2018 12:55:11 +0900
Subject: [PATCH] Don't use ConcurrentQueue for API

This queue type can hold several references to already dequeued requests. In our usage, this can cause old api calls to hold references to already-disposed screens (and in turn, very large memory portions).
---
 osu.Game/Online/API/APIAccess.cs | 56 +++++++++++++++++---------------
 1 file changed, 30 insertions(+), 26 deletions(-)

diff --git a/osu.Game/Online/API/APIAccess.cs b/osu.Game/Online/API/APIAccess.cs
index 12935a5ffe..ff5247e8f6 100644
--- a/osu.Game/Online/API/APIAccess.cs
+++ b/osu.Game/Online/API/APIAccess.cs
@@ -2,7 +2,6 @@
 // Licensed under the MIT Licence - https://raw.githubusercontent.com/ppy/osu/master/LICENCE
 
 using System;
-using System.Collections.Concurrent;
 using System.Collections.Generic;
 using System.Diagnostics;
 using System.Net;
@@ -26,7 +25,7 @@ namespace osu.Game.Online.API
         private const string client_id = @"5";
         private const string client_secret = @"FGc9GAtyHzeQDshWP5Ah7dega8hJACAJpQtw6OXk";
 
-        private ConcurrentQueue<APIRequest> queue = new ConcurrentQueue<APIRequest>();
+        private readonly Queue<APIRequest> queue = new Queue<APIRequest>();
 
         /// <summary>
         /// The username/email provided by the user when initiating a login.
@@ -75,10 +74,7 @@ namespace osu.Game.Online.API
 
         public void Unregister(IOnlineComponent component)
         {
-            Scheduler.Add(delegate
-            {
-                components.Remove(component);
-            });
+            Scheduler.Add(delegate { components.Remove(component); });
         }
 
         public string AccessToken => authentication.RequestAccessToken();
@@ -103,6 +99,7 @@ namespace osu.Game.Online.API
                             log.Add(@"Queueing a ping request");
                             Queue(new ListChannelsRequest { Timeout = 5000 });
                         }
+
                         break;
                     case APIState.Offline:
                     case APIState.Connecting:
@@ -161,18 +158,19 @@ namespace osu.Game.Online.API
                     continue;
                 }
 
-                //process the request queue.
-                APIRequest req;
-                while (queue.TryPeek(out req))
+                APIRequest req = null;
+
+                lock (queue)
+                    if (queue.Count > 0)
+                        req = queue.Dequeue();
+
+                if (req != null)
                 {
-                    if (handleRequest(req))
-                    {
-                        //we have succeeded, so let's unqueue.
-                        queue.TryDequeue(out req);
-                    }
+                    // TODO: handle failures better
+                    handleRequest(req);
                 }
 
-                Thread.Sleep(1);
+                Thread.Sleep(50);
             }
         }
 
@@ -205,7 +203,8 @@ namespace osu.Game.Online.API
             }
             catch (WebException we)
             {
-                HttpStatusCode statusCode = (we.Response as HttpWebResponse)?.StatusCode ?? (we.Status == WebExceptionStatus.UnknownError ? HttpStatusCode.NotAcceptable : HttpStatusCode.RequestTimeout);
+                HttpStatusCode statusCode = (we.Response as HttpWebResponse)?.StatusCode
+                                            ?? (we.Status == WebExceptionStatus.UnknownError ? HttpStatusCode.NotAcceptable : HttpStatusCode.RequestTimeout);
 
                 // special cases for un-typed but useful message responses.
                 switch (we.Message)
@@ -247,6 +246,7 @@ namespace osu.Game.Online.API
         }
 
         private APIState state;
+
         public APIState State
         {
             get { return state; }
@@ -271,7 +271,10 @@ namespace osu.Game.Online.API
 
         public bool IsLoggedIn => LocalUser.Value.Id > 1;
 
-        public void Queue(APIRequest request) => queue.Enqueue(request);
+        public void Queue(APIRequest request)
+        {
+            lock (queue) queue.Enqueue(request);
+        }
 
         public event StateChangeDelegate OnStateChange;
 
@@ -279,16 +282,17 @@ namespace osu.Game.Online.API
 
         private void flushQueue(bool failOldRequests = true)
         {
-            var oldQueue = queue;
-
-            //flush the queue.
-            queue = new ConcurrentQueue<APIRequest>();
-
-            if (failOldRequests)
+            lock (queue)
             {
-                APIRequest req;
-                while (oldQueue.TryDequeue(out req))
-                    req.Fail(new WebException(@"Disconnected from server"));
+                var oldQueueRequests = queue.ToArray();
+
+                queue.Clear();
+
+                if (failOldRequests)
+                {
+                    foreach (var req in oldQueueRequests)
+                        req.Fail(new WebException(@"Disconnected from server"));
+                }
             }
         }