ppy / osu

rhythm is just a *click* away!
https://osu.ppy.sh
MIT License
15.32k stars 2.28k forks source link

Asynchronous data update in tournament clients cause exceptions #24136

Closed cdwcgt closed 1 year ago

cdwcgt commented 1 year ago

Type

Crash to desktop

Bug description

regress from https://github.com/ppy/osu/pull/24037

after https://github.com/ppy/osu/pull/24037/commits/1b671b8c6e3296d373d168f643745af660a81399, beatmap update will cause exceptions

Can't use GetResultSafely from inside an async operation.

this is because the code below

https://github.com/ppy/osu/blob/1b671b8c6e3296d373d168f643745af660a81399/osu.Game.Tournament/TournamentGameBase.cs#L85-L95

readBracket() is running at thread pool so addRoundBeatmaps() and addSeedingBeatmaps() will also run in this. so it cannot use GetResultSafely()

we can use ContinueWith to solve it like

        private bool addRoundBeatmaps()
        {
            var beatmapsRequiringPopulation = ladder.Rounds
                                                    .SelectMany(r => r.Beatmaps)
                                                    .Where(b => b.Beatmap?.OnlineID == 0 && b.ID > 0).ToList();

            if (beatmapsRequiringPopulation.Count == 0)
                return false;

            int count = 0;

            foreach (RoundBeatmap b in beatmapsRequiringPopulation)
            {
                RoundBeatmap b1 = b;
                beatmapCache.GetBeatmapAsync(b.ID).ContinueWith(task =>
                {
                    b1.Beatmap = new TournamentBeatmap(task.GetResultSafely() ?? new APIBeatmap());
                    count++;
                    updateLoadProgressMessage($"Populating round beatmaps ({count} / {beatmapsRequiringPopulation.Count})");
                });
            }

            return true;
        }

but there will something problem with updateLoadProgressMessage and saveChanges. because thread will not wait, so addRoundBeatmaps() and addSeedingBeatmaps() will run together, so these two method will update progress message together, also it will cause saveChanges() early execution before beatmap updated.

Screenshots or videos

No response

Version

commit 1b671b8c6e3296d373d168f643745af660a81399

Logs

nan

peppy commented 1 year ago

Where are the logs?

Where's the call stack?

Could you try posting the actual issue rather than an essay about a potential cause / solution? It would better allow actually taking action.