Cancel difficulty calculation after 10 seconds by default

[smoogipoo]

The following (when run in osu-tools) will die in the new taiko code during diffcalc:

dotnet run -- difficulty 4328583 -r:1

This adds the same 10s timeout as WorkingBeatmap has for loading beatmaps, to now additionally govern the diffcalc process itself. It will apply to both osu-tools and osu-difficulty-calculator.

[peppy]

I get this is 100% a nitpick but we might want to avoid creating this when it's not being used. Just to set a good example for the rest of diffcalc (which is definitely where any allocation issue will lie 🤷).

diff --git a/osu.Game/Rulesets/Difficulty/DifficultyCalculator.cs b/osu.Game/Rulesets/Difficulty/DifficultyCalculator.cs
index add24f7866..7d01e678b1 100644
--- a/osu.Game/Rulesets/Difficulty/DifficultyCalculator.cs
+++ b/osu.Game/Rulesets/Difficulty/DifficultyCalculator.cs
@@ -62,13 +62,18 @@ public DifficultyAttributes Calculate(CancellationToken cancellationToken = defa
         /// <returns>A structure describing the difficulty of the beatmap.</returns>
         public DifficultyAttributes Calculate([NotNull] IEnumerable<Mod> mods, CancellationToken cancellationToken = default)
         {
-            using var timedCancellationSource = new CancellationTokenSource(TimeSpan.FromSeconds(10));
-
-            if (!cancellationToken.CanBeCanceled)
-                cancellationToken = timedCancellationSource.Token;
-
-            cancellationToken.ThrowIfCancellationRequested();
-            preProcess(mods, cancellationToken);
+            if (cancellationToken.CanBeCanceled)
+            {
+                cancellationToken.ThrowIfCancellationRequested();
+                preProcess(mods, cancellationToken);
+            }
+            else
+            {
+                // Difficulty cancellation can potentially get stuck in weird beatmaps, so force a timeout if no cancellation
+                // mechanism was provided.
+                using (var timedCancellationSource = new CancellationTokenSource(TimeSpan.FromSeconds(10)))
+                    preProcess(mods, timedCancellationSource.Token);
+            }
 
             var skills = CreateSkills(Beatmap, playableMods, clockRate);
 
@@ -103,19 +108,24 @@ public List<TimedDifficultyAttributes> CalculateTimed(CancellationToken cancella
         /// <returns>The set of <see cref="TimedDifficultyAttributes"/>.</returns>
         public List<TimedDifficultyAttributes> CalculateTimed([NotNull] IEnumerable<Mod> mods, CancellationToken cancellationToken = default)
         {
-            using var timedCancellationSource = new CancellationTokenSource(TimeSpan.FromSeconds(10));
-
-            if (!cancellationToken.CanBeCanceled)
-                cancellationToken = timedCancellationSource.Token;
-
-            cancellationToken.ThrowIfCancellationRequested();
-            preProcess(mods, cancellationToken);
-
             var attribs = new List<TimedDifficultyAttributes>();
 
             if (!Beatmap.HitObjects.Any())
                 return attribs;
 
+            if (cancellationToken.CanBeCanceled)
+            {
+                cancellationToken.ThrowIfCancellationRequested();
+                preProcess(mods, cancellationToken);
+            }
+            else
+            {
+                // Difficulty cancellation can potentially get stuck in weird beatmaps, so force a timeout if no cancellation
+                // mechanism was provided.
+                using (var timedCancellationSource = new CancellationTokenSource(TimeSpan.FromSeconds(10)))
+                    preProcess(mods, timedCancellationSource.Token);
+            }
+
             var skills = CreateSkills(Beatmap, playableMods, clockRate);
             var progressiveBeatmap = new ProgressiveCalculationBeatmap(Beatmap);
             var difficultyObjects = getDifficultyHitObjects().ToArray();

[smoogipoo]

The important part isn't the preProcess(), it's the usage in the loop below. In other words, the lifetime of the CTS has to be for the entire method.

One way of structuring it would be a nested method, like so:

diff --git a/osu.Game/Rulesets/Difficulty/DifficultyCalculator.cs b/osu.Game/Rulesets/Difficulty/DifficultyCalculator.cs
index add24f7866..624a8d72bf 100644
--- a/osu.Game/Rulesets/Difficulty/DifficultyCalculator.cs
+++ b/osu.Game/Rulesets/Difficulty/DifficultyCalculator.cs
@@ -62,29 +62,33 @@ public DifficultyAttributes Calculate(CancellationToken cancellationToken = defa
         /// <returns>A structure describing the difficulty of the beatmap.</returns>
         public DifficultyAttributes Calculate([NotNull] IEnumerable<Mod> mods, CancellationToken cancellationToken = default)
         {
-            using var timedCancellationSource = new CancellationTokenSource(TimeSpan.FromSeconds(10));
+            if (cancellationToken.CanBeCanceled)
+                return doTheThing(cancellationToken);
 
-            if (!cancellationToken.CanBeCanceled)
-                cancellationToken = timedCancellationSource.Token;
+            using (var timedCancellationSource = new CancellationTokenSource(TimeSpan.FromSeconds(10)))
+                return doTheThing(timedCancellationSource.Token);
 
-            cancellationToken.ThrowIfCancellationRequested();
-            preProcess(mods, cancellationToken);
+            DifficultyAttributes doTheThing(CancellationToken token)
+            {
+                token.ThrowIfCancellationRequested();
+                preProcess(mods, token);
 
-            var skills = CreateSkills(Beatmap, playableMods, clockRate);
+                var skills = CreateSkills(Beatmap, playableMods, clockRate);
 
-            if (!Beatmap.HitObjects.Any())
-                return CreateDifficultyAttributes(Beatmap, playableMods, skills, clockRate);
+                if (!Beatmap.HitObjects.Any())
+                    return CreateDifficultyAttributes(Beatmap, playableMods, skills, clockRate);
 
-            foreach (var hitObject in getDifficultyHitObjects())
-            {
-                foreach (var skill in skills)
+                foreach (var hitObject in getDifficultyHitObjects())
                 {
-                    cancellationToken.ThrowIfCancellationRequested();
-                    skill.Process(hitObject);
+                    foreach (var skill in skills)
+                    {
+                        token.ThrowIfCancellationRequested();
+                        skill.Process(hitObject);
+                    }
                 }
-            }
 
-            return CreateDifficultyAttributes(Beatmap, playableMods, skills, clockRate);
+                return CreateDifficultyAttributes(Beatmap, playableMods, skills, clockRate);
+            }
         }
 
         /// <summary>

But imo this isn't really worth it? If you disagree and feel that it is then I can push it.

[peppy]

ah right, i missed that.

let's just leave it