From af345ea5db05c056234e665f665b53b0a4912d1a Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Tue, 9 Feb 2021 01:52:35 +0300 Subject: [PATCH 01/12] Add a SignalR hub client connector component --- osu.Game/Online/HubClientConnector.cs | 209 ++++++++++++++++++++++++++ 1 file changed, 209 insertions(+) create mode 100644 osu.Game/Online/HubClientConnector.cs diff --git a/osu.Game/Online/HubClientConnector.cs b/osu.Game/Online/HubClientConnector.cs new file mode 100644 index 0000000000..49b1ab639a --- /dev/null +++ b/osu.Game/Online/HubClientConnector.cs @@ -0,0 +1,209 @@ +// 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; +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.Graphics; +using osu.Framework.Logging; +using osu.Game.Online.API; + +namespace osu.Game.Online +{ + /// + /// A component that maintains over a hub connection between client and server. + /// + public class HubClientConnector : Component + { + /// + /// Invoked whenever a new hub connection is built. + /// + public Action? OnNewConnection; + + private readonly string clientName; + private readonly string endpoint; + + /// + /// The current connection opened by this connector. + /// + public HubConnection? CurrentConnection { get; private set; } + + /// + /// Whether this is connected to the hub, use to access the connection, if this is true. + /// + public IBindable IsConnected => isConnected; + + private readonly Bindable isConnected = new Bindable(); + private readonly SemaphoreSlim connectionLock = new SemaphoreSlim(1); + private CancellationTokenSource connectCancelSource = new CancellationTokenSource(); + + [Resolved] + private IAPIProvider api { get; set; } = null!; + + private readonly IBindable apiState = new Bindable(); + + /// + /// Constructs a new . + /// + /// The name of the client this connector connects for, used for logging. + /// The endpoint to the hub. + public HubClientConnector(string clientName, string endpoint) + { + this.clientName = clientName; + this.endpoint = endpoint; + } + + [BackgroundDependencyLoader] + private void load() + { + apiState.BindTo(api.State); + apiState.BindValueChanged(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 (apiState.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($"{clientName} connecting...", LoggingTarget.Network); + + try + { + // importantly, rebuild the connection each attempt to get an updated access token. + CurrentConnection = createConnection(cancellationToken); + + await CurrentConnection.StartAsync(cancellationToken); + + Logger.Log($"{clientName} connected!", LoggingTarget.Network); + isConnected.Value = true; + return; + } + catch (OperationCanceledException) + { + //connection process was cancelled. + throw; + } + catch (Exception e) + { + Logger.Log($"{clientName} connection error: {e}", LoggingTarget.Network); + + // retry on any failure. + await Task.Delay(5000, cancellationToken); + } + } + } + finally + { + connectionLock.Release(); + } + } + + 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(); + + OnNewConnection?.Invoke(newConnection); + + newConnection.Closed += ex => + { + isConnected.Value = false; + + Logger.Log(ex != null ? $"{clientName} lost connection: {ex}" : $"{clientName} 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; + } + + 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 (CurrentConnection != null) + await CurrentConnection.DisposeAsync(); + } + finally + { + CurrentConnection = null; + if (takeLock) + connectionLock.Release(); + } + } + + private void cancelExistingConnect() + { + connectCancelSource.Cancel(); + connectCancelSource = new CancellationTokenSource(); + } + + public override string ToString() => $"Connector for {clientName} ({(IsConnected.Value ? "connected" : "not connected")}"; + + protected override void Dispose(bool isDisposing) + { + base.Dispose(isDisposing); + cancelExistingConnect(); + } + } +} From 28b815ffe13a5439241ccaeb763e26cb4e794ca6 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Tue, 9 Feb 2021 02:01:52 +0300 Subject: [PATCH 02/12] Clean up multiplayer client with new hub connector --- .../Online/Multiplayer/MultiplayerClient.cs | 209 +++--------------- .../Multiplayer/StatefulMultiplayerClient.cs | 7 +- 2 files changed, 31 insertions(+), 185 deletions(-) diff --git a/osu.Game/Online/Multiplayer/MultiplayerClient.cs b/osu.Game/Online/Multiplayer/MultiplayerClient.cs index 493518ac80..07036e7ffc 100644 --- a/osu.Game/Online/Multiplayer/MultiplayerClient.cs +++ b/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,106 +15,37 @@ namespace osu.Game.Online.Multiplayer { public class MultiplayerClient : StatefulMultiplayerClient { - public override IBindable IsConnected => isConnected; + private readonly HubClientConnector connector; - private readonly Bindable isConnected = new Bindable(); - private readonly IBindable apiState = new Bindable(); + public override IBindable IsConnected => connector.IsConnected; - 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; + private HubConnection? connection => connector.CurrentConnection; public MultiplayerClient(EndpointConfiguration endpoints) { - endpoint = endpoints.MultiplayerEndpointUrl; - } - - [BackgroundDependencyLoader] - private void load() - { - apiState.BindTo(api.State); - apiState.BindValueChanged(apiStateChanged, true); - } - - private void apiStateChanged(ValueChangedEvent state) - { - switch (state.NewValue) + InternalChild = connector = new HubClientConnector("Multiplayer client", endpoints.MultiplayerEndpointUrl) { - 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) + OnNewConnection = newConnection => { - // 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(); - } + // this is kind of SILLY + // https://github.com/dotnet/aspnetcore/issues/15198 + newConnection.On(nameof(IMultiplayerClient.RoomStateChanged), ((IMultiplayerClient)this).RoomStateChanged); + newConnection.On(nameof(IMultiplayerClient.UserJoined), ((IMultiplayerClient)this).UserJoined); + newConnection.On(nameof(IMultiplayerClient.UserLeft), ((IMultiplayerClient)this).UserLeft); + newConnection.On(nameof(IMultiplayerClient.HostChanged), ((IMultiplayerClient)this).HostChanged); + newConnection.On(nameof(IMultiplayerClient.SettingsChanged), ((IMultiplayerClient)this).SettingsChanged); + newConnection.On(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>(nameof(IMultiplayerClient.UserModsChanged), ((IMultiplayerClient)this).UserModsChanged); + }, + }; } protected override Task JoinRoom(long roomId) { - if (!isConnected.Value) + if (!IsConnected.Value) return Task.FromCanceled(new CancellationToken(true)); return connection.InvokeAsync(nameof(IMultiplayerServer.JoinRoom), roomId); @@ -128,7 +53,7 @@ namespace osu.Game.Online.Multiplayer protected override Task LeaveRoomInternal() { - if (!isConnected.Value) + if (!IsConnected.Value) return Task.FromCanceled(new CancellationToken(true)); return connection.InvokeAsync(nameof(IMultiplayerServer.LeaveRoom)); @@ -136,7 +61,7 @@ namespace osu.Game.Online.Multiplayer public override Task TransferHost(int userId) { - if (!isConnected.Value) + if (!IsConnected.Value) return Task.CompletedTask; return connection.InvokeAsync(nameof(IMultiplayerServer.TransferHost), userId); @@ -144,7 +69,7 @@ namespace osu.Game.Online.Multiplayer public override Task ChangeSettings(MultiplayerRoomSettings settings) { - if (!isConnected.Value) + if (!IsConnected.Value) return Task.CompletedTask; return connection.InvokeAsync(nameof(IMultiplayerServer.ChangeSettings), settings); @@ -152,7 +77,7 @@ namespace osu.Game.Online.Multiplayer public override Task ChangeState(MultiplayerUserState newState) { - if (!isConnected.Value) + if (!IsConnected.Value) return Task.CompletedTask; return connection.InvokeAsync(nameof(IMultiplayerServer.ChangeState), newState); @@ -160,7 +85,7 @@ namespace osu.Game.Online.Multiplayer public override Task ChangeBeatmapAvailability(BeatmapAvailability newBeatmapAvailability) { - if (!isConnected.Value) + if (!IsConnected.Value) return Task.CompletedTask; return connection.InvokeAsync(nameof(IMultiplayerServer.ChangeBeatmapAvailability), newBeatmapAvailability); @@ -168,7 +93,7 @@ namespace osu.Game.Online.Multiplayer public override Task ChangeUserMods(IEnumerable newMods) { - if (!isConnected.Value) + if (!IsConnected.Value) return Task.CompletedTask; return connection.InvokeAsync(nameof(IMultiplayerServer.ChangeUserMods), newMods); @@ -176,90 +101,10 @@ namespace osu.Game.Online.Multiplayer public override Task StartMatch() { - if (!isConnected.Value) + 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(nameof(IMultiplayerClient.RoomStateChanged), ((IMultiplayerClient)this).RoomStateChanged); - newConnection.On(nameof(IMultiplayerClient.UserJoined), ((IMultiplayerClient)this).UserJoined); - newConnection.On(nameof(IMultiplayerClient.UserLeft), ((IMultiplayerClient)this).UserLeft); - newConnection.On(nameof(IMultiplayerClient.HostChanged), ((IMultiplayerClient)this).HostChanged); - newConnection.On(nameof(IMultiplayerClient.SettingsChanged), ((IMultiplayerClient)this).SettingsChanged); - newConnection.On(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>(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(); - } } } diff --git a/osu.Game/Online/Multiplayer/StatefulMultiplayerClient.cs b/osu.Game/Online/Multiplayer/StatefulMultiplayerClient.cs index f454fe619b..06f6754258 100644 --- a/osu.Game/Online/Multiplayer/StatefulMultiplayerClient.cs +++ b/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 { /// /// 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 => { From f76f92515e7eb3588af91e0b3aac4c47fbc26731 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Tue, 9 Feb 2021 02:15:51 +0300 Subject: [PATCH 03/12] Clean up spectator streaming client with new hub connector --- .../Visual/Gameplay/TestSceneSpectator.cs | 8 +- ...TestSceneMultiplayerGameplayLeaderboard.cs | 5 +- .../Spectator/SpectatorStreamingClient.cs | 159 +++++------------- 3 files changed, 48 insertions(+), 124 deletions(-) diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs index 26524f07da..61b0961638 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs @@ -3,7 +3,6 @@ using System.Collections.Generic; using System.Linq; -using System.Threading.Tasks; using NUnit.Framework; using osu.Framework.Allocation; using osu.Framework.Bindables; @@ -233,6 +232,8 @@ namespace osu.Game.Tests.Visual.Gameplay public class TestSpectatorStreamingClient : SpectatorStreamingClient { + protected override IBindable IsConnected { get; } = new BindableBool(false); + public readonly User StreamingUser = new User { Id = 55, Username = "Test user" }; public new BindableList PlayingUsers => (BindableList)base.PlayingUsers; @@ -244,11 +245,6 @@ namespace osu.Game.Tests.Visual.Gameplay { } - protected override Task Connect() - { - return Task.CompletedTask; - } - public void StartPlay(int beatmapId) { this.beatmapId = beatmapId; diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerGameplayLeaderboard.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerGameplayLeaderboard.cs index d016accc25..6a777e2a78 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerGameplayLeaderboard.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerGameplayLeaderboard.cs @@ -4,7 +4,6 @@ using System; using System.Collections.Generic; using System.Linq; -using System.Threading.Tasks; using NUnit.Framework; using osu.Framework.Allocation; using osu.Framework.Bindables; @@ -96,6 +95,8 @@ namespace osu.Game.Tests.Visual.Multiplayer public class TestMultiplayerStreaming : SpectatorStreamingClient { + protected override IBindable IsConnected { get; } = new BindableBool(false); + public new BindableList PlayingUsers => (BindableList)base.PlayingUsers; private readonly int totalUsers; @@ -163,8 +164,6 @@ namespace osu.Game.Tests.Visual.Multiplayer ((ISpectatorClient)this).UserSentFrames(userId, new FrameDataBundle(header, Array.Empty())); } } - - protected override Task Connect() => Task.CompletedTask; } } } diff --git a/osu.Game/Online/Spectator/SpectatorStreamingClient.cs b/osu.Game/Online/Spectator/SpectatorStreamingClient.cs index b95e3f1297..7cea76c969 100644 --- a/osu.Game/Online/Spectator/SpectatorStreamingClient.cs +++ b/osu.Game/Online/Spectator/SpectatorStreamingClient.cs @@ -8,13 +8,9 @@ using System.Linq; using System.Threading.Tasks; using JetBrains.Annotations; 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.Graphics; -using osu.Framework.Logging; +using osu.Framework.Graphics.Containers; using osu.Game.Beatmaps; using osu.Game.Online.API; using osu.Game.Replays.Legacy; @@ -27,14 +23,18 @@ using osu.Game.Screens.Play; namespace osu.Game.Online.Spectator { - public class SpectatorStreamingClient : Component, ISpectatorClient + public class SpectatorStreamingClient : CompositeDrawable, ISpectatorClient { /// /// The maximum milliseconds between frame bundle sends. /// public const double TIME_BETWEEN_SENDS = 200; - private HubConnection connection; + private readonly HubClientConnector connector; + + protected virtual IBindable IsConnected => connector.IsConnected; + + private HubConnection connection => connector.CurrentConnection; private readonly List watchingUsers = new List(); @@ -44,13 +44,6 @@ namespace osu.Game.Online.Spectator private readonly BindableList playingUsers = new BindableList(); - private readonly IBindable apiState = new Bindable(); - - private bool isConnected; - - [Resolved] - private IAPIProvider api { get; set; } - [CanBeNull] private IBeatmap currentBeatmap; @@ -82,114 +75,50 @@ namespace osu.Game.Online.Spectator /// public event Action OnUserFinishedPlaying; - private readonly string endpoint; - public SpectatorStreamingClient(EndpointConfiguration endpoints) { - endpoint = endpoints.SpectatorEndpointUrl; + InternalChild = connector = new HubClientConnector("Spectator client", endpoints.SpectatorEndpointUrl) + { + OnNewConnection = newConnection => + { + // until strong typed client support is added, each method must be manually bound + // (see https://github.com/dotnet/aspnetcore/issues/15198) + newConnection.On(nameof(ISpectatorClient.UserBeganPlaying), ((ISpectatorClient)this).UserBeganPlaying); + newConnection.On(nameof(ISpectatorClient.UserSentFrames), ((ISpectatorClient)this).UserSentFrames); + newConnection.On(nameof(ISpectatorClient.UserFinishedPlaying), ((ISpectatorClient)this).UserFinishedPlaying); + } + }; } [BackgroundDependencyLoader] private void load() { - apiState.BindTo(api.State); - apiState.BindValueChanged(apiStateChanged, true); - } - - private void apiStateChanged(ValueChangedEvent state) - { - switch (state.NewValue) + IsConnected.BindValueChanged(connected => { - case APIState.Failing: - case APIState.Offline: - connection?.StopAsync(); - connection = null; - break; - - case APIState.Online: - Task.Run(Connect); - break; - } - } - - protected virtual async Task Connect() - { - if (connection != null) - return; - - 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; }); - } - - connection = builder.Build(); - // until strong typed client support is added, each method must be manually bound (see https://github.com/dotnet/aspnetcore/issues/15198) - connection.On(nameof(ISpectatorClient.UserBeganPlaying), ((ISpectatorClient)this).UserBeganPlaying); - connection.On(nameof(ISpectatorClient.UserSentFrames), ((ISpectatorClient)this).UserSentFrames); - connection.On(nameof(ISpectatorClient.UserFinishedPlaying), ((ISpectatorClient)this).UserFinishedPlaying); - - connection.Closed += async ex => - { - isConnected = false; - playingUsers.Clear(); - - if (ex != null) + if (connected.NewValue) { - Logger.Log($"Spectator client lost connection: {ex}", LoggingTarget.Network); - await tryUntilConnected(); + // get all the users that were previously being watched + int[] users; + + lock (userLock) + { + users = watchingUsers.ToArray(); + watchingUsers.Clear(); + } + + // resubscribe to watched users. + foreach (var userId in users) + WatchUser(userId); + + // re-send state in case it wasn't received + if (isPlaying) + beginPlaying(); } - }; - - await tryUntilConnected(); - - async Task tryUntilConnected() - { - Logger.Log("Spectator client connecting...", LoggingTarget.Network); - - while (api.State.Value == APIState.Online) + else { - try - { - // reconnect on any failure - await connection.StartAsync(); - Logger.Log("Spectator client connected!", LoggingTarget.Network); - - // get all the users that were previously being watched - int[] users; - - lock (userLock) - { - users = watchingUsers.ToArray(); - watchingUsers.Clear(); - } - - // success - isConnected = true; - - // resubscribe to watched users - foreach (var userId in users) - WatchUser(userId); - - // re-send state in case it wasn't received - if (isPlaying) - beginPlaying(); - - break; - } - catch (Exception e) - { - Logger.Log($"Spectator client connection error: {e}", LoggingTarget.Network); - await Task.Delay(5000); - } + playingUsers.Clear(); } - } + }, true); } Task ISpectatorClient.UserBeganPlaying(int userId, SpectatorState state) @@ -240,14 +169,14 @@ namespace osu.Game.Online.Spectator { Debug.Assert(isPlaying); - if (!isConnected) return; + if (!IsConnected.Value) return; connection.SendAsync(nameof(ISpectatorServer.BeginPlaySession), currentState); } public void SendFrames(FrameDataBundle data) { - if (!isConnected) return; + if (!IsConnected.Value) return; lastSend = connection.SendAsync(nameof(ISpectatorServer.SendFrameData), data); } @@ -257,7 +186,7 @@ namespace osu.Game.Online.Spectator isPlaying = false; currentBeatmap = null; - if (!isConnected) return; + if (!IsConnected.Value) return; connection.SendAsync(nameof(ISpectatorServer.EndPlaySession), currentState); } @@ -271,7 +200,7 @@ namespace osu.Game.Online.Spectator watchingUsers.Add(userId); - if (!isConnected) + if (!IsConnected.Value) return; } @@ -284,7 +213,7 @@ namespace osu.Game.Online.Spectator { watchingUsers.Remove(userId); - if (!isConnected) + if (!IsConnected.Value) return; } From 71e564d399e617d6083d8540bdcdc2d89a18d2e3 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Tue, 9 Feb 2021 07:46:00 +0300 Subject: [PATCH 04/12] Revert clients to be `Component`s --- osu.Game/Online/Multiplayer/StatefulMultiplayerClient.cs | 4 ++-- osu.Game/Online/Spectator/SpectatorStreamingClient.cs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/osu.Game/Online/Multiplayer/StatefulMultiplayerClient.cs b/osu.Game/Online/Multiplayer/StatefulMultiplayerClient.cs index 06f6754258..18464a5f61 100644 --- a/osu.Game/Online/Multiplayer/StatefulMultiplayerClient.cs +++ b/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.Containers; +using osu.Framework.Graphics; 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 : CompositeDrawable, IMultiplayerClient, IMultiplayerRoomServer + public abstract class StatefulMultiplayerClient : Component, IMultiplayerClient, IMultiplayerRoomServer { /// /// Invoked when any change occurs to the multiplayer room. diff --git a/osu.Game/Online/Spectator/SpectatorStreamingClient.cs b/osu.Game/Online/Spectator/SpectatorStreamingClient.cs index 7cea76c969..33ebe27937 100644 --- a/osu.Game/Online/Spectator/SpectatorStreamingClient.cs +++ b/osu.Game/Online/Spectator/SpectatorStreamingClient.cs @@ -10,7 +10,7 @@ using JetBrains.Annotations; using Microsoft.AspNetCore.SignalR.Client; using osu.Framework.Allocation; using osu.Framework.Bindables; -using osu.Framework.Graphics.Containers; +using osu.Framework.Graphics; using osu.Game.Beatmaps; using osu.Game.Online.API; using osu.Game.Replays.Legacy; @@ -23,7 +23,7 @@ using osu.Game.Screens.Play; namespace osu.Game.Online.Spectator { - public class SpectatorStreamingClient : CompositeDrawable, ISpectatorClient + public class SpectatorStreamingClient : Component, ISpectatorClient { /// /// The maximum milliseconds between frame bundle sends. From 848b81e952934ee90f3cc4e86428bb385c0fdde6 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Tue, 9 Feb 2021 07:53:22 +0300 Subject: [PATCH 05/12] Remove necessity of making hub client connector a component --- osu.Game/Online/HubClientConnector.cs | 51 ++++++++++++++------------- 1 file changed, 26 insertions(+), 25 deletions(-) diff --git a/osu.Game/Online/HubClientConnector.cs b/osu.Game/Online/HubClientConnector.cs index 49b1ab639a..b740aabb92 100644 --- a/osu.Game/Online/HubClientConnector.cs +++ b/osu.Game/Online/HubClientConnector.cs @@ -4,15 +4,14 @@ #nullable enable using System; +using System.Diagnostics; 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.Graphics; using osu.Framework.Logging; using osu.Game.Online.API; @@ -21,7 +20,7 @@ namespace osu.Game.Online /// /// A component that maintains over a hub connection between client and server. /// - public class HubClientConnector : Component + public class HubClientConnector : IDisposable { /// /// Invoked whenever a new hub connection is built. @@ -30,6 +29,7 @@ namespace osu.Game.Online private readonly string clientName; private readonly string endpoint; + private readonly IAPIProvider? api; /// /// The current connection opened by this connector. @@ -45,9 +45,6 @@ namespace osu.Game.Online private readonly SemaphoreSlim connectionLock = new SemaphoreSlim(1); private CancellationTokenSource connectCancelSource = new CancellationTokenSource(); - [Resolved] - private IAPIProvider api { get; set; } = null!; - private readonly IBindable apiState = new Bindable(); /// @@ -55,30 +52,32 @@ namespace osu.Game.Online /// /// The name of the client this connector connects for, used for logging. /// The endpoint to the hub. - public HubClientConnector(string clientName, string endpoint) + /// The API provider for listening to state changes, or null to not listen. + public HubClientConnector(string clientName, string endpoint, IAPIProvider? api) { this.clientName = clientName; this.endpoint = endpoint; - } - [BackgroundDependencyLoader] - private void load() - { - apiState.BindTo(api.State); - apiState.BindValueChanged(state => + this.api = api; + + if (api != null) { - switch (state.NewValue) + apiState.BindTo(api.State); + apiState.BindValueChanged(state => { - case APIState.Failing: - case APIState.Offline: - Task.Run(() => disconnect(true)); - break; + switch (state.NewValue) + { + case APIState.Failing: + case APIState.Offline: + Task.Run(() => disconnect(true)); + break; - case APIState.Online: - Task.Run(connect); - break; - } - }); + case APIState.Online: + Task.Run(connect); + break; + } + }, true); + } } private async Task connect() @@ -137,6 +136,8 @@ namespace osu.Game.Online private HubConnection createConnection(CancellationToken cancellationToken) { + Debug.Assert(api != null); + var builder = new HubConnectionBuilder() .WithUrl(endpoint, options => { options.Headers.Add("Authorization", $"Bearer {api.AccessToken}"); }); @@ -200,9 +201,9 @@ namespace osu.Game.Online public override string ToString() => $"Connector for {clientName} ({(IsConnected.Value ? "connected" : "not connected")}"; - protected override void Dispose(bool isDisposing) + public void Dispose() { - base.Dispose(isDisposing); + apiState.UnbindAll(); cancelExistingConnect(); } } From f04d6d5e5e98ddbdc0d94e4825d4d31392024be7 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Tue, 9 Feb 2021 08:02:32 +0300 Subject: [PATCH 06/12] Update hub clients with changes to connecotr --- .../Visual/Gameplay/TestSceneSpectator.cs | 2 - ...TestSceneMultiplayerGameplayLeaderboard.cs | 2 - .../Online/Multiplayer/MultiplayerClient.cs | 46 +++++++++++++------ .../Spectator/SpectatorStreamingClient.cs | 45 ++++++++++-------- 4 files changed, 56 insertions(+), 39 deletions(-) diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs index 61b0961638..4a0e1282c4 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs @@ -232,8 +232,6 @@ namespace osu.Game.Tests.Visual.Gameplay public class TestSpectatorStreamingClient : SpectatorStreamingClient { - protected override IBindable IsConnected { get; } = new BindableBool(false); - public readonly User StreamingUser = new User { Id = 55, Username = "Test user" }; public new BindableList PlayingUsers => (BindableList)base.PlayingUsers; diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerGameplayLeaderboard.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerGameplayLeaderboard.cs index 6a777e2a78..aab69d687a 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerGameplayLeaderboard.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerGameplayLeaderboard.cs @@ -95,8 +95,6 @@ namespace osu.Game.Tests.Visual.Multiplayer public class TestMultiplayerStreaming : SpectatorStreamingClient { - protected override IBindable IsConnected { get; } = new BindableBool(false); - public new BindableList PlayingUsers => (BindableList)base.PlayingUsers; private readonly int totalUsers; diff --git a/osu.Game/Online/Multiplayer/MultiplayerClient.cs b/osu.Game/Online/Multiplayer/MultiplayerClient.cs index 07036e7ffc..6b67954351 100644 --- a/osu.Game/Online/Multiplayer/MultiplayerClient.cs +++ b/osu.Game/Online/Multiplayer/MultiplayerClient.cs @@ -7,6 +7,7 @@ using System.Collections.Generic; using System.Threading; using System.Threading.Tasks; using Microsoft.AspNetCore.SignalR.Client; +using osu.Framework.Allocation; using osu.Framework.Bindables; using osu.Game.Online.API; using osu.Game.Online.Rooms; @@ -15,32 +16,41 @@ namespace osu.Game.Online.Multiplayer { public class MultiplayerClient : StatefulMultiplayerClient { - private readonly HubClientConnector connector; + private readonly string endpoint; + private HubClientConnector? connector; - public override IBindable IsConnected => connector.IsConnected; + public override IBindable IsConnected { get; } = new BindableBool(); - private HubConnection? connection => connector.CurrentConnection; + private HubConnection? connection => connector?.CurrentConnection; public MultiplayerClient(EndpointConfiguration endpoints) { - InternalChild = connector = new HubClientConnector("Multiplayer client", endpoints.MultiplayerEndpointUrl) + endpoint = endpoints.MultiplayerEndpointUrl; + } + + [BackgroundDependencyLoader] + private void load(IAPIProvider api) + { + connector = new HubClientConnector(nameof(MultiplayerClient), endpoint, api) { - OnNewConnection = newConnection => + OnNewConnection = connection => { // this is kind of SILLY // https://github.com/dotnet/aspnetcore/issues/15198 - newConnection.On(nameof(IMultiplayerClient.RoomStateChanged), ((IMultiplayerClient)this).RoomStateChanged); - newConnection.On(nameof(IMultiplayerClient.UserJoined), ((IMultiplayerClient)this).UserJoined); - newConnection.On(nameof(IMultiplayerClient.UserLeft), ((IMultiplayerClient)this).UserLeft); - newConnection.On(nameof(IMultiplayerClient.HostChanged), ((IMultiplayerClient)this).HostChanged); - newConnection.On(nameof(IMultiplayerClient.SettingsChanged), ((IMultiplayerClient)this).SettingsChanged); - newConnection.On(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>(nameof(IMultiplayerClient.UserModsChanged), ((IMultiplayerClient)this).UserModsChanged); + connection.On(nameof(IMultiplayerClient.RoomStateChanged), ((IMultiplayerClient)this).RoomStateChanged); + connection.On(nameof(IMultiplayerClient.UserJoined), ((IMultiplayerClient)this).UserJoined); + connection.On(nameof(IMultiplayerClient.UserLeft), ((IMultiplayerClient)this).UserLeft); + connection.On(nameof(IMultiplayerClient.HostChanged), ((IMultiplayerClient)this).HostChanged); + connection.On(nameof(IMultiplayerClient.SettingsChanged), ((IMultiplayerClient)this).SettingsChanged); + connection.On(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>(nameof(IMultiplayerClient.UserModsChanged), ((IMultiplayerClient)this).UserModsChanged); }, }; + + IsConnected.BindTo(connector.IsConnected); } protected override Task JoinRoom(long roomId) @@ -106,5 +116,11 @@ namespace osu.Game.Online.Multiplayer return connection.InvokeAsync(nameof(IMultiplayerServer.StartMatch)); } + + protected override void Dispose(bool isDisposing) + { + base.Dispose(isDisposing); + connector?.Dispose(); + } } } diff --git a/osu.Game/Online/Spectator/SpectatorStreamingClient.cs b/osu.Game/Online/Spectator/SpectatorStreamingClient.cs index 33ebe27937..4ef59b5e47 100644 --- a/osu.Game/Online/Spectator/SpectatorStreamingClient.cs +++ b/osu.Game/Online/Spectator/SpectatorStreamingClient.cs @@ -30,9 +30,11 @@ namespace osu.Game.Online.Spectator /// public const double TIME_BETWEEN_SENDS = 200; - private readonly HubClientConnector connector; + private readonly string endpoint; - protected virtual IBindable IsConnected => connector.IsConnected; + private HubClientConnector connector; + + private readonly IBindable isConnected = new BindableBool(); private HubConnection connection => connector.CurrentConnection; @@ -77,23 +79,24 @@ namespace osu.Game.Online.Spectator public SpectatorStreamingClient(EndpointConfiguration endpoints) { - InternalChild = connector = new HubClientConnector("Spectator client", endpoints.SpectatorEndpointUrl) - { - OnNewConnection = newConnection => - { - // until strong typed client support is added, each method must be manually bound - // (see https://github.com/dotnet/aspnetcore/issues/15198) - newConnection.On(nameof(ISpectatorClient.UserBeganPlaying), ((ISpectatorClient)this).UserBeganPlaying); - newConnection.On(nameof(ISpectatorClient.UserSentFrames), ((ISpectatorClient)this).UserSentFrames); - newConnection.On(nameof(ISpectatorClient.UserFinishedPlaying), ((ISpectatorClient)this).UserFinishedPlaying); - } - }; + endpoint = endpoints.SpectatorEndpointUrl; } [BackgroundDependencyLoader] - private void load() + private void load(IAPIProvider api) { - IsConnected.BindValueChanged(connected => + connector = CreateConnector(nameof(SpectatorStreamingClient), endpoint, api); + connector.OnNewConnection = connection => + { + // until strong typed client support is added, each method must be manually bound + // (see https://github.com/dotnet/aspnetcore/issues/15198) + connection.On(nameof(ISpectatorClient.UserBeganPlaying), ((ISpectatorClient)this).UserBeganPlaying); + connection.On(nameof(ISpectatorClient.UserSentFrames), ((ISpectatorClient)this).UserSentFrames); + connection.On(nameof(ISpectatorClient.UserFinishedPlaying), ((ISpectatorClient)this).UserFinishedPlaying); + }; + + isConnected.BindTo(connector.IsConnected); + isConnected.BindValueChanged(connected => { if (connected.NewValue) { @@ -121,6 +124,8 @@ namespace osu.Game.Online.Spectator }, true); } + protected virtual HubClientConnector CreateConnector(string name, string endpoint, IAPIProvider api) => new HubClientConnector(name, endpoint, api); + Task ISpectatorClient.UserBeganPlaying(int userId, SpectatorState state) { if (!playingUsers.Contains(userId)) @@ -169,14 +174,14 @@ namespace osu.Game.Online.Spectator { Debug.Assert(isPlaying); - if (!IsConnected.Value) return; + if (!isConnected.Value) return; connection.SendAsync(nameof(ISpectatorServer.BeginPlaySession), currentState); } public void SendFrames(FrameDataBundle data) { - if (!IsConnected.Value) return; + if (!isConnected.Value) return; lastSend = connection.SendAsync(nameof(ISpectatorServer.SendFrameData), data); } @@ -186,7 +191,7 @@ namespace osu.Game.Online.Spectator isPlaying = false; currentBeatmap = null; - if (!IsConnected.Value) return; + if (!isConnected.Value) return; connection.SendAsync(nameof(ISpectatorServer.EndPlaySession), currentState); } @@ -200,7 +205,7 @@ namespace osu.Game.Online.Spectator watchingUsers.Add(userId); - if (!IsConnected.Value) + if (!isConnected.Value) return; } @@ -213,7 +218,7 @@ namespace osu.Game.Online.Spectator { watchingUsers.Remove(userId); - if (!IsConnected.Value) + if (!isConnected.Value) return; } From a0ead38496b9dcb9ead023729765c437e7902f95 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Tue, 9 Feb 2021 08:02:51 +0300 Subject: [PATCH 07/12] Prevent test spectator clients from attempting hub connections --- osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs | 7 +++++++ .../Multiplayer/TestSceneMultiplayerGameplayLeaderboard.cs | 7 +++++++ 2 files changed, 14 insertions(+) diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs index 4a0e1282c4..1e499f20cb 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs @@ -12,6 +12,7 @@ using osu.Framework.Testing; using osu.Framework.Utils; using osu.Game.Beatmaps; using osu.Game.Online; +using osu.Game.Online.API; using osu.Game.Online.Spectator; using osu.Game.Replays.Legacy; using osu.Game.Rulesets.Osu; @@ -243,6 +244,12 @@ namespace osu.Game.Tests.Visual.Gameplay { } + protected override HubClientConnector CreateConnector(string name, string endpoint, IAPIProvider api) + { + // do not pass API to prevent attempting failing connections on an actual hub. + return base.CreateConnector(name, endpoint, null); + } + public void StartPlay(int beatmapId) { this.beatmapId = beatmapId; diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerGameplayLeaderboard.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerGameplayLeaderboard.cs index aab69d687a..b459cebdd7 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerGameplayLeaderboard.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerGameplayLeaderboard.cs @@ -13,6 +13,7 @@ using osu.Framework.Testing; using osu.Framework.Utils; using osu.Game.Database; using osu.Game.Online; +using osu.Game.Online.API; using osu.Game.Online.Spectator; using osu.Game.Replays.Legacy; using osu.Game.Rulesets.Osu.Scoring; @@ -105,6 +106,12 @@ namespace osu.Game.Tests.Visual.Multiplayer this.totalUsers = totalUsers; } + protected override HubClientConnector CreateConnector(string name, string endpoint, IAPIProvider api) + { + // do not pass API to prevent attempting failing connections on an actual hub. + return base.CreateConnector(name, endpoint, null); + } + public void Start(int beatmapId) { for (int i = 0; i < totalUsers; i++) From 5fb99fdc52e12845dadc48a57f043e0da4696bbd Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Thu, 11 Feb 2021 10:49:16 +0300 Subject: [PATCH 08/12] Rename some members and extract connection closure to separate method --- osu.Game/Online/HubClientConnector.cs | 35 ++++++++++--------- .../Online/Multiplayer/MultiplayerClient.cs | 2 +- .../Spectator/SpectatorStreamingClient.cs | 2 +- 3 files changed, 20 insertions(+), 19 deletions(-) diff --git a/osu.Game/Online/HubClientConnector.cs b/osu.Game/Online/HubClientConnector.cs index b740aabb92..65285882d9 100644 --- a/osu.Game/Online/HubClientConnector.cs +++ b/osu.Game/Online/HubClientConnector.cs @@ -25,7 +25,7 @@ namespace osu.Game.Online /// /// Invoked whenever a new hub connection is built. /// - public Action? OnNewConnection; + public Action? ConfigureConnection; private readonly string clientName; private readonly string endpoint; @@ -106,7 +106,7 @@ namespace osu.Game.Online try { // importantly, rebuild the connection each attempt to get an updated access token. - CurrentConnection = createConnection(cancellationToken); + CurrentConnection = buildConnection(cancellationToken); await CurrentConnection.StartAsync(cancellationToken); @@ -134,7 +134,7 @@ namespace osu.Game.Online } } - private HubConnection createConnection(CancellationToken cancellationToken) + private HubConnection buildConnection(CancellationToken cancellationToken) { Debug.Assert(api != null); @@ -152,24 +152,25 @@ namespace osu.Game.Online var newConnection = builder.Build(); - OnNewConnection?.Invoke(newConnection); - - newConnection.Closed += ex => - { - isConnected.Value = false; - - Logger.Log(ex != null ? $"{clientName} lost connection: {ex}" : $"{clientName} 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; - }; + ConfigureConnection?.Invoke(newConnection); + newConnection.Closed += ex => onConnectionClosed(ex, cancellationToken); return newConnection; } + private Task onConnectionClosed(Exception? ex, CancellationToken cancellationToken) + { + isConnected.Value = false; + + Logger.Log(ex != null ? $"{clientName} lost connection: {ex}" : $"{clientName} 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; + } + private async Task disconnect(bool takeLock) { cancelExistingConnect(); diff --git a/osu.Game/Online/Multiplayer/MultiplayerClient.cs b/osu.Game/Online/Multiplayer/MultiplayerClient.cs index f025a5b429..ba2a8d7246 100644 --- a/osu.Game/Online/Multiplayer/MultiplayerClient.cs +++ b/osu.Game/Online/Multiplayer/MultiplayerClient.cs @@ -33,7 +33,7 @@ namespace osu.Game.Online.Multiplayer { connector = new HubClientConnector(nameof(MultiplayerClient), endpoint, api) { - OnNewConnection = connection => + ConfigureConnection = connection => { // this is kind of SILLY // https://github.com/dotnet/aspnetcore/issues/15198 diff --git a/osu.Game/Online/Spectator/SpectatorStreamingClient.cs b/osu.Game/Online/Spectator/SpectatorStreamingClient.cs index 4ef59b5e47..532f717f2c 100644 --- a/osu.Game/Online/Spectator/SpectatorStreamingClient.cs +++ b/osu.Game/Online/Spectator/SpectatorStreamingClient.cs @@ -86,7 +86,7 @@ namespace osu.Game.Online.Spectator private void load(IAPIProvider api) { connector = CreateConnector(nameof(SpectatorStreamingClient), endpoint, api); - connector.OnNewConnection = connection => + connector.ConfigureConnection = connection => { // until strong typed client support is added, each method must be manually bound // (see https://github.com/dotnet/aspnetcore/issues/15198) From 18acd7f08032369f3c87d2327a3e1197b3c09d76 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Thu, 11 Feb 2021 10:51:04 +0300 Subject: [PATCH 09/12] Apply documentation suggestions Co-authored-by: Dean Herbert --- osu.Game/Online/HubClientConnector.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/osu.Game/Online/HubClientConnector.cs b/osu.Game/Online/HubClientConnector.cs index 65285882d9..71d9df84c4 100644 --- a/osu.Game/Online/HubClientConnector.cs +++ b/osu.Game/Online/HubClientConnector.cs @@ -18,7 +18,7 @@ using osu.Game.Online.API; namespace osu.Game.Online { /// - /// A component that maintains over a hub connection between client and server. + /// A component that manages the life cycle of a connection to a SignalR Hub. /// public class HubClientConnector : IDisposable { @@ -52,7 +52,7 @@ namespace osu.Game.Online /// /// The name of the client this connector connects for, used for logging. /// The endpoint to the hub. - /// The API provider for listening to state changes, or null to not listen. + /// An API provider used to react to connection state changes, or null to not establish connection at all (for testing purposes). public HubClientConnector(string clientName, string endpoint, IAPIProvider? api) { this.clientName = clientName; From d3c1b475929227b761731ab2c0f7b5c0a1ae54cd Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Thu, 11 Feb 2021 12:32:54 +0300 Subject: [PATCH 10/12] Replace nullable API with null connector instead --- .../Visual/Gameplay/TestSceneSpectator.cs | 6 +- ...TestSceneMultiplayerGameplayLeaderboard.cs | 6 +- osu.Game/Online/HubClientConnector.cs | 36 +++++----- .../Spectator/SpectatorStreamingClient.cs | 67 ++++++++++--------- 4 files changed, 53 insertions(+), 62 deletions(-) diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs index 1e499f20cb..36e7e1fb29 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs @@ -244,11 +244,7 @@ namespace osu.Game.Tests.Visual.Gameplay { } - protected override HubClientConnector CreateConnector(string name, string endpoint, IAPIProvider api) - { - // do not pass API to prevent attempting failing connections on an actual hub. - return base.CreateConnector(name, endpoint, null); - } + protected override HubClientConnector CreateConnector(string name, string endpoint, IAPIProvider api) => null; public void StartPlay(int beatmapId) { diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerGameplayLeaderboard.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerGameplayLeaderboard.cs index b459cebdd7..49abd62dba 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerGameplayLeaderboard.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerGameplayLeaderboard.cs @@ -106,11 +106,7 @@ namespace osu.Game.Tests.Visual.Multiplayer this.totalUsers = totalUsers; } - protected override HubClientConnector CreateConnector(string name, string endpoint, IAPIProvider api) - { - // do not pass API to prevent attempting failing connections on an actual hub. - return base.CreateConnector(name, endpoint, null); - } + protected override HubClientConnector CreateConnector(string name, string endpoint, IAPIProvider api) => null; public void Start(int beatmapId) { diff --git a/osu.Game/Online/HubClientConnector.cs b/osu.Game/Online/HubClientConnector.cs index 71d9df84c4..cfc4ff5d60 100644 --- a/osu.Game/Online/HubClientConnector.cs +++ b/osu.Game/Online/HubClientConnector.cs @@ -29,7 +29,7 @@ namespace osu.Game.Online private readonly string clientName; private readonly string endpoint; - private readonly IAPIProvider? api; + private readonly IAPIProvider api; /// /// The current connection opened by this connector. @@ -52,32 +52,28 @@ namespace osu.Game.Online /// /// The name of the client this connector connects for, used for logging. /// The endpoint to the hub. - /// An API provider used to react to connection state changes, or null to not establish connection at all (for testing purposes). - public HubClientConnector(string clientName, string endpoint, IAPIProvider? api) + /// An API provider used to react to connection state changes. + public HubClientConnector(string clientName, string endpoint, IAPIProvider api) { this.clientName = clientName; this.endpoint = endpoint; - this.api = api; - if (api != null) + apiState.BindTo(api.State); + apiState.BindValueChanged(state => { - apiState.BindTo(api.State); - apiState.BindValueChanged(state => + switch (state.NewValue) { - switch (state.NewValue) - { - case APIState.Failing: - case APIState.Offline: - Task.Run(() => disconnect(true)); - break; + case APIState.Failing: + case APIState.Offline: + Task.Run(() => disconnect(true)); + break; - case APIState.Online: - Task.Run(connect); - break; - } - }, true); - } + case APIState.Online: + Task.Run(connect); + break; + } + }, true); } private async Task connect() @@ -136,8 +132,6 @@ namespace osu.Game.Online private HubConnection buildConnection(CancellationToken cancellationToken) { - Debug.Assert(api != null); - var builder = new HubConnectionBuilder() .WithUrl(endpoint, options => { options.Headers.Add("Authorization", $"Bearer {api.AccessToken}"); }); diff --git a/osu.Game/Online/Spectator/SpectatorStreamingClient.cs b/osu.Game/Online/Spectator/SpectatorStreamingClient.cs index 532f717f2c..7e61da9b87 100644 --- a/osu.Game/Online/Spectator/SpectatorStreamingClient.cs +++ b/osu.Game/Online/Spectator/SpectatorStreamingClient.cs @@ -32,11 +32,12 @@ namespace osu.Game.Online.Spectator private readonly string endpoint; + [CanBeNull] private HubClientConnector connector; private readonly IBindable isConnected = new BindableBool(); - private HubConnection connection => connector.CurrentConnection; + private HubConnection connection => connector?.CurrentConnection; private readonly List watchingUsers = new List(); @@ -86,42 +87,46 @@ namespace osu.Game.Online.Spectator private void load(IAPIProvider api) { connector = CreateConnector(nameof(SpectatorStreamingClient), endpoint, api); - connector.ConfigureConnection = connection => - { - // until strong typed client support is added, each method must be manually bound - // (see https://github.com/dotnet/aspnetcore/issues/15198) - connection.On(nameof(ISpectatorClient.UserBeganPlaying), ((ISpectatorClient)this).UserBeganPlaying); - connection.On(nameof(ISpectatorClient.UserSentFrames), ((ISpectatorClient)this).UserSentFrames); - connection.On(nameof(ISpectatorClient.UserFinishedPlaying), ((ISpectatorClient)this).UserFinishedPlaying); - }; - isConnected.BindTo(connector.IsConnected); - isConnected.BindValueChanged(connected => + if (connector != null) { - if (connected.NewValue) + connector.ConfigureConnection = connection => { - // get all the users that were previously being watched - int[] users; + // until strong typed client support is added, each method must be manually bound + // (see https://github.com/dotnet/aspnetcore/issues/15198) + connection.On(nameof(ISpectatorClient.UserBeganPlaying), ((ISpectatorClient)this).UserBeganPlaying); + connection.On(nameof(ISpectatorClient.UserSentFrames), ((ISpectatorClient)this).UserSentFrames); + connection.On(nameof(ISpectatorClient.UserFinishedPlaying), ((ISpectatorClient)this).UserFinishedPlaying); + }; - lock (userLock) + isConnected.BindTo(connector.IsConnected); + isConnected.BindValueChanged(connected => + { + if (connected.NewValue) { - users = watchingUsers.ToArray(); - watchingUsers.Clear(); + // get all the users that were previously being watched + int[] users; + + lock (userLock) + { + users = watchingUsers.ToArray(); + watchingUsers.Clear(); + } + + // resubscribe to watched users. + foreach (var userId in users) + WatchUser(userId); + + // re-send state in case it wasn't received + if (isPlaying) + beginPlaying(); } - - // resubscribe to watched users. - foreach (var userId in users) - WatchUser(userId); - - // re-send state in case it wasn't received - if (isPlaying) - beginPlaying(); - } - else - { - playingUsers.Clear(); - } - }, true); + else + { + playingUsers.Clear(); + } + }, true); + } } protected virtual HubClientConnector CreateConnector(string name, string endpoint, IAPIProvider api) => new HubClientConnector(name, endpoint, api); From 37e3d95c35a67f24f66302977bb31f0906e6babc Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Thu, 11 Feb 2021 12:39:06 +0300 Subject: [PATCH 11/12] Slight reword in `ConfigureConnection`'s xmldoc --- osu.Game/Online/HubClientConnector.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Online/HubClientConnector.cs b/osu.Game/Online/HubClientConnector.cs index cfc4ff5d60..fb76049446 100644 --- a/osu.Game/Online/HubClientConnector.cs +++ b/osu.Game/Online/HubClientConnector.cs @@ -23,7 +23,7 @@ namespace osu.Game.Online public class HubClientConnector : IDisposable { /// - /// Invoked whenever a new hub connection is built. + /// Invoked whenever a new hub connection is built, to configure it before it's started. /// public Action? ConfigureConnection; From f4a7ec57e982922928ed3f327c61b228457135cc Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Thu, 11 Feb 2021 13:00:18 +0300 Subject: [PATCH 12/12] Remove unused using --- osu.Game/Online/HubClientConnector.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/osu.Game/Online/HubClientConnector.cs b/osu.Game/Online/HubClientConnector.cs index fb76049446..2298ac4243 100644 --- a/osu.Game/Online/HubClientConnector.cs +++ b/osu.Game/Online/HubClientConnector.cs @@ -4,7 +4,6 @@ #nullable enable using System; -using System.Diagnostics; using System.Threading; using System.Threading.Tasks; using Microsoft.AspNetCore.SignalR.Client;