h1or4dme8/osu-lazer
1
0
Fork 0
mirror of https://github.com/ppy/osu.git synced 2026-05-13 20:33:35 +08:00
Code Issues Releases Wiki Activity

Do not send replay frames to spectator server if initial begin play invocation failed (#37159)

Browse Source 
RFC

Until now, if the initial `BeginPlaySession()` call failed, the client
would continue operating as if it didn't - it would still continue to
send frames and call `EndPlaySession()` at the end of a session.

Server-side, two things generally can happen after this:

- The sent frames and the `EndPlaySession()` call are
[completely](https://github.com/ppy/osu-server-spectator/blob/7bab117e9d161455485368f63a0607a9e53f9f8a/osu.Server.Spectator/Hubs/Spectator/SpectatorHub.cs#L122-L125)
[ignored](https://github.com/ppy/osu-server-spectator/blob/7bab117e9d161455485368f63a0607a9e53f9f8a/osu.Server.Spectator/Hubs/Spectator/SpectatorHub.cs#L153-L157)
as no-ops, or
- A hub filter (like `ClientVersionChecker`) that failed the initial
`BeginPlaySession()` call continues to fail the calls to
`SendFrameData()` and `EndPlaySession()`, all the while creating a storm
in logs, because it needs to throw `HubException`s to communicate to
users that they need to update their game, and the exceptions can't be
silenced from logs because they look like every other failure.

To that end, this has two goals: reduce useless network traffic, and
reduce noise in spectator server logs after the client version checks
were recently reactivated.

Probably needs tests, but unsure if everyone's going to be on board with
this to begin with to be quite frank, so I'm leaving tests for when I'm
told this needs tests.
This commit is contained in:
Bartłomiej Dach
2026-04-08 20:40:32 +02:00
committed by GitHub
Unverified
parent 8c6818e275
commit ca44d8233d
3 changed files with 69 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files
osu.Game/Online/Spectator/OnlineSpectatorClient.cs
+9 -4
View File
@@ -7,6 +7,7 @@ using System.Threading.Tasks;
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 @@ namespace osu.Game.Online.Spectator
}
}
protected override async Task BeginPlayingInternal(long? scoreToken, SpectatorState state)
protected override async Task<bool> 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 @@ namespace osu.Game.Online.Spectator
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;
}
}
osu.Game/Online/Spectator/SpectatorClient.cs
+57 -13
View File
@@ -10,6 +10,7 @@ using JetBrains.Annotations;
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 @@ namespace osu.Game.Online.Spectator
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 @@ namespace osu.Game.Online.Spectator
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);
BeginPlayingInternal(currentScoreToken, currentState).ContinueWith(t =>
{
bool success = t.GetResultSafely();
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 @@ namespace osu.Game.Online.Spectator
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 @@ namespace osu.Game.Online.Spectator
});
}
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 @@ namespace osu.Game.Online.Spectator
});
}
protected abstract Task BeginPlayingInternal(long? scoreToken, SpectatorState state);
/// <summary>
/// Contains the actual implementation of the "begin play" operation.
/// </summary>
/// <returns>Whether the server-side invocation to start play succeeded.</returns>
protected abstract Task<bool> BeginPlayingInternal(long? scoreToken, SpectatorState state);
protected abstract Task SendFramesInternal(FrameDataBundle bundle);
@@ -355,6 +389,16 @@ namespace osu.Game.Online.Spectator
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);
osu.Game/Tests/Visual/Spectator/TestSpectatorClient.cs
+3 -2
View File
@@ -159,14 +159,15 @@ namespace osu.Game.Tests.Visual.Spectator
}
}
protected override Task BeginPlayingInternal(long? scoreToken, SpectatorState state)
protected override async Task<bool> 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)