mirror of
https://github.com/ppy/osu.git
synced 2024-12-14 07:33:20 +08:00
Fix possible double score submission when auto-retrying via perfect mod
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.
This commit is contained in:
parent
5b8e9a5bd8
commit
3c5e9ac9a9
@ -41,6 +41,7 @@ namespace osu.Game.Screens.Play
|
||||
[Resolved]
|
||||
private SessionStatics statics { get; set; }
|
||||
|
||||
private readonly object scoreSubmissionLock = new object();
|
||||
private TaskCompletionSource<bool> scoreSubmissionSource;
|
||||
|
||||
protected SubmittingPlayer(PlayerConfiguration configuration = null)
|
||||
@ -228,16 +229,19 @@ namespace osu.Game.Screens.Play
|
||||
return Task.CompletedTask;
|
||||
}
|
||||
|
||||
if (scoreSubmissionSource != null)
|
||||
return scoreSubmissionSource.Task;
|
||||
lock (scoreSubmissionLock)
|
||||
{
|
||||
if (scoreSubmissionSource != null)
|
||||
return scoreSubmissionSource.Task;
|
||||
|
||||
scoreSubmissionSource = new TaskCompletionSource<bool>();
|
||||
}
|
||||
|
||||
// if the user never hit anything, this score should not be counted in any way.
|
||||
if (!score.ScoreInfo.Statistics.Any(s => s.Key.IsHit() && s.Value > 0))
|
||||
return Task.CompletedTask;
|
||||
|
||||
Logger.Log($"Beginning score submission (token:{token.Value})...");
|
||||
|
||||
scoreSubmissionSource = new TaskCompletionSource<bool>();
|
||||
var request = CreateSubmissionRequest(score, token.Value);
|
||||
|
||||
request.Success += s =>
|
||||
|
Loading…
Reference in New Issue
Block a user