From bf6fb1c38048f8a7f44cfc3bc525cab22b56591b Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Sat, 1 Sep 2018 12:55:11 +0900 Subject: [PATCH 1/3] 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 queue = new ConcurrentQueue(); + private readonly Queue queue = new Queue(); /// /// 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(); - - 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")); + } } } From 562a792a9957e92ba82c78f0e3fff1b25428f20b Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Sat, 1 Sep 2018 13:00:55 +0900 Subject: [PATCH 2/3] Use thread instead of LongRunning for API --- osu.Game/Online/API/APIAccess.cs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/osu.Game/Online/API/APIAccess.cs b/osu.Game/Online/API/APIAccess.cs index ff5247e8f6..eb9a60115f 100644 --- a/osu.Game/Online/API/APIAccess.cs +++ b/osu.Game/Online/API/APIAccess.cs @@ -6,7 +6,6 @@ using System.Collections.Generic; using System.Diagnostics; using System.Net; using System.Threading; -using System.Threading.Tasks; using osu.Framework.Configuration; using osu.Framework.Graphics; using osu.Framework.Logging; @@ -54,7 +53,13 @@ namespace osu.Game.Online.API authentication.TokenString = config.Get(OsuSetting.Token); authentication.Token.ValueChanged += onTokenChanged; - Task.Factory.StartNew(run, cancellationToken.Token, TaskCreationOptions.LongRunning, TaskScheduler.Default); + var thread = new Thread(run) + { + Name = "APIAccess", + IsBackground = true + }; + + thread.Start(); } private void onTokenChanged(OAuthToken token) => config.Set(OsuSetting.Token, config.Get(OsuSetting.SavePassword) ? authentication.TokenString : string.Empty); From d05cd52d9aecc3e2cc3bf888291e413167bda555 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 6 Sep 2018 17:38:15 +0900 Subject: [PATCH 3/3] Run the queue faster if multiple requests are pending --- osu.Game/Online/API/APIAccess.cs | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/osu.Game/Online/API/APIAccess.cs b/osu.Game/Online/API/APIAccess.cs index eb9a60115f..1dda257c95 100644 --- a/osu.Game/Online/API/APIAccess.cs +++ b/osu.Game/Online/API/APIAccess.cs @@ -163,14 +163,16 @@ namespace osu.Game.Online.API continue; } - APIRequest req = null; - - lock (queue) - if (queue.Count > 0) - req = queue.Dequeue(); - - if (req != null) + while (true) { + APIRequest req; + + lock (queue) + { + if (queue.Count == 0) break; + req = queue.Dequeue(); + } + // TODO: handle failures better handleRequest(req); }