From 964976f604e9071e929f90cdd934ab104cf20bdb Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Mon, 25 Jan 2021 20:41:51 +0900 Subject: [PATCH] Use a task chain and fix potential misordering of events --- .../Online/Multiplayer/MultiplayerClient.cs | 11 +- .../Multiplayer/StatefulMultiplayerClient.cs | 123 +++++------------- .../Multiplayer/TestMultiplayerClient.cs | 2 + osu.Game/Utils/TaskChain.cs | 30 +++++ 4 files changed, 70 insertions(+), 96 deletions(-) create mode 100644 osu.Game/Utils/TaskChain.cs diff --git a/osu.Game/Online/Multiplayer/MultiplayerClient.cs b/osu.Game/Online/Multiplayer/MultiplayerClient.cs index 5d18521eac..8573adc94a 100644 --- a/osu.Game/Online/Multiplayer/MultiplayerClient.cs +++ b/osu.Game/Online/Multiplayer/MultiplayerClient.cs @@ -134,17 +134,12 @@ protected override Task JoinRoom(long roomId) return connection.InvokeAsync(nameof(IMultiplayerServer.JoinRoom), roomId); } - public override async Task LeaveRoom() + protected override Task LeaveRoomInternal() { if (!isConnected.Value) - { - // even if not connected, make sure the local room state can be cleaned up. - await base.LeaveRoom(); - return; - } + return Task.FromCanceled(new CancellationToken(true)); - await base.LeaveRoom(); - await connection.InvokeAsync(nameof(IMultiplayerServer.LeaveRoom)); + return connection.InvokeAsync(nameof(IMultiplayerServer.LeaveRoom)); } public override Task TransferHost(int userId) diff --git a/osu.Game/Online/Multiplayer/StatefulMultiplayerClient.cs b/osu.Game/Online/Multiplayer/StatefulMultiplayerClient.cs index f2b5a44fcf..94122aeff5 100644 --- a/osu.Game/Online/Multiplayer/StatefulMultiplayerClient.cs +++ b/osu.Game/Online/Multiplayer/StatefulMultiplayerClient.cs @@ -108,67 +108,38 @@ protected StatefulMultiplayerClient() }); } - private readonly object joinOrLeaveTaskLock = new object(); - private Task? joinOrLeaveTask; + private readonly TaskChain joinOrLeaveTaskChain = new TaskChain(); /// /// Joins the for a given API . /// /// The API . - public async Task JoinRoom(Room room) + public async Task JoinRoom(Room room) => await joinOrLeaveTaskChain.Add(async () => { - Task? lastTask; - Task newTask; + if (Room != null) + 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; - joinOrLeaveTask = newTask = Task.Run(async () => - { - if (lastTask != null) - await lastTask; + Room = joinedRoom; + apiRoom = room; + playlistItemId = room.Playlist.SingleOrDefault()?.ID ?? 0; + }); - // Should be thread-safe since joinOrLeaveTask is locked on in both JoinRoom() and LeaveRoom(). - if (Room != null) - 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; - } - } - } + // Update room settings. + await updateLocalRoomSettings(joinedRoom.Settings); + }); /// /// Joins the with a given ID. @@ -177,48 +148,24 @@ await scheduleAsync(() => /// The joined . protected abstract Task JoinRoom(long roomId); - public virtual async Task LeaveRoom() + public async Task LeaveRoom() => await joinOrLeaveTaskChain.Add(async () => { - Task? lastTask; - Task newTask; + if (Room == null) + return; - lock (joinOrLeaveTaskLock) + await scheduleAsync(() => { - lastTask = joinOrLeaveTask; - joinOrLeaveTask = newTask = Task.Run(async () => - { - if (lastTask != null) - await lastTask; + apiRoom = null; + Room = null; + CurrentMatchPlayingUserIds.Clear(); - // Should be thread-safe since joinOrLeaveTask is locked on in both JoinRoom() and LeaveRoom(). - if (Room == null) - return; + RoomUpdated?.Invoke(); + }); - await scheduleAsync(() => - { - apiRoom = null; - Room = null; - CurrentMatchPlayingUserIds.Clear(); + await LeaveRoomInternal(); + }); - RoomUpdated?.Invoke(); - }); - }); - } - - 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; - } - } - } + protected abstract Task LeaveRoomInternal(); /// /// Change the current settings. diff --git a/osu.Game/Tests/Visual/Multiplayer/TestMultiplayerClient.cs b/osu.Game/Tests/Visual/Multiplayer/TestMultiplayerClient.cs index 7fbc770351..a79183fdab 100644 --- a/osu.Game/Tests/Visual/Multiplayer/TestMultiplayerClient.cs +++ b/osu.Game/Tests/Visual/Multiplayer/TestMultiplayerClient.cs @@ -98,6 +98,8 @@ protected override Task JoinRoom(long roomId) 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) diff --git a/osu.Game/Utils/TaskChain.cs b/osu.Game/Utils/TaskChain.cs new file mode 100644 index 0000000000..b397b0c45b --- /dev/null +++ b/osu.Game/Utils/TaskChain.cs @@ -0,0 +1,30 @@ +// Copyright (c) ppy Pty Ltd . 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 +{ + /// + /// A chain of s that run sequentially. + /// + public class TaskChain + { + private readonly object currentTaskLock = new object(); + private Task? currentTask; + + public Task Add(Func taskFunc) + { + lock (currentTaskLock) + { + currentTask = currentTask == null + ? taskFunc() + : currentTask.ContinueWith(_ => taskFunc()).Unwrap(); + return currentTask; + } + } + } +}