Use a task chain and fix potential misordering of events

osu.Game/Online/Multiplayer/MultiplayerClient.cs

@@ -134,17 +134,12 @@ namespace osu.Game.Online.Multiplayer
             return connection.InvokeAsync<MultiplayerRoom>(nameof(IMultiplayerServer.JoinRoom), roomId);
         }
 
-        public override async Task LeaveRoom()
+        protected override Task LeaveRoomInternal()
         {
             if (!isConnected.Value)
-            {
+                return Task.FromCanceled(new CancellationToken(true));
-                // even if not connected, make sure the local room state can be cleaned up.
-                await base.LeaveRoom();
-                return;
-            }
 
-            await base.LeaveRoom();
+            return connection.InvokeAsync(nameof(IMultiplayerServer.LeaveRoom));
-            await connection.InvokeAsync(nameof(IMultiplayerServer.LeaveRoom));
         }
 
         public override Task TransferHost(int userId)

osu.Game/Online/Multiplayer/StatefulMultiplayerClient.cs

@@ -108,67 +108,38 @@ namespace osu.Game.Online.Multiplayer
             });
         }
 
-        private readonly object joinOrLeaveTaskLock = new object();
+        private readonly TaskChain joinOrLeaveTaskChain = new TaskChain();
-        private Task? joinOrLeaveTask;
 
         /// <summary>
         /// Joins the <see cref="MultiplayerRoom"/> for a given API <see cref="Room"/>.
         /// </summary>
         /// <param name="room">The API <see cref="Room"/>.</param>
-        public async Task JoinRoom(Room room)
+        public async Task JoinRoom(Room room) => await joinOrLeaveTaskChain.Add(async () =>
         {
-            Task? lastTask;
+            if (Room != null)
-            Task newTask;
+                throw new InvalidOperationException("Cannot join a multiplayer room while already in one.");
 
-            lock (joinOrLeaveTaskLock)
+            Debug.Assert(room.RoomID.Value != null);
+
+            // Join the server-side room.
+            var joinedRoom = await JoinRoom(room.RoomID.Value.Value);
+            Debug.Assert(joinedRoom != null);
+
+            // Populate users.
+            Debug.Assert(joinedRoom.Users != null);
+            await Task.WhenAll(joinedRoom.Users.Select(PopulateUser));
+
+            // Update the stored room (must be done on update thread for thread-safety).
+            await scheduleAsync(() =>
             {
-                lastTask = joinOrLeaveTask;
+                Room = joinedRoom;
-                joinOrLeaveTask = newTask = Task.Run(async () =>
+                apiRoom = room;
-                {
+                playlistItemId = room.Playlist.SingleOrDefault()?.ID ?? 0;
-                    if (lastTask != null)
+            });
-                        await lastTask;
 
-                    // Should be thread-safe since joinOrLeaveTask is locked on in both JoinRoom() and LeaveRoom().
+            // Update room settings.
-                    if (Room != null)
+            await updateLocalRoomSettings(joinedRoom.Settings);
-                        throw new InvalidOperationException("Cannot join a multiplayer room while already in one.");
+        });
-
-                    Debug.Assert(room.RoomID.Value != null);
-
-                    // Join the server-side room.
-                    var joinedRoom = await JoinRoom(room.RoomID.Value.Value);
-                    Debug.Assert(joinedRoom != null);
-
-                    // Populate users.
-                    Debug.Assert(joinedRoom.Users != null);
-                    await Task.WhenAll(joinedRoom.Users.Select(PopulateUser));
-
-                    // Update the stored room (must be done on update thread for thread-safety).
-                    await scheduleAsync(() =>
-                    {
-                        Room = joinedRoom;
-                        apiRoom = room;
-                        playlistItemId = room.Playlist.SingleOrDefault()?.ID ?? 0;
-                    });
-
-                    // Update room settings.
-                    await updateLocalRoomSettings(joinedRoom.Settings);
-                });
-            }
-
-            try
-            {
-                await newTask;
-            }
-            finally
-            {
-                // The task will be awaited in the future, so reset it so that the user doesn't get into a permanently faulted state if anything fails.
-                lock (joinOrLeaveTaskLock)
-                {
-                    if (joinOrLeaveTask == newTask)
-                        joinOrLeaveTask = null;
-                }
-            }
-        }
 
         /// <summary>
         /// Joins the <see cref="MultiplayerRoom"/> with a given ID.
@@ -177,48 +148,24 @@ namespace osu.Game.Online.Multiplayer
         /// <returns>The joined <see cref="MultiplayerRoom"/>.</returns>
         protected abstract Task<MultiplayerRoom> JoinRoom(long roomId);
 
-        public virtual async Task LeaveRoom()
+        public async Task LeaveRoom() => await joinOrLeaveTaskChain.Add(async () =>
         {
-            Task? lastTask;
+            if (Room == null)
-            Task newTask;
+                return;
 
-            lock (joinOrLeaveTaskLock)
+            await scheduleAsync(() =>
             {
-                lastTask = joinOrLeaveTask;
+                apiRoom = null;
-                joinOrLeaveTask = newTask = Task.Run(async () =>
+                Room = null;
-                {
+                CurrentMatchPlayingUserIds.Clear();
-                    if (lastTask != null)
-                        await lastTask;
 
-                    // Should be thread-safe since joinOrLeaveTask is locked on in both JoinRoom() and LeaveRoom().
+                RoomUpdated?.Invoke();
-                    if (Room == null)
+            });
-                        return;
 
-                    await scheduleAsync(() =>
+            await LeaveRoomInternal();
-                    {
+        });
-                        apiRoom = null;
-                        Room = null;
-                        CurrentMatchPlayingUserIds.Clear();
 
-                        RoomUpdated?.Invoke();
+        protected abstract Task LeaveRoomInternal();
-                    });
-                });
-            }
-
-            try
-            {
-                await newTask;
-            }
-            finally
-            {
-                // The task will be awaited in the future, so reset it so that the user doesn't get into a permanently faulted state if anything fails.
-                lock (joinOrLeaveTaskLock)
-                {
-                    if (joinOrLeaveTask == newTask)
-                        joinOrLeaveTask = null;
-                }
-            }
-        }
 
         /// <summary>
         /// Change the current <see cref="MultiplayerRoom"/> settings.

osu.Game/Tests/Visual/Multiplayer/TestMultiplayerClient.cs

@@ -98,6 +98,8 @@ namespace osu.Game.Tests.Visual.Multiplayer
             return Task.FromResult(room);
         }
 
+        protected override Task LeaveRoomInternal() => Task.CompletedTask;
+
         public override Task TransferHost(int userId) => ((IMultiplayerClient)this).HostChanged(userId);
 
         public override async Task ChangeSettings(MultiplayerRoomSettings settings)

osu.Game/Utils/TaskChain.cs

@@ -0,0 +1,30 @@
+// 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 enable
+
+using System;
+using System.Threading.Tasks;
+
+namespace osu.Game.Utils
+{
+    /// <summary>
+    /// A chain of <see cref="Task"/>s that run sequentially.
+    /// </summary>
+    public class TaskChain
+    {
+        private readonly object currentTaskLock = new object();
+        private Task? currentTask;
+
+        public Task Add(Func<Task> taskFunc)
+        {
+            lock (currentTaskLock)
+            {
+                currentTask = currentTask == null
+                    ? taskFunc()
+                    : currentTask.ContinueWith(_ => taskFunc()).Unwrap();
+                return currentTask;
+            }
+        }
+    }
+}