Send notifications about score processing completion to clients #157

bdach · 2022-12-20T23:14:23Z

Continuation of Show changes to user statistics in-game osu#18877
Depends on Add score processed callback to spectator client osu#21738
Depends on game package bump
Soft-depends on Ensure score submission completion before notifying spectator server when exiting play early osu#21753 (for correct delivery of notifications for aborted scores)

Note: This is an RFC. It has been tested very lightly. The happy path seems to work, but I'm not sure about the multitude of sad paths. I'm PRing this now to get it out for comments and see how many WTFs I get, as I'm not confident about almost everything in this diff, so I wouldn't necessarily advise to hurry this one up too much.

Testing procedure, for whoever is inclined:

Begin with a working full-stack osu + osu-web + osu-server-spectator setup
In osu, check out pull mentioned above
In osu-server-spectator, switch to local checkout
Set up & launch osu-queue-score-statistics so that it can pick up scores placed in redis by osu-web
At this point messages will flow, but there will be no visible effect game-side. To make one happen, the following crude patch can be applied:

patch to apply to game

diff --git a/osu.Game/OsuGame.cs b/osu.Game/OsuGame.cs
index a0a45e18a8..45c94b73d2 100644
--- a/osu.Game/OsuGame.cs
+++ b/osu.Game/OsuGame.cs
@@ -990,6 +990,11 @@ protected override void LoadComplete()
 
             // Importantly, this should be run after binding PostNotification to the import handlers so they can present the import after game startup.
             handleStartupImport();
+
+            SpectatorClient.OnUserScoreProcessed += (userId, scoreId) =>
+            {
+                Notifications.Post(new SimpleNotification { Text = $"OnUserScoreProcessed (userId: {userId}, scoreId: {scoreId})" });
+            };
         }
 
         private void handleStartupImport()
diff --git a/osu.Game/OsuGameBase.cs b/osu.Game/OsuGameBase.cs
index 7586dc6407..468bc61696 100644
--- a/osu.Game/OsuGameBase.cs
+++ b/osu.Game/OsuGameBase.cs
@@ -188,7 +188,7 @@ public virtual string Version
 
         private RulesetConfigCache rulesetConfigCache;
 
-        private SpectatorClient spectatorClient;
+        protected SpectatorClient SpectatorClient { get; private set; }
 
         protected MultiplayerClient MultiplayerClient { get; private set; }
 
@@ -298,7 +298,7 @@ private void load(ReadableKeyCombinationProvider keyCombinationProvider)
 
             // TODO: OsuGame or OsuGameBase?
             dependencies.CacheAs(beatmapUpdater = new BeatmapUpdater(BeatmapManager, difficultyCache, API, Storage));
-            dependencies.CacheAs(spectatorClient = new OnlineSpectatorClient(endpoints));
+            dependencies.CacheAs(SpectatorClient = new OnlineSpectatorClient(endpoints));
             dependencies.CacheAs(MultiplayerClient = new OnlineMultiplayerClient(endpoints));
             dependencies.CacheAs(metadataClient = new OnlineMetadataClient(endpoints));
 
@@ -343,7 +343,7 @@ private void load(ReadableKeyCombinationProvider keyCombinationProvider)
             if (API is APIAccess apiAccess)
                 AddInternal(apiAccess);
 
-            AddInternal(spectatorClient);
+            AddInternal(SpectatorClient);
             AddInternal(MultiplayerClient);
             AddInternal(metadataClient);

At this point, every completed play should result in a notification being fired. Aborted plays won't always fire the notification. I'm not sure why yet, I'm going to look into that a bit more still tomorrow if this approach doesn't just get dumpstered immediately.

I'll be adding some self-commentary review comments shortly.

bdach · 2022-12-20T23:16:00Z

osu.Server.Spectator/Hubs/ScoreProcessedSubscriber.cs

+        }
+    }
+
+    private void purgeTimedOutSubscriptions()


I've added this entire flow to avoid a situation where the subscriber gets backlogged due to the score processor not being able to catch up to incoming scores and accumulates too many subscriptions, causing a cascading failure. I'm not sure how realistic of a scenario this is in practice.

Going to have to see how this plays out post deploy really.

bdach · 2022-12-20T23:16:46Z

osu.Server.Spectator/Hubs/ScoreProcessedSubscriber.cs

+            if (subscriptions.TryRemove(scoreProcessed.ScoreId, out var subscription))
+            {
+                using (subscription)
+                    subscription.InvokeAsync().Wait();


The .Wait() is unfortunate, but I'm not sure how to avoid it.

Probably fine.

osu.Server.Spectator/Hubs/SpectatorHub.cs

As it is undefined behaviour: dotnet/AspNetCore.Docs#6888 `IHubContext<SpectatorClient>` can be used instead.

bdach · 2022-12-21T22:19:16Z

Aborted plays won't always fire the notification. I'm not sure why yet

I've figured out what this was; see ppy/osu#21753 for explanation and proposed fix.

The class name caught me off-guard and I thought this was doing something extra/weird.

…nspection

peppy · 2022-12-26T05:55:36Z

osu.Server.Spectator/Hubs/ScoreProcessedSubscriber.cs

+    /// <summary>
+    /// The maximum amount of time to wait for a <see cref="ScoreProcessed"/> message for a given score in milliseconds.
+    /// </summary>
+    private const int timeout_interval_ms = 30_000;


We'll see how this plays out. I have a feeling going higher than this will be what we want eventually (minutes at least) to allow for potential server-side issues/outages.

smoogipoo · 2022-12-26T06:36:52Z

osu.Server.Spectator/Hubs/ScoreProcessedSubscriber.cs

+
+        foreach (var scoreId in scoreIds)
+        {
+            if (subscriptions.TryGetValue(scoreId, out var subscription) && subscription.TimedOut)


One thing to watch out for is that CancellationTokenSource.Token throws an exception if the CTS has been disposed. There could theoretically be a small moment where this TryGetValue() retrieves the value on this thread, but then the using (subscription) above runs and only after that does the subscription.TimedOut check happen.

I was aware of this, but when I checked the implementation, then I didn't expect an IsCancellationRequested check to throw after disposal. Maybe bad form relying on implementation details.

No you're correct, I didn't know there was such a discrepancy between Token and IsCancellationRequested

bdach added 5 commits December 20, 2022 21:32

Register Redis connection multiplexer in DI

900c437

Implement new spectator client method in sample client

b25fca2

Add subscriber interface for score processing completions

7372460

Implement score processed subscriber

1d94f26

Register for score processed notifications on finished play

4347884

bdach commented Dec 20, 2022

View reviewed changes

bdach added 2 commits December 21, 2022 00:23

Fix silly errors in sample spectator client implementation

80078ce

Do not capture hub instance in callback

15346db

As it is undefined behaviour: dotnet/AspNetCore.Docs#6888 `IHubContext<SpectatorClient>` can be used instead.

This was referenced Dec 22, 2022

Add solo statistics watcher component to deliver incremental global user statistics updates ppy/osu#21762

Merged

Add overall ranking display to solo results screen ppy/osu#21778

Merged

peppy self-requested a review December 25, 2022 14:38

peppy added 4 commits December 25, 2022 23:57

Update dependencies

7fdc6a4

Merge branch 'master' into score-processed-notification

b3a7ccf

Add mention of the fact that IConnectionMultiplexer is redis in xmldoc

96290cf

The class name caught me off-guard and I thought this was doing something extra/weird.

Pass through cancellation token in MetadataBroadcaster to fix new i…

b7bf31d

…nspection

peppy reviewed Dec 26, 2022

View reviewed changes

peppy approved these changes Dec 26, 2022

View reviewed changes

peppy merged commit 162ac6a into ppy:master Dec 26, 2022

smoogipoo reviewed Dec 26, 2022

View reviewed changes

bdach deleted the score-processed-notification branch December 26, 2022 07:40

peppy mentioned this pull request Dec 26, 2022

Show changes to user statistics in-game ppy/osu#18877

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Send notifications about score processing completion to clients #157

Send notifications about score processing completion to clients #157

bdach commented Dec 20, 2022 •

edited by peppy

Loading

bdach Dec 20, 2022 •

edited

Loading

peppy Dec 26, 2022

bdach Dec 20, 2022

peppy Dec 26, 2022

bdach commented Dec 21, 2022

peppy Dec 26, 2022

smoogipoo Dec 26, 2022

bdach Dec 26, 2022

smoogipoo Dec 26, 2022

Send notifications about score processing completion to clients #157

Send notifications about score processing completion to clients #157

Conversation

bdach commented Dec 20, 2022 • edited by peppy Loading

bdach Dec 20, 2022 • edited Loading

Choose a reason for hiding this comment

peppy Dec 26, 2022

Choose a reason for hiding this comment

bdach Dec 20, 2022

Choose a reason for hiding this comment

peppy Dec 26, 2022

Choose a reason for hiding this comment

bdach commented Dec 21, 2022

peppy Dec 26, 2022

Choose a reason for hiding this comment

smoogipoo Dec 26, 2022

Choose a reason for hiding this comment

bdach Dec 26, 2022

Choose a reason for hiding this comment

smoogipoo Dec 26, 2022

Choose a reason for hiding this comment

bdach commented Dec 20, 2022 •

edited by peppy

Loading

bdach Dec 20, 2022 •

edited

Loading