Merge pull request #11565 from peppy/fix-multiplayer-client-connection-reliability

Rewrite connection logic to better handle failure cases

osu.Game/Online/Multiplayer/MultiplayerClient.cs

@@ -5,7 +5,6 @@
 
 using System;
 using System.Collections.Generic;
-using System.Diagnostics;
 using System.Threading;
 using System.Threading.Tasks;
 using Microsoft.AspNetCore.SignalR.Client;
@@ -27,11 +26,15 @@ namespace osu.Game.Online.Multiplayer
         private readonly Bindable<bool> isConnected = new Bindable<bool>();
         private readonly IBindable<APIState> apiState = new Bindable<APIState>();
 
+        private readonly SemaphoreSlim connectionLock = new SemaphoreSlim(1);
+
         [Resolved]
         private IAPIProvider api { get; set; } = null!;
 
         private HubConnection? connection;
 
+        private CancellationTokenSource connectCancelSource = new CancellationTokenSource();
+
         private readonly string endpoint;
 
         public MultiplayerClient(EndpointConfiguration endpoints)
@@ -52,20 +55,21 @@ namespace osu.Game.Online.Multiplayer
             {
                 case APIState.Failing:
                 case APIState.Offline:
-                    connection?.StopAsync();
+                    Task.Run(() => disconnect(true));
-                    connection = null;
                     break;
 
                 case APIState.Online:
-                    Task.Run(Connect);
+                    Task.Run(connect);
                     break;
             }
         }
 
-        protected virtual async Task Connect()
+        private async Task connect()
         {
-            if (connection != null)
+            cancelExistingConnect();
-                return;
+
+            if (!await connectionLock.WaitAsync(10000))
+                throw new TimeoutException("Could not obtain a lock to connect. A previous attempt is likely stuck.");
 
             var builder = new HubConnectionBuilder()
                 .WithUrl(endpoint, options => { options.Headers.Add("Authorization", $"Bearer {api.AccessToken}"); });
@@ -81,58 +85,51 @@ namespace osu.Game.Online.Multiplayer
 
             connection = builder.Build();
 
-            // this is kind of SILLY
-            // https://github.com/dotnet/aspnetcore/issues/15198
-            connection.On<MultiplayerRoomState>(nameof(IMultiplayerClient.RoomStateChanged), ((IMultiplayerClient)this).RoomStateChanged);
-            connection.On<MultiplayerRoomUser>(nameof(IMultiplayerClient.UserJoined), ((IMultiplayerClient)this).UserJoined);
-            connection.On<MultiplayerRoomUser>(nameof(IMultiplayerClient.UserLeft), ((IMultiplayerClient)this).UserLeft);
-            connection.On<int>(nameof(IMultiplayerClient.HostChanged), ((IMultiplayerClient)this).HostChanged);
-            connection.On<MultiplayerRoomSettings>(nameof(IMultiplayerClient.SettingsChanged), ((IMultiplayerClient)this).SettingsChanged);
-            connection.On<int, MultiplayerUserState>(nameof(IMultiplayerClient.UserStateChanged), ((IMultiplayerClient)this).UserStateChanged);
-            connection.On(nameof(IMultiplayerClient.LoadRequested), ((IMultiplayerClient)this).LoadRequested);
-            connection.On(nameof(IMultiplayerClient.MatchStarted), ((IMultiplayerClient)this).MatchStarted);
-            connection.On(nameof(IMultiplayerClient.ResultsReady), ((IMultiplayerClient)this).ResultsReady);
-            connection.On<int, IEnumerable<APIMod>>(nameof(IMultiplayerClient.UserModsChanged), ((IMultiplayerClient)this).UserModsChanged);
-
-            connection.Closed += async ex =>
-            {
-                isConnected.Value = false;
-
-                Logger.Log(ex != null
-                    ? $"Multiplayer client lost connection: {ex}"
-                    : "Multiplayer client disconnected", LoggingTarget.Network);
-
-                if (connection != null)
-                    await tryUntilConnected();
-            };
-
-            await tryUntilConnected();
-
-            async Task tryUntilConnected()
-            {
-                Logger.Log("Multiplayer client connecting...", LoggingTarget.Network);
-
-                while (api.State.Value == APIState.Online)
-                {
             try
             {
-                        Debug.Assert(connection != null);
+                while (api.State.Value == APIState.Online)
+                {
+                    // ensure any previous connection was disposed.
+                    // this will also create a new cancellation token source.
+                    await disconnect(false);
+
+                    // this token will be valid for the scope of this connection.
+                    // if cancelled, we can be sure that a disconnect or reconnect is handled elsewhere.
+                    var cancellationToken = connectCancelSource.Token;
+
+                    cancellationToken.ThrowIfCancellationRequested();
+
+                    Logger.Log("Multiplayer client connecting...", LoggingTarget.Network);
+
+                    try
+                    {
+                        // importantly, rebuild the connection each attempt to get an updated access token.
+                        connection = createConnection(cancellationToken);
+
+                        await connection.StartAsync(cancellationToken);
 
-                        // reconnect on any failure
-                        await connection.StartAsync();
                         Logger.Log("Multiplayer client connected!", LoggingTarget.Network);
-
-                        // Success.
                         isConnected.Value = true;
-                        break;
+                        return;
+                    }
+                    catch (OperationCanceledException)
+                    {
+                        //connection process was cancelled.
+                        throw;
                     }
                     catch (Exception e)
                     {
                         Logger.Log($"Multiplayer client connection error: {e}", LoggingTarget.Network);
-                        await Task.Delay(5000);
+
+                        // retry on any failure.
+                        await Task.Delay(5000, cancellationToken);
                     }
                 }
             }
+            finally
+            {
+                connectionLock.Release();
+            }
         }
 
         protected override Task<MultiplayerRoom> JoinRoom(long roomId)
@@ -206,5 +203,76 @@ namespace osu.Game.Online.Multiplayer
 
             return connection.InvokeAsync(nameof(IMultiplayerServer.StartMatch));
         }
+
+        private async Task disconnect(bool takeLock)
+        {
+            cancelExistingConnect();
+
+            if (takeLock)
+            {
+                if (!await connectionLock.WaitAsync(10000))
+                    throw new TimeoutException("Could not obtain a lock to disconnect. A previous attempt is likely stuck.");
+            }
+
+            try
+            {
+                if (connection != null)
+                    await connection.DisposeAsync();
+            }
+            finally
+            {
+                connection = null;
+                if (takeLock)
+                    connectionLock.Release();
+            }
+        }
+
+        private void cancelExistingConnect()
+        {
+            connectCancelSource.Cancel();
+            connectCancelSource = new CancellationTokenSource();
+        }
+
+        private HubConnection createConnection(CancellationToken cancellationToken)
+        {
+            var newConnection = new HubConnectionBuilder()
+                                .WithUrl(endpoint, options => { options.Headers.Add("Authorization", $"Bearer {api.AccessToken}"); })
+                                .AddNewtonsoftJsonProtocol(options => { options.PayloadSerializerSettings.ReferenceLoopHandling = ReferenceLoopHandling.Ignore; })
+                                .Build();
+
+            // this is kind of SILLY
+            // https://github.com/dotnet/aspnetcore/issues/15198
+            newConnection.On<MultiplayerRoomState>(nameof(IMultiplayerClient.RoomStateChanged), ((IMultiplayerClient)this).RoomStateChanged);
+            newConnection.On<MultiplayerRoomUser>(nameof(IMultiplayerClient.UserJoined), ((IMultiplayerClient)this).UserJoined);
+            newConnection.On<MultiplayerRoomUser>(nameof(IMultiplayerClient.UserLeft), ((IMultiplayerClient)this).UserLeft);
+            newConnection.On<int>(nameof(IMultiplayerClient.HostChanged), ((IMultiplayerClient)this).HostChanged);
+            newConnection.On<MultiplayerRoomSettings>(nameof(IMultiplayerClient.SettingsChanged), ((IMultiplayerClient)this).SettingsChanged);
+            newConnection.On<int, MultiplayerUserState>(nameof(IMultiplayerClient.UserStateChanged), ((IMultiplayerClient)this).UserStateChanged);
+            newConnection.On(nameof(IMultiplayerClient.LoadRequested), ((IMultiplayerClient)this).LoadRequested);
+            newConnection.On(nameof(IMultiplayerClient.MatchStarted), ((IMultiplayerClient)this).MatchStarted);
+            newConnection.On(nameof(IMultiplayerClient.ResultsReady), ((IMultiplayerClient)this).ResultsReady);
+            newConnection.On<int, IEnumerable<APIMod>>(nameof(IMultiplayerClient.UserModsChanged), ((IMultiplayerClient)this).UserModsChanged);
+
+            newConnection.Closed += ex =>
+            {
+                isConnected.Value = false;
+
+                Logger.Log(ex != null ? $"Multiplayer client lost connection: {ex}" : "Multiplayer client disconnected", LoggingTarget.Network);
+
+                // make sure a disconnect wasn't triggered (and this is still the active connection).
+                if (!cancellationToken.IsCancellationRequested)
+                    Task.Run(connect, default);
+
+                return Task.CompletedTask;
+            };
+            return newConnection;
+        }
+
+        protected override void Dispose(bool isDisposing)
+        {
+            base.Dispose(isDisposing);
+
+            cancelExistingConnect();
+        }
     }
 }