Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Asynchronous data update in tournament clients cause exceptions #24136

Closed
cdwcgt opened this issue Jul 6, 2023 · 1 comment · Fixed by #24139
Closed

Asynchronous data update in tournament clients cause exceptions #24136

cdwcgt opened this issue Jul 6, 2023 · 1 comment · Fixed by #24139
Assignees
Labels
area:tournament priority:0 Showstopper. Critical to the next release.

Comments

@cdwcgt
Copy link
Contributor

cdwcgt commented Jul 6, 2023

Type

Crash to desktop

Bug description

regress from #24037

after 1b671b8, beatmap update will cause exceptions

Can't use GetResultSafely from inside an async operation.

this is because the code below

protected override void LoadComplete()
{
GlobalCursorDisplay.MenuCursor.AlwaysPresent = true; // required for tooltip display
// we don't want to show the menu cursor as it would appear on stream output.
GlobalCursorDisplay.MenuCursor.Alpha = 0;
base.LoadComplete();
Task.Run(readBracket);
}

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 1b671b8

Logs

nan

@peppy
Copy link
Member

peppy commented Jul 6, 2023

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.

peppy added a commit to peppy/osu that referenced this issue Jul 6, 2023
@peppy peppy added priority:0 Showstopper. Critical to the next release. area:tournament labels Jul 6, 2023
@peppy peppy self-assigned this Jul 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:tournament priority:0 Showstopper. Critical to the next release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants