Clean up multiplayer client with new hub connector

osu.Game/Online/Multiplayer/MultiplayerClient.cs

@@ -3,17 +3,11 @@
 
 #nullable enable
 
-using System;
 using System.Collections.Generic;
 using System.Threading;
 using System.Threading.Tasks;
 using Microsoft.AspNetCore.SignalR.Client;
-using Microsoft.Extensions.DependencyInjection;
-using Newtonsoft.Json;
-using osu.Framework;
-using osu.Framework.Allocation;
 using osu.Framework.Bindables;
-using osu.Framework.Logging;
 using osu.Game.Online.API;
 using osu.Game.Online.Rooms;
 
@@ -21,212 +15,18 @@ namespace osu.Game.Online.Multiplayer
 {
     public class MultiplayerClient : StatefulMultiplayerClient
     {
-        public override IBindable<bool> IsConnected => isConnected;
+        private readonly HubClientConnector connector;
 
-        private readonly Bindable<bool> isConnected = new Bindable<bool>();
+        public override IBindable<bool> IsConnected => connector.IsConnected;
-        private readonly IBindable<APIState> apiState = new Bindable<APIState>();
 
-        private readonly SemaphoreSlim connectionLock = new SemaphoreSlim(1);
+        private HubConnection? connection => connector.CurrentConnection;
-
-        [Resolved]
-        private IAPIProvider api { get; set; } = null!;
-
-        private HubConnection? connection;
-
-        private CancellationTokenSource connectCancelSource = new CancellationTokenSource();
-
-        private readonly string endpoint;
 
         public MultiplayerClient(EndpointConfiguration endpoints)
         {
-            endpoint = endpoints.MultiplayerEndpointUrl;
+            InternalChild = connector = new HubClientConnector("Multiplayer client", endpoints.MultiplayerEndpointUrl)
-        }
-
-        [BackgroundDependencyLoader]
-        private void load()
             {
-            apiState.BindTo(api.State);
+                OnNewConnection = newConnection =>
-            apiState.BindValueChanged(apiStateChanged, true);
-        }
-
-        private void apiStateChanged(ValueChangedEvent<APIState> state)
                 {
-            switch (state.NewValue)
-            {
-                case APIState.Failing:
-                case APIState.Offline:
-                    Task.Run(() => disconnect(true));
-                    break;
-
-                case APIState.Online:
-                    Task.Run(connect);
-                    break;
-            }
-        }
-
-        private async Task connect()
-        {
-            cancelExistingConnect();
-
-            if (!await connectionLock.WaitAsync(10000))
-                throw new TimeoutException("Could not obtain a lock to connect. A previous attempt is likely stuck.");
-
-            try
-            {
-                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);
-
-                        Logger.Log("Multiplayer client connected!", LoggingTarget.Network);
-                        isConnected.Value = true;
-                        return;
-                    }
-                    catch (OperationCanceledException)
-                    {
-                        //connection process was cancelled.
-                        throw;
-                    }
-                    catch (Exception e)
-                    {
-                        Logger.Log($"Multiplayer client connection error: {e}", LoggingTarget.Network);
-
-                        // retry on any failure.
-                        await Task.Delay(5000, cancellationToken);
-                    }
-                }
-            }
-            finally
-            {
-                connectionLock.Release();
-            }
-        }
-
-        protected override Task<MultiplayerRoom> JoinRoom(long roomId)
-        {
-            if (!isConnected.Value)
-                return Task.FromCanceled<MultiplayerRoom>(new CancellationToken(true));
-
-            return connection.InvokeAsync<MultiplayerRoom>(nameof(IMultiplayerServer.JoinRoom), roomId);
-        }
-
-        protected override Task LeaveRoomInternal()
-        {
-            if (!isConnected.Value)
-                return Task.FromCanceled(new CancellationToken(true));
-
-            return connection.InvokeAsync(nameof(IMultiplayerServer.LeaveRoom));
-        }
-
-        public override Task TransferHost(int userId)
-        {
-            if (!isConnected.Value)
-                return Task.CompletedTask;
-
-            return connection.InvokeAsync(nameof(IMultiplayerServer.TransferHost), userId);
-        }
-
-        public override Task ChangeSettings(MultiplayerRoomSettings settings)
-        {
-            if (!isConnected.Value)
-                return Task.CompletedTask;
-
-            return connection.InvokeAsync(nameof(IMultiplayerServer.ChangeSettings), settings);
-        }
-
-        public override Task ChangeState(MultiplayerUserState newState)
-        {
-            if (!isConnected.Value)
-                return Task.CompletedTask;
-
-            return connection.InvokeAsync(nameof(IMultiplayerServer.ChangeState), newState);
-        }
-
-        public override Task ChangeBeatmapAvailability(BeatmapAvailability newBeatmapAvailability)
-        {
-            if (!isConnected.Value)
-                return Task.CompletedTask;
-
-            return connection.InvokeAsync(nameof(IMultiplayerServer.ChangeBeatmapAvailability), newBeatmapAvailability);
-        }
-
-        public override Task ChangeUserMods(IEnumerable<APIMod> newMods)
-        {
-            if (!isConnected.Value)
-                return Task.CompletedTask;
-
-            return connection.InvokeAsync(nameof(IMultiplayerServer.ChangeUserMods), newMods);
-        }
-
-        public override Task StartMatch()
-        {
-            if (!isConnected.Value)
-                return Task.CompletedTask;
-
-            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 builder = new HubConnectionBuilder()
-                .WithUrl(endpoint, options => { options.Headers.Add("Authorization", $"Bearer {api.AccessToken}"); });
-
-            if (RuntimeInfo.SupportsJIT)
-                builder.AddMessagePackProtocol();
-            else
-            {
-                // eventually we will precompile resolvers for messagepack, but this isn't working currently
-                // see https://github.com/neuecc/MessagePack-CSharp/issues/780#issuecomment-768794308.
-                builder.AddNewtonsoftJsonProtocol(options => { options.PayloadSerializerSettings.ReferenceLoopHandling = ReferenceLoopHandling.Ignore; });
-            }
-
-            var newConnection = builder.Build();
-
                     // this is kind of SILLY
                     // https://github.com/dotnet/aspnetcore/issues/15198
                     newConnection.On<MultiplayerRoomState>(nameof(IMultiplayerClient.RoomStateChanged), ((IMultiplayerClient)this).RoomStateChanged);
@@ -239,27 +39,72 @@ namespace osu.Game.Online.Multiplayer
                     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)
+        protected override Task<MultiplayerRoom> JoinRoom(long roomId)
         {
-            base.Dispose(isDisposing);
+            if (!IsConnected.Value)
+                return Task.FromCanceled<MultiplayerRoom>(new CancellationToken(true));
 
-            cancelExistingConnect();
+            return connection.InvokeAsync<MultiplayerRoom>(nameof(IMultiplayerServer.JoinRoom), roomId);
+        }
+
+        protected override Task LeaveRoomInternal()
+        {
+            if (!IsConnected.Value)
+                return Task.FromCanceled(new CancellationToken(true));
+
+            return connection.InvokeAsync(nameof(IMultiplayerServer.LeaveRoom));
+        }
+
+        public override Task TransferHost(int userId)
+        {
+            if (!IsConnected.Value)
+                return Task.CompletedTask;
+
+            return connection.InvokeAsync(nameof(IMultiplayerServer.TransferHost), userId);
+        }
+
+        public override Task ChangeSettings(MultiplayerRoomSettings settings)
+        {
+            if (!IsConnected.Value)
+                return Task.CompletedTask;
+
+            return connection.InvokeAsync(nameof(IMultiplayerServer.ChangeSettings), settings);
+        }
+
+        public override Task ChangeState(MultiplayerUserState newState)
+        {
+            if (!IsConnected.Value)
+                return Task.CompletedTask;
+
+            return connection.InvokeAsync(nameof(IMultiplayerServer.ChangeState), newState);
+        }
+
+        public override Task ChangeBeatmapAvailability(BeatmapAvailability newBeatmapAvailability)
+        {
+            if (!IsConnected.Value)
+                return Task.CompletedTask;
+
+            return connection.InvokeAsync(nameof(IMultiplayerServer.ChangeBeatmapAvailability), newBeatmapAvailability);
+        }
+
+        public override Task ChangeUserMods(IEnumerable<APIMod> newMods)
+        {
+            if (!IsConnected.Value)
+                return Task.CompletedTask;
+
+            return connection.InvokeAsync(nameof(IMultiplayerServer.ChangeUserMods), newMods);
+        }
+
+        public override Task StartMatch()
+        {
+            if (!IsConnected.Value)
+                return Task.CompletedTask;
+
+            return connection.InvokeAsync(nameof(IMultiplayerServer.StartMatch));
         }
     }
 }

osu.Game/Online/Multiplayer/StatefulMultiplayerClient.cs

@@ -12,7 +12,7 @@ using System.Threading.Tasks;
 using osu.Framework.Allocation;
 using osu.Framework.Bindables;
 using osu.Framework.Extensions.ObjectExtensions;
-using osu.Framework.Graphics;
+using osu.Framework.Graphics.Containers;
 using osu.Framework.Logging;
 using osu.Game.Beatmaps;
 using osu.Game.Database;
@@ -28,7 +28,7 @@ using osu.Game.Utils;
 
 namespace osu.Game.Online.Multiplayer
 {
-    public abstract class StatefulMultiplayerClient : Component, IMultiplayerClient, IMultiplayerRoomServer
+    public abstract class StatefulMultiplayerClient : CompositeDrawable, IMultiplayerClient, IMultiplayerRoomServer
     {
         /// <summary>
         /// Invoked when any change occurs to the multiplayer room.
@@ -97,7 +97,8 @@ namespace osu.Game.Online.Multiplayer
         // Todo: This is temporary, until the multiplayer server returns the item id on match start or otherwise.
         private int playlistItemId;
 
-        protected StatefulMultiplayerClient()
+        [BackgroundDependencyLoader]
+        private void load()
         {
             IsConnected.BindValueChanged(connected =>
             {