diff --git a/osu.Game/Online/Spectator/OnlineSpectatorClient.cs b/osu.Game/Online/Spectator/OnlineSpectatorClient.cs index 29d174f8e3ca..22f8cfbf2b44 100644 --- a/osu.Game/Online/Spectator/OnlineSpectatorClient.cs +++ b/osu.Game/Online/Spectator/OnlineSpectatorClient.cs @@ -7,6 +7,7 @@ using Microsoft.AspNetCore.SignalR.Client; using osu.Framework.Allocation; using osu.Framework.Bindables; +using osu.Framework.Logging; using osu.Game.Online.API; using osu.Game.Online.Multiplayer; @@ -51,16 +52,17 @@ private void load(IAPIProvider api) } } - protected override async Task BeginPlayingInternal(long? scoreToken, SpectatorState state) + protected override async Task BeginPlayingInternal(long? scoreToken, SpectatorState state) { if (!IsConnected.Value) - return; + return false; Debug.Assert(connection != null); try { await connection.InvokeAsync(nameof(ISpectatorServer.BeginPlaySession), scoreToken, state).ConfigureAwait(false); + return true; } catch (Exception exception) { @@ -69,11 +71,14 @@ protected override async Task BeginPlayingInternal(long? scoreToken, SpectatorSt Debug.Assert(connector != null); await connector.Reconnect().ConfigureAwait(false); - await BeginPlayingInternal(scoreToken, state).ConfigureAwait(false); + return await BeginPlayingInternal(scoreToken, state).ConfigureAwait(false); } // Exceptions can occur if, for instance, the locally played beatmap doesn't have a server-side counterpart. - // For now, let's ignore these so they don't cause unobserved exceptions to appear to the user (and sentry). + // For now, let's ignore these so they don't cause unobserved exceptions to appear to the user (and sentry), + // but log to disk for diagnostic purposes. + Logger.Log($"{nameof(OnlineSpectatorClient)}.{nameof(BeginPlayingInternal)} failed: {exception.Message}", LoggingTarget.Network); + return false; } } diff --git a/osu.Game/Online/Spectator/SpectatorClient.cs b/osu.Game/Online/Spectator/SpectatorClient.cs index f245e8cf3a27..9489fb4ad057 100644 --- a/osu.Game/Online/Spectator/SpectatorClient.cs +++ b/osu.Game/Online/Spectator/SpectatorClient.cs @@ -10,6 +10,7 @@ using osu.Framework.Allocation; using osu.Framework.Bindables; using osu.Framework.Development; +using osu.Framework.Extensions; using osu.Framework.Graphics; using osu.Framework.Logging; using osu.Game.Beatmaps; @@ -216,8 +217,6 @@ public void BeginPlaying(long? scoreToken, GameplayState state, Score score) if (isPlaying) throw new InvalidOperationException($"Cannot invoke {nameof(BeginPlaying)} when already playing"); - isPlaying = true; - // transfer state at point of beginning play currentState.BeatmapID = score.ScoreInfo.BeatmapInfo!.OnlineID; currentState.RulesetID = score.ScoreInfo.RulesetID; @@ -225,12 +224,27 @@ public void BeginPlaying(long? scoreToken, GameplayState state, Score score) currentState.State = SpectatedUserState.Playing; currentState.MaximumStatistics = state.ScoreProcessor.MaximumStatistics; - currentBeatmap = state.Beatmap; - currentScore = score; - currentScoreToken = scoreToken; - currentScoreProcessor = state.ScoreProcessor; + setStateForScore(scoreToken, state, score); + + BeginPlayingInternal(currentScoreToken, currentState).ContinueWith(t => + { + bool success = t.GetResultSafely(); - BeginPlayingInternal(currentScoreToken, currentState); + if (!success) + { + Logger.Log($"Clearing {nameof(SpectatorClient)} state due to failed {nameof(BeginPlayingInternal)} call."); + Schedule(() => + { + clearScoreState(); + + currentState.BeatmapID = null; + currentState.RulesetID = null; + currentState.Mods = []; + currentState.State = SpectatedUserState.Idle; + currentState.MaximumStatistics = []; + }); + } + }); }); } @@ -278,11 +292,7 @@ public void EndPlaying(GameplayState state) if (pendingFrames.Count > 0) purgePendingFrames(); - isPlaying = false; - currentBeatmap = null; - currentScore = null; - currentScoreProcessor = null; - currentScoreToken = null; + clearScoreState(); if (state.HasPassed) currentState.State = SpectatedUserState.Passed; @@ -295,6 +305,26 @@ public void EndPlaying(GameplayState state) }); } + private void setStateForScore(long? scoreToken, GameplayState state, Score score) + { + isPlaying = true; + + currentBeatmap = state.Beatmap; + currentScore = score; + currentScoreToken = scoreToken; + currentScoreProcessor = state.ScoreProcessor; + } + + private void clearScoreState() + { + isPlaying = false; + + currentBeatmap = null; + currentScore = null; + currentScoreProcessor = null; + currentScoreToken = null; + } + public virtual void WatchUser(int userId) { Debug.Assert(ThreadSafety.IsUpdateThread); @@ -326,7 +356,11 @@ public void StopWatchingUser(int userId) }); } - protected abstract Task BeginPlayingInternal(long? scoreToken, SpectatorState state); + /// + /// Contains the actual implementation of the "begin play" operation. + /// + /// Whether the server-side invocation to start play succeeded. + protected abstract Task BeginPlayingInternal(long? scoreToken, SpectatorState state); protected abstract Task SendFramesInternal(FrameDataBundle bundle); @@ -355,6 +389,16 @@ private void purgePendingFrames() if (pendingFrames.Count == 0) return; + if (!isPlaying) + { + // it is possible for this to happen if the `BeginPlayingInternal()` call takes a long time, + // the client accumulates a purgeable bundle of frames in the meantime, + // and then `BeginPlayingInternal()` finally fails and `clearScoreState()` is called to abort the streaming session. + Logger.Log($"{nameof(SpectatorClient)} dropping pending frames as the user is no longer considered to be playing."); + pendingFrames.Clear(); + return; + } + Debug.Assert(currentScore != null); Debug.Assert(currentScoreProcessor != null); diff --git a/osu.Game/Tests/Visual/Spectator/TestSpectatorClient.cs b/osu.Game/Tests/Visual/Spectator/TestSpectatorClient.cs index 5d33afd2881a..0eb1e7c28575 100644 --- a/osu.Game/Tests/Visual/Spectator/TestSpectatorClient.cs +++ b/osu.Game/Tests/Visual/Spectator/TestSpectatorClient.cs @@ -159,14 +159,15 @@ void flush() } } - protected override Task BeginPlayingInternal(long? scoreToken, SpectatorState state) + protected override async Task BeginPlayingInternal(long? scoreToken, SpectatorState state) { // Track the local user's playing beatmap ID. Debug.Assert(state.BeatmapID != null); userBeatmapDictionary[api.LocalUser.Value.Id] = state.BeatmapID.Value; userModsDictionary[api.LocalUser.Value.Id] = state.Mods.ToArray(); - return ((ISpectatorClient)this).UserBeganPlaying(api.LocalUser.Value.Id, state); + await ((ISpectatorClient)this).UserBeganPlaying(api.LocalUser.Value.Id, state).ConfigureAwait(false); + return true; } protected override Task SendFramesInternal(FrameDataBundle bundle)