Do not send replay frames to spectator server if initial begin play invocation failed#37159
Open
bdach wants to merge 2 commits intoppy:masterfrom
Open
Do not send replay frames to spectator server if initial begin play invocation failed#37159bdach wants to merge 2 commits intoppy:masterfrom
bdach wants to merge 2 commits intoppy:masterfrom
Conversation
…nvocation failed 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 ignored as no-ops: https://github.com/ppy/osu-server-spectator/blob/7bab117e9d161455485368f63a0607a9e53f9f8a/osu.Server.Spectator/Hubs/Spectator/SpectatorHub.cs#L122-L125 https://github.com/ppy/osu-server-spectator/blob/7bab117e9d161455485368f63a0607a9e53f9f8a/osu.Server.Spectator/Hubs/Spectator/SpectatorHub.cs#L153-L157 - 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.
Member
|
LGTM |
Collaborator
Author
|
Tests not looking very good, checking. |
…from begin play failure
Collaborator
Author
|
Above commit should do it I think, but I'll wait for CI to confirm before unblocking... As per inline comment in said commit, one way to reproduce is diff --git a/osu.Game/Online/Spectator/OnlineSpectatorClient.cs b/osu.Game/Online/Spectator/OnlineSpectatorClient.cs
index 22f8cfbf2b..a9f9de812c 100644
--- a/osu.Game/Online/Spectator/OnlineSpectatorClient.cs
+++ b/osu.Game/Online/Spectator/OnlineSpectatorClient.cs
@@ -59,10 +59,12 @@ protected override async Task<bool> BeginPlayingInternal(long? scoreToken, Spect
Debug.Assert(connection != null);
+ await Task.Delay(10000).ConfigureAwait(false);
+
try
{
await connection.InvokeAsync(nameof(ISpectatorServer.BeginPlaySession), scoreToken, state).ConfigureAwait(false);
- return true;
+ return false;
}
catch (Exception exception)
{
For reference, logging with the aforementioned commit shows the following sequence of events: |
Collaborator
Author
|
CI still 4 red, but this time it looks like I just got unlucky. Rerunning to be sure. |
peppy
reviewed
Mar 31, 2026
|
|
||
| if (!isPlaying) | ||
| { | ||
| // it is possible for this to happen if the `BeginPlayingInternal()` call takes a long time, |
Member
There was a problem hiding this comment.
The question I pose is that what if it takes a long time but doesn't fail? We are potentially dropping frames from the start of a recorded replay... unless I'm missing something.
Collaborator
Author
There was a problem hiding this comment.
I've not looked into it. If it's a real shortcoming, it's already in master.
My hope based on what I know about signalr delivery guarantees would be that signalr side invocation ordering would delay / queue the client frame sending calls until the begin play call succeeds, but I've not tested. I can try testing it tomorrow.
Member
There was a problem hiding this comment.
The difference on master may be the clearing of frames which was added here. But yeah, let's investigate this one before pushing this out.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 callEndPlaySession()at the end of a session.Server-side, two things generally can happen after this:
EndPlaySession()call are completely ignored as no-ops, orClientVersionChecker) that failed the initialBeginPlaySession()call continues to fail the calls toSendFrameData()andEndPlaySession(), all the while creating a storm in logs, because it needs to throwHubExceptions 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.