From 8a0b975d71d08f6a845b10b46aec50d714d00c68 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 14 Jan 2021 18:25:32 +0900 Subject: [PATCH] Fix deadlock scenario when calculating fallback difficulty The previous code would run a calcaulation for the beatmap's own ruleset if the current one failed. While this does make sense, with the current way we use this component (and the implementation flow) it is quite unsafe. The to the call on `.Result` in the `catch` block, this would 100% deadlock due to the thread concurrency of the `ThreadedTaskScheduler` being 1. Even if the nested run could be run inline (it should be), the task scheduler won't even get to the point of checking whether this is feasible due to it being saturated by the already running task. I'm not sure if we still need this fallback lookup logic. After removing it, it's feasible that 0 stars will be returned during the scenario that previously caused a deadlock, but I don't necessarily think this is incorrect. There may be another reason for this needing to exist which I'm not aware of (diffcalc?) but if that's the case we may want to move the try-catch handling to the point of usage. To reproduce the deadlock scenario with 100% success (the repro instructions in the linked issue aren't that simple and require some patience and good timing), the main portion of the lookup can be changed to randomly trigger a nested lookup: ``` if (RNG.NextSingle() > 0.5f) return GetAsync(new DifficultyCacheLookup(key.Beatmap, key.Beatmap.Ruleset, key.OrderedMods)).Result; else return new StarDifficulty(attributes); ``` After switching beatmap once or twice, pausing debug and viewing the state of threads should show exactly what is going on. --- osu.Game/Beatmaps/BeatmapDifficultyCache.cs | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/osu.Game/Beatmaps/BeatmapDifficultyCache.cs b/osu.Game/Beatmaps/BeatmapDifficultyCache.cs index 3b58062add..37d262abe5 100644 --- a/osu.Game/Beatmaps/BeatmapDifficultyCache.cs +++ b/osu.Game/Beatmaps/BeatmapDifficultyCache.cs @@ -260,17 +260,10 @@ namespace osu.Game.Beatmaps } catch (BeatmapInvalidForRulesetException e) { - // Conversion has failed for the given ruleset, so return the difficulty in the beatmap's default ruleset. - - // Ensure the beatmap's default ruleset isn't the one already being converted to. - // This shouldn't happen as it means something went seriously wrong, but if it does an endless loop should be avoided. if (rulesetInfo.Equals(beatmapInfo.Ruleset)) - { Logger.Error(e, $"Failed to convert {beatmapInfo.OnlineBeatmapID} to the beatmap's default ruleset ({beatmapInfo.Ruleset})."); - return new StarDifficulty(); - } - return GetAsync(new DifficultyCacheLookup(key.Beatmap, key.Beatmap.Ruleset, key.OrderedMods)).Result; + return new StarDifficulty(); } catch {