Closes https://github.com/ppy/osu/issues/26035.
`submitOnFailOrQuit()`, as the name suggests, can be called both when
the player has failed, or when the player screen is being exited from.
Notably, when perfect mod with auto-retry is active, the two happen
almost simultaneously.
This double call exposes a data race in `submitScore()` concerning the
handling of `scoreSubmissionSource`. The race could be experimentally
confirmed by applying the following patch:
diff --git a/osu.Game/Screens/Play/SubmittingPlayer.cs b/osu.Game/Screens/Play/SubmittingPlayer.cs
index 83adf1f960..76dd29bbdb 100644
--- a/osu.Game/Screens/Play/SubmittingPlayer.cs
+++ b/osu.Game/Screens/Play/SubmittingPlayer.cs
@@ -228,6 +228,7 @@ private Task submitScore(Score score)
return Task.CompletedTask;
}
+ Logger.Log($"{nameof(scoreSubmissionSource)} is {(scoreSubmissionSource == null ? "null" : "not null")}");
if (scoreSubmissionSource != null)
return scoreSubmissionSource.Task;
@@ -237,6 +238,7 @@ private Task submitScore(Score score)
Logger.Log($"Beginning score submission (token:{token.Value})...");
+ Logger.Log($"creating new {nameof(scoreSubmissionSource)}");
scoreSubmissionSource = new TaskCompletionSource<bool>();
var request = CreateSubmissionRequest(score, token.Value);
which would result in the following log output:
[runtime] 2024-01-02 09:54:13 [verbose]: scoreSubmissionSource is null
[runtime] 2024-01-02 09:54:13 [verbose]: scoreSubmissionSource is null
[runtime] 2024-01-02 09:54:13 [verbose]: Beginning score submission (token:36780)...
[runtime] 2024-01-02 09:54:13 [verbose]: creating new scoreSubmissionSource
[runtime] 2024-01-02 09:54:13 [verbose]: Beginning score submission (token:36780)...
[runtime] 2024-01-02 09:54:13 [verbose]: creating new scoreSubmissionSource
[network] 2024-01-02 09:54:13 [verbose]: Performing request osu.Game.Online.Solo.SubmitSoloScoreRequest
[network] 2024-01-02 09:54:14 [verbose]: Request to https://dev.ppy.sh/api/v2/beatmaps/869310/solo/scores/36780 successfully completed!
[network] 2024-01-02 09:54:14 [verbose]: SubmitSoloScoreRequest finished with response size of 639 bytes
[network] 2024-01-02 09:54:14 [verbose]: Performing request osu.Game.Online.Solo.SubmitSoloScoreRequest
[runtime] 2024-01-02 09:54:14 [verbose]: Score submission completed! (token:36780 id:20247)
[network] 2024-01-02 09:54:14 [verbose]: Request to https://dev.ppy.sh/api/v2/beatmaps/869310/solo/scores/36780 successfully completed!
[network] 2024-01-02 09:54:14 [verbose]: SubmitSoloScoreRequest finished with response size of 639 bytes
[runtime] 2024-01-02 09:54:14 [error]: An unhandled error has occurred.
[runtime] 2024-01-02 09:54:14 [error]: System.InvalidOperationException: An attempt was made to transition a task to a final state when it had already completed.
[runtime] 2024-01-02 09:54:14 [error]: at osu.Game.Screens.Play.SubmittingPlayer.<>c__DisplayClass30_0.<submitScore>b__0(MultiplayerScore s) in /home/dachb/Documents/opensource/osu/osu.Game/Screens/Play/SubmittingPlayer.cs:line 250
The intention of the submission logic was to only ever create one
`scoreSubmissionSource`, and then reuse this one if a redundant
submission request was made. However, because of the temporal proximity
of fail and quit in this particular case, combined with the fact that
the calls to `submitScore()` are taking place on TPL threads, means that
there is a read-write data race on `scoreSubmissionSource`, wherein the
source can be actually created twice.
This leads to two concurrent score submission requests, which, upon
completion, attempt to transition only _the second_
`scoreSubmissionSource` to a final state (this is because the API
success/failure request callbacks capture `this`, i.e. the entire
`SubmittingPlayer` instance, rather than the `scoreSubmissionSource`
reference specifically).
To fix, ensure correct synchronisation on the read-write critical
section, which should prevent the `scoreSubmissionSource` from being
created multiple times.
Originally when popping in, the ReplayPlayer was loaded first (if previous screen was MainMenu), and afterwards the SkinEditor component was loaded asynchronously. However, if the ReplayPlayer screen exits quickly (like in the event the beatmap has no objects), the skin editor component has not finished initializing (this is before it was even added to the component tree, so it's still not marked `Visible`), then the screen exiting will cause `OsuGame` to call SetTarget(newScreen) -> setTarget(...) which sees that the cached `skinEditor` is not visible yet, and hides/nulls the field. This is the point where LoadComponentAsync(editor, ...) finishes, and the callback sees that the cached skinEditor field is now different (null) than the one that was loaded, and never adds it to the component tree. This occurrence is unhandled and as such the SkinEditorOverlay never hides itself, consuming all input infinitely.
This PR changes the loading to start loading the ReplayPlayer *after* the SkinEditor has been loaded and added to the component tree.
Additionally, this lowers the exit delay for ReplayPlayer and changes the "no hit objects" notification to not be an error since it's a controlled exit.
This allows each implementation to have control over scheduling. Without
this, the solo implementation would not be able to handle quit events
while watching a player, as it would push a child (gameplay) screen to
the stack where the `SpectatorScreen` would usually be.
A previous attempt at this was unsuccessful due to a partially
off-screen elements not getting the correct size early enough (see
https://github.com/ppy/osu/issues/14793). This can be accounted for by
setting `AlwaysPresent` when visibility is expected.
This fixes [test failures](https://github.com/ppy/osu/actions/runs/6838444698/job/18595535795)
due to the newly added `Width` / `Height` being persisted with
floating-point errors (by not persisting the values in the first place,
via `AutoSize.Both`).
The import preparation task can actually be non-null when exiting even
if the player hasn't passed:
- fail beatmap
- click import button to import the failed replay
Closes https://github.com/ppy/osu/issues/25247.
The scenario involved here is as follows:
1. User completes beatmap by hitting all notes.
2. `checkScoreCompleted()` determines completion, and calls
`progressToResults(withDelay: true)`.
3. `progressToResults()` schedules `resultsDisplayDelegate`, which
includes a call to `prepareAndImportScoreAsync()`, a second in the
future.
4. User presses quick retry hotkey.
This calls `Player.Restart(quickRestart: true)`,
which invokes `Player.RestartRequested`,
which in turn calls `PlayerLoader.restartRequested(true)`,
which in turn causes `PlayerLoader` to make itself current,
which means that `Player.OnExiting()` will get called.
5. `Player.OnExiting()` sees that `prepareScoreForDisplayTask` is null
(because `prepareAndImportScoreAsync()` - which sets it -
is scheduled to happen in the future), and as such assumes that
the score did not complete. Thus, it marks the score as failed.
6. `Player.Restart()` after invoking `RestartRequested` calls
`PerformExit(false)`, which then will unconditionally call
`prepareAndImportScoreAsync()`. But the score has already been marked
as failed.
The flow above can be described as "convoluted", but I'm not sure I have
it in me right now to try and refactor it again. Therefore, to fix,
switch the `prepareScoreForDisplayTask` null check in
`Player.OnExiting()` to check `GameplayState.HasPassed` instead, as it
is not susceptible to the same out-of-order read issue.
- Closes https://github.com/ppy/osu/issues/25248
- Possibly also closes https://github.com/ppy/osu/issues/20475
Regressed in e33486a766.
`StopUsingBeatmapClock()` intends to, as the name says, stop operating
on the working beatmap clock to yield its usage to other components on
exit. As part of that it tries to unapply audio adjustments so that
other screens can apply theirs freely instead.
However, the aforementioned commit introduced a bug in this. Previously
to it, `track` was an alias for the `SourceClock`, which could be
mutated in an indirect way via `ChangeSource()` calls. The
aforementioned commit made `track` a `readonly` field, initialised in
constructor, which would _never_ change value. In particular, it would
_always_ be the beatmap track, which meant that
`StopUsingBeatmapClock()` would remove the adjustments from the beatmap
track, but then at the end of the method, _apply them onto that same
track again_.
This was only saved by the fact that clock adjustments are removed again
on disposal of the `MasterGameplayClockContainer()`. This - due to async
disposal pressure - could explain infrequently reported cases wherein
the track would just continue to speed up ad infinitum.
To fix, fully substitute the beatmap track for a virtual track at the
point of calling `StopUsingBeatmapClock()`.
Previously, for lazer scores, the ID returned from `osu-web` was
discarded and replaced with -1, due to the fact that the appropriate
structures for unification with stable, as well as unification across
solo and multiplayer, were not in place yet.
Now we're at the point where scores from all the aforementioned sources
receive a `solo_scores` DB row, and as such, we can start treating
`solo_scores`-scheme IDs as canonical "online IDs" for a score.
It wasn't correctly checking the current underlying health, which could
be zero in usages of `AccumulatingHealthProcessor`, for instance.
Closes#25046.
I've gone ahead and matched the osu!stable behaviour for this, as it
seems like it's what people are used to and they will settle for no
less.
Closes https://github.com/ppy/osu/issues/18089.
The reasoning is explained in the inline comment, but basically this was
getting blocked by `isPaused` being in an initial `true` state (as it is
on construction), while the source clock was still `IsRunning`.
There's no real guarantee of sync between the source and the `isPaused`
bindable right now. Maybe there should be in the future, but to restore
sanity, let's ensure that a call to `Reset` can at least stop the track
as we expect.
In discussion with nanaya, we likely want this now that we're adding the
ability for some icons to be extended. Historically mod icons have been
displayed in a stable but arbitrary order (based on their position in
the `Mods` enum).
This change aims to make them stable across lazer scores (where they are
stored based on .. the order the user selected them).
This avoids it ever mutating the underlying track (aka attempting to start
it). Resolves the one caveat mentioned in
aeef92fa710648d4a00edc523e13c17ac6104125.
When watching from the middle of gameplay, due to a series of failures,
`SpectatorClock` would not get seeked to the current time, causing all
clients to look like they were out of sync.
This is a hotfix for the issue. A better fix will require framework
changes or considerable restructuring.
I'd recommend testing this works in practice and agreeing that while it
is a hack, it's likely not going to cause issues and is something we
want to see fixed sooner rather than later.
