Always broadcast user presence to users' friends

[smoogipoo]

Supersedes #252
See client side changes in ppy/osu#31444

Note that this doesn't update when the user adds/removes friends. That's left as a future task for when we figure out a method to signal changes to/from osu-web (redis, polling the database, or otherwise).

[peppy]

Seems pretty logical

[peppy]

Looks mostly good

[peppy]

For this particular case, do we actually need a lock? Wondering if it can be avoided..

[smoogipoo]

Yes, because ItemUsage.Item enforces that the item must be locked:

osu-server-spectator/osu.Server.Spectator/Entities/EntityStore.cs

Lines 195 to 201 in 718fb5a

	public T? Item
	{
	get
	{
	checkValidForUse();
	return item;
	}

A deadlock concern is valid here, saved by the fact that EntityStore will continuously retry taking the lock over 5 seconds, before throwing a timeout exception. So I think it'll be fine after all.

I've tried to resolve this in a more "proper" way, and have come up with the following. I'm not sure if it's over-engineered (the hub part), and likely not over-engineered enough:

diff --git a/osu.Server.Spectator/Entities/EntityStore.cs b/osu.Server.Spectator/Entities/EntityStore.cs
index ebeb4dc..42b7af2 100644
--- a/osu.Server.Spectator/Entities/EntityStore.cs
+++ b/osu.Server.Spectator/Entities/EntityStore.cs
@@ -6,6 +6,7 @@ using System.Collections.Generic;
 using System.Linq;
 using System.Threading;
 using System.Threading.Tasks;
+using JetBrains.Annotations;
 using osu.Framework.Extensions.ObjectExtensions;
 using StatsdClient;
 
@@ -32,13 +33,19 @@ namespace osu.Server.Spectator.Entities
         {
             get
             {
-                lock (entityMapping)
+                using (GetRetrievalLock())
                     return entityMapping.Count;
             }
         }
 
         public string EntityName => typeof(T).Name;
 
+        /// <summary>
+        /// Locks this <see cref="EntityStore{T}"/> for retrieval of an entity.
+        /// </summary>
+        [MustDisposeResource]
+        public EntityStoreLock GetRetrievalLock() => new EntityStoreLock(entityMapping);
+
         /// <summary>
         /// Retrieves an entity.
         /// </summary>
@@ -49,7 +56,7 @@ namespace osu.Server.Spectator.Entities
         /// <returns>The entity.</returns>
         public T? GetEntityUnsafe(long id)
         {
-            lock (entityMapping)
+            using (GetRetrievalLock())
                 return !entityMapping.TryGetValue(id, out var entity) ? null : entity.GetItemUnsafe();
         }
 
@@ -68,7 +75,7 @@ namespace osu.Server.Spectator.Entities
             {
                 TrackedEntity? item;
 
-                lock (entityMapping)
+                using (GetRetrievalLock())
                 {
                     if (!entityMapping.TryGetValue(id, out item))
                     {
@@ -119,7 +126,7 @@ namespace osu.Server.Spectator.Entities
         {
             TrackedEntity? item;
 
-            lock (entityMapping)
+            using (GetRetrievalLock())
             {
                 if (!entityMapping.TryGetValue(id, out item))
                 {
@@ -147,7 +154,7 @@ namespace osu.Server.Spectator.Entities
         {
             TrackedEntity? item;
 
-            lock (entityMapping)
+            using (GetRetrievalLock())
             {
                 if (!entityMapping.TryGetValue(id, out item))
                     // was not tracking.
@@ -173,7 +180,7 @@ namespace osu.Server.Spectator.Entities
         /// </summary>
         public KeyValuePair<long, T>[] GetAllEntities()
         {
-            lock (entityMapping)
+            using (GetRetrievalLock())
             {
                 return entityMapping
                        .Where(kvp => kvp.Value.GetItemUnsafe() != null)
@@ -187,7 +194,7 @@ namespace osu.Server.Spectator.Entities
         /// </summary>
         public void Clear()
         {
-            lock (entityMapping)
+            using (GetRetrievalLock())
             {
                 entityMapping.Clear();
             }
@@ -195,7 +202,7 @@ namespace osu.Server.Spectator.Entities
 
         private void remove(long id)
         {
-            lock (entityMapping)
+            using (GetRetrievalLock())
             {
                 entityMapping.Remove(id);
 
@@ -290,5 +297,28 @@ namespace osu.Server.Spectator.Entities
                 if (shouldBeLocked && !isLocked) throw new InvalidOperationException("Attempted to access a tracked entity without holding a lock");
             }
         }
+
+        [MustDisposeResource]
+        public class EntityStoreLock : IDisposable
+        {
+            private readonly object lockObject;
+
+            public EntityStoreLock(object lockObject)
+            {
+                this.lockObject = lockObject;
+                Monitor.Enter(lockObject);
+            }
+
+            public void Dispose()
+            {
+                try
+                {
+                    Monitor.Exit(lockObject);
+                }
+                catch
+                {
+                }
+            }
+        }
     }
 }
diff --git a/osu.Server.Spectator/Hubs/Metadata/MetadataHub.cs b/osu.Server.Spectator/Hubs/Metadata/MetadataHub.cs
index 9a3e12c..65ddd0b 100644
--- a/osu.Server.Spectator/Hubs/Metadata/MetadataHub.cs
+++ b/osu.Server.Spectator/Hubs/Metadata/MetadataHub.cs
@@ -52,6 +52,8 @@ namespace osu.Server.Spectator.Hubs.Metadata
         {
             await base.OnConnectedAsync();
 
+            var friendUserIds = new List<int>();
+
             using (var usage = await GetOrCreateLocalUserState())
             {
                 string? versionHash = null;
@@ -76,16 +78,31 @@ namespace osu.Server.Spectator.Hubs.Metadata
                     foreach (int friendId in await db.GetUserFriendsAsync(usage.Item.UserId))
                     {
                         await Groups.AddToGroupAsync(Context.ConnectionId, FRIEND_PRESENCE_WATCHERS_GROUP(friendId));
-
-                        // Check if the friend is online, and if they are, broadcast to the connected user.
-                        using (var friendUsage = await TryGetStateFromUser(friendId))
-                        {
-                            if (friendUsage?.Item != null && shouldBroadcastPresenceToOtherUsers(friendUsage.Item))
-                                await Clients.Caller.FriendPresenceUpdated(friendId, friendUsage.Item.ToUserPresence());
-                        }
+                        friendUserIds.Add(friendId);
                     }
                 }
             }
+
+            // Broadcast online friend states to the newly connected user.
+
+            // We're using multiple user states here...
+            using (UserStates.GetRetrievalLock())
+            {
+                using var userUsage = await TryGetStateFromUser(Context.GetUserId());
+
+                if (userUsage?.Item == null)
+                    return;
+
+                foreach (int friendId in friendUserIds)
+                {
+                    using var friendUsage = await TryGetStateFromUser(friendId);
+
+                    if (friendUsage?.Item == null || !shouldBroadcastPresenceToOtherUsers(friendUsage.Item))
+                        continue;
+
+                    await Clients.Caller.FriendPresenceUpdated(friendId, friendUsage.Item.ToUserPresence());
+                }
+            }
         }
 
         private async Task logLogin(ItemUsage<MetadataClientState> usage)

[peppy]

Aha, I see.

Let's... save this for another time 😅 .

[peppy]

I guess clients are going to get double status updates for friends, but probably fine for now.