Remove locking from SpectatorScreen

osu.Game/Screens/Spectate/SpectatorScreen.cs

@@ -42,9 +42,6 @@ namespace osu.Game.Screens.Spectate
         [Resolved]
         private UserLookupCache userLookupCache { get; set; }
 
-        // A lock is used to synchronise access to spectator/gameplay states, since this class is a screen which may become non-current and stop receiving updates at any point.
-        private readonly object stateLock = new object();
-
         private readonly Dictionary<int, User> userMap = new Dictionary<int, User>();
         private readonly Dictionary<int, GameplayState> gameplayStates = new Dictionary<int, GameplayState>();
 
@@ -63,8 +60,11 @@ namespace osu.Game.Screens.Spectate
         {
             base.LoadComplete();
 
-            populateAllUsers().ContinueWith(_ => Schedule(() =>
+            getAllUsers().ContinueWith(users => Schedule(() =>
             {
+                foreach (var u in users.Result)
+                    userMap[u.Id] = u;
+
                 spectatorClient.BindUserBeganPlaying(userBeganPlaying, true);
                 spectatorClient.OnUserFinishedPlaying += userFinishedPlaying;
                 spectatorClient.OnNewFrames += userSentFrames;
@@ -72,27 +72,23 @@ namespace osu.Game.Screens.Spectate
                 managerUpdated = beatmaps.ItemUpdated.GetBoundCopy();
                 managerUpdated.BindValueChanged(beatmapUpdated);
 
-                lock (stateLock)
-                {
                 foreach (var (id, _) in userMap)
                     spectatorClient.WatchUser(id);
-                }
             }));
         }
 
-        private Task populateAllUsers()
+        private Task<User[]> getAllUsers()
         {
-            var userLookupTasks = new List<Task>();
+            var userLookupTasks = new List<Task<User>>();
 
             foreach (var u in userIds)
             {
                 userLookupTasks.Add(userLookupCache.GetUserAsync(u).ContinueWith(task =>
                 {
                     if (!task.IsCompletedSuccessfully)
-                        return;
+                        return null;
 
-                    lock (stateLock)
+                    return task.Result;
-                        userMap[u] = task.Result;
                 }));
             }
 
@@ -104,8 +100,6 @@ namespace osu.Game.Screens.Spectate
             if (!e.NewValue.TryGetTarget(out var beatmapSet))
                 return;
 
-            lock (stateLock)
-            {
             foreach (var (userId, _) in userMap)
             {
                 if (!spectatorClient.TryGetPlayingUserState(userId, out var userState))
@@ -115,15 +109,12 @@ namespace osu.Game.Screens.Spectate
                     updateGameplayState(userId);
             }
         }
-        }
 
         private void userBeganPlaying(int userId, SpectatorState state)
         {
             if (state.RulesetID == null || state.BeatmapID == null)
                 return;
 
-            lock (stateLock)
-            {
             if (!userMap.ContainsKey(userId))
                 return;
 
@@ -135,11 +126,8 @@ namespace osu.Game.Screens.Spectate
 
             updateGameplayState(userId);
         }
-        }
 
         private void updateGameplayState(int userId)
-        {
-            lock (stateLock)
         {
             Debug.Assert(userMap.ContainsKey(userId));
 
@@ -174,11 +162,8 @@ namespace osu.Game.Screens.Spectate
             gameplayStates[userId] = gameplayState;
             Schedule(() => StartGameplay(userId, gameplayState));
         }
-        }
 
         private void userSentFrames(int userId, FrameDataBundle bundle)
-        {
-            lock (stateLock)
         {
             if (!userMap.ContainsKey(userId))
                 return;
@@ -200,11 +185,8 @@ namespace osu.Game.Screens.Spectate
                 gameplayState.Score.Replay.Frames.Add(convertedFrame);
             }
         }
-        }
 
         private void userFinishedPlaying(int userId, SpectatorState state)
-        {
-            lock (stateLock)
         {
             if (!userMap.ContainsKey(userId))
                 return;
@@ -217,7 +199,6 @@ namespace osu.Game.Screens.Spectate
             gameplayStates.Remove(userId);
             Schedule(() => EndGameplay(userId));
         }
-        }
 
         /// <summary>
         /// Invoked when a spectated user's state has changed.
@@ -244,8 +225,6 @@ namespace osu.Game.Screens.Spectate
         /// </summary>
         /// <param name="userId">The user to stop spectating.</param>
         protected void RemoveUser(int userId)
-        {
-            lock (stateLock)
         {
             userFinishedPlaying(userId, null);
 
@@ -254,7 +233,6 @@ namespace osu.Game.Screens.Spectate
 
             spectatorClient.StopWatchingUser(userId);
         }
-        }
 
         protected override void Dispose(bool isDisposing)
         {
@@ -266,12 +244,9 @@ namespace osu.Game.Screens.Spectate
                 spectatorClient.OnUserFinishedPlaying -= userFinishedPlaying;
                 spectatorClient.OnNewFrames -= userSentFrames;
 
-                lock (stateLock)
-                {
                 foreach (var (userId, _) in userMap)
                     spectatorClient.StopWatchingUser(userId);
             }
-            }
 
             managerUpdated?.UnbindAll();
         }