Merge pull request #21871 from bdach/solo-statistics-watcher-reliability

Improve reliability of solo statistics watcher

osu.Game.Tests/Visual/Online/TestSceneSoloStatisticsWatcher.cs

@@ -35,6 +35,8 @@ public partial class TestSceneSoloStatisticsWatcher : OsuTestScene
         private Action<GetUsersRequest>? handleGetUsersRequest;
         private Action<GetUserRequest>? handleGetUserRequest;
 
+        private IDisposable? subscription;
+
         private readonly Dictionary<(int userId, string rulesetName), UserStatistics> serverSideStatistics = new Dictionary<(int userId, string rulesetName), UserStatistics>();
 
         [SetUpSteps]
@@ -246,6 +248,26 @@ public void TestIgnoredScoreUpdateIsMergedIntoNextOne()
             AddAssert("values after are correct", () => update!.After.TotalScore, () => Is.EqualTo(6_000_000));
         }
 
+        [Test]
+        public void TestStatisticsUpdateNotFiredAfterSubscriptionDisposal()
+        {
+            int userId = getUserId();
+            setUpUser(userId);
+
+            long scoreId = getScoreId();
+            var ruleset = new OsuRuleset().RulesetInfo;
+
+            SoloStatisticsUpdate? update = null;
+            registerForUpdates(scoreId, ruleset, receivedUpdate => update = receivedUpdate);
+            AddStep("unsubscribe", () => subscription!.Dispose());
+
+            feignScoreProcessing(userId, ruleset, 5_000_000);
+
+            AddStep("signal score processed", () => ((ISpectatorClient)spectatorClient).UserScoreProcessed(userId, scoreId));
+            AddWaitStep("wait a bit", 5);
+            AddAssert("update not received", () => update == null);
+        }
+
         private int nextUserId = 2000;
         private long nextScoreId = 50000;
 
@@ -266,7 +288,7 @@ private void setUpUser(int userId)
         }
 
         private void registerForUpdates(long scoreId, RulesetInfo rulesetInfo, Action<SoloStatisticsUpdate> onUpdateReady) =>
-            AddStep("register for updates", () => watcher.RegisterForStatisticsUpdateAfter(
+            AddStep("register for updates", () => subscription = watcher.RegisterForStatisticsUpdateAfter(
                 new ScoreInfo(Beatmap.Value.BeatmapInfo, new OsuRuleset().RulesetInfo, new RealmUser())
                 {
                     Ruleset = rulesetInfo,

osu.Game/Online/Solo/SoloStatisticsWatcher.cs

@@ -46,24 +46,30 @@ protected override void LoadComplete()
         /// </summary>
         /// <param name="score">The score to listen for the statistics update for.</param>
         /// <param name="onUpdateReady">The callback to be invoked once the statistics update has been prepared.</param>
-        public void RegisterForStatisticsUpdateAfter(ScoreInfo score, Action<SoloStatisticsUpdate> onUpdateReady) => Schedule(() =>
+        /// <returns>An <see cref="IDisposable"/> representing the subscription. Disposing it is equivalent to unsubscribing from future notifications.</returns>
+        public IDisposable RegisterForStatisticsUpdateAfter(ScoreInfo score, Action<SoloStatisticsUpdate> onUpdateReady)
         {
-            if (!api.IsLoggedIn)
-                return;
+            Schedule(() =>
+            {
+                if (!api.IsLoggedIn)
+                    return;
 
-            if (!score.Ruleset.IsLegacyRuleset())
-                return;
+                if (!score.Ruleset.IsLegacyRuleset() || score.OnlineID <= 0)
+                    return;
 
-            var callback = new StatisticsUpdateCallback(score, onUpdateReady);
+                var callback = new StatisticsUpdateCallback(score, onUpdateReady);
 
-            if (lastProcessedScoreId == score.OnlineID)
-            {
-                requestStatisticsUpdate(api.LocalUser.Value.Id, callback);
-                return;
-            }
+                if (lastProcessedScoreId == score.OnlineID)
+                {
+                    requestStatisticsUpdate(api.LocalUser.Value.Id, callback);
+                    return;
+                }
 
-            callbacks.Add(score.OnlineID, callback);
-        });
+                callbacks.Add(score.OnlineID, callback);
+            });
+
+            return new InvokeOnDisposal(() => Schedule(() => callbacks.Remove(score.OnlineID)));
+        }
 
         private void onUserChanged(APIUser? localUser) => Schedule(() =>
         {
@@ -75,13 +81,25 @@ private void onUserChanged(APIUser? localUser) => Schedule(() =>
                 return;
 
             var userRequest = new GetUsersRequest(new[] { localUser.OnlineID });
-            userRequest.Success += response => Schedule(() =>
+            userRequest.Success += initialiseUserStatistics;
+            api.Queue(userRequest);
+        });
+
+        private void initialiseUserStatistics(GetUsersResponse response) => Schedule(() =>
+        {
+            var user = response.Users.SingleOrDefault();
+
+            // possible if the user is restricted or similar.
+            if (user == null)
+                return;
+
+            latestStatistics = new Dictionary<string, UserStatistics>();
+
+            if (user.RulesetsStatistics != null)
             {
-                latestStatistics = new Dictionary<string, UserStatistics>();
-                foreach (var rulesetStats in response.Users.Single().RulesetsStatistics)
+                foreach (var rulesetStats in user.RulesetsStatistics)
                     latestStatistics.Add(rulesetStats.Key, rulesetStats.Value);
-            });
-            api.Queue(userRequest);
+            }
         });
 
         private void userScoreProcessed(int userId, long scoreId)

osu.Game/Screens/Ranking/SoloResultsScreen.cs

@@ -1,8 +1,6 @@
 // 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.
 
-#nullable disable
-
 using System;
 using System.Collections.Generic;
 using System.Linq;
@@ -26,15 +24,16 @@ public partial class SoloResultsScreen : ResultsScreen
         /// </summary>
         public bool ShowUserStatistics { get; init; }
 
-        private GetScoresRequest getScoreRequest;
+        private GetScoresRequest? getScoreRequest;
 
         [Resolved]
-        private RulesetStore rulesets { get; set; }
+        private RulesetStore rulesets { get; set; } = null!;
 
         [Resolved]
-        private SoloStatisticsWatcher soloStatisticsWatcher { get; set; }
+        private SoloStatisticsWatcher soloStatisticsWatcher { get; set; } = null!;
 
-        private readonly Bindable<SoloStatisticsUpdate> statisticsUpdate = new Bindable<SoloStatisticsUpdate>();
+        private IDisposable? statisticsSubscription;
+        private readonly Bindable<SoloStatisticsUpdate?> statisticsUpdate = new Bindable<SoloStatisticsUpdate?>();
 
         public SoloResultsScreen(ScoreInfo score, bool allowRetry)
             : base(score, allowRetry)
@@ -46,7 +45,7 @@ protected override void LoadComplete()
             base.LoadComplete();
 
             if (ShowUserStatistics)
-                soloStatisticsWatcher.RegisterForStatisticsUpdateAfter(Score, update => statisticsUpdate.Value = update);
+                statisticsSubscription = soloStatisticsWatcher.RegisterForStatisticsUpdateAfter(Score, update => statisticsUpdate.Value = update);
         }
 
         protected override StatisticsPanel CreateStatisticsPanel()
@@ -62,7 +61,7 @@ protected override StatisticsPanel CreateStatisticsPanel()
             return base.CreateStatisticsPanel();
         }
 
-        protected override APIRequest FetchScores(Action<IEnumerable<ScoreInfo>> scoresCallback)
+        protected override APIRequest? FetchScores(Action<IEnumerable<ScoreInfo>>? scoresCallback)
         {
             if (Score.BeatmapInfo.OnlineID <= 0 || Score.BeatmapInfo.Status <= BeatmapOnlineStatus.Pending)
                 return null;
@@ -77,6 +76,7 @@ protected override void Dispose(bool isDisposing)
             base.Dispose(isDisposing);
 
             getScoreRequest?.Cancel();
+            statisticsSubscription?.Dispose();
         }
     }
 }