h1or4dme8/osu-lazer
1
0
Fork 0
mirror of https://github.com/ppy/osu.git synced 2026-05-13 20:33:35 +08:00
Code Issues Releases Wiki Activity

Fix race condition in beatmap difficulty cache invalidation flow (#37178)

Browse Source 
Exposed by CI failures
([example](https://github.com/ppy/osu/actions/runs/23888446400#user-content-r0s0)).

The race occurs when a consumer calls `GetBindableDifficulty()` for the
first time and then a cache invalidation is triggered. The sequence of
events triggering the failure is as follows:

1. Consumer calls `GetBindableDifficulty()` to get a difficulty bindable
for a given beatmap tracking the game-global ruleset / mods. This
triggers difficulty calculation A.
2. In the meantime, another process requests a cache invalidation for
the same beatmap as the one supplied by the consumer in step (1). This
incurs a cache purge and triggers difficulty calculation B, but never
cancels difficulty calculation A.
3. Difficulty calculation B concludes and writes the correct, latest
difficulty value to the bindable.
4. Difficulty calculation A concludes and writes an incorrect, stale
difficulty value to the bindable.

See below for patch that reproduces this behaviour on my machine 100%
reliably:

```diff
diff --git a/osu.Game/Beatmaps/BeatmapDifficultyCache.cs b/osu.Game/Beatmaps/BeatmapDifficultyCache.cs
index https://github.com/ppy/osu/commit/d6b40639161e26af223f03761b3826b0cd7f4a87..c9604e0e58 100644
--- a/osu.Game/Beatmaps/BeatmapDifficultyCache.cs
+++ b/osu.Game/Beatmaps/BeatmapDifficultyCache.cs
@@ -252,17 +252,17 @@ private void updateBindable(BindableStarDifficulty bindable, IRulesetInfo? rules
             GetDifficultyAsync(bindable.BeatmapInfo, rulesetInfo, mods, cancellationToken, computationDelay)
                 .ContinueWith(task =>
                 {
+                    StarDifficulty? starDifficulty = task.GetResultSafely();
+
                     // We're on a threadpool thread, but we should exit back to the update thread so consumers can safely handle value-changed events.
-                    Schedule(() =>
+                    Scheduler.AddDelayed(() =>
                     {
                         if (cancellationToken.IsCancellationRequested)
                             return;

-                        StarDifficulty? starDifficulty = task.GetResultSafely();
-
                         if (starDifficulty != null)
                             bindable.Value = starDifficulty.Value;
-                    });
+                    }, starDifficulty?.Stars > 0 ? 400 : 0);
                 }, cancellationToken);
         }

```

The goal of the patch is to reorder the write to the bindable in order
to trigger the scenario described above.

Due to the invasiveness of the patch it is not suitable to add as a
test, and chances are that the schedule delay may need to be tweaked if
anyone else wants to check that patch.

Anyway, the solution here is to use the same pattern of creating a
linked cancellation token even on the first retrieval of a bindable
difficulty, and registering it in the list of cancellation tokens that
already existed to service the ruleset- / mod-tracking flow.

Some extra rearranging in
https://github.com/ppy/osu/commit/9184299239a8b5e82957d46db33f1d26bab238fd
is needed to ensure the linked tokens created to do this don't stay
behind after they are no longer needed for anything.
This commit is contained in:
Bartłomiej Dach
2026-04-03 14:57:07 +02:00
committed by GitHub
Unverified
parent 31c1168d76
commit ce8406613a
1 changed files with 39 additions and 16 deletions
Download Patch File Download Diff File Expand all files Collapse all files
osu.Game/Beatmaps/BeatmapDifficultyCache.cs
+39 -16
View File
@@ -137,10 +137,15 @@ namespace osu.Game.Beatmaps
Value = new StarDifficulty(beatmapInfo.StarRating, 0)
};
updateBindable(bindable, currentRuleset.Value, currentMods.Value, cancellationToken, computationDelay);
lock (bindableUpdateLock)
{
var linkedSource = CancellationTokenSource.CreateLinkedTokenSource(trackedUpdateCancellationSource.Token, cancellationToken);
linkedCancellationSources.Add(linkedSource);
updateBindable(bindable, currentRuleset.Value, currentMods.Value, linkedSource, computationDelay);
trackedBindables.Add(bindable);
}
return bindable;
}
@@ -212,7 +217,7 @@ namespace osu.Game.Beatmaps
var linkedSource = CancellationTokenSource.CreateLinkedTokenSource(trackedUpdateCancellationSource.Token, b.CancellationToken);
linkedCancellationSources.Add(linkedSource);
updateBindable(b, currentRuleset.Value, currentMods.Value, linkedSource.Token);
updateBindable(b, currentRuleset.Value, currentMods.Value, linkedSource);
}
}
}
@@ -243,27 +248,45 @@ namespace osu.Game.Beatmaps
/// <param name="bindable">The <see cref="BindableStarDifficulty"/> to update.</param>
/// <param name="rulesetInfo">The <see cref="IRulesetInfo"/> to update with.</param>
/// <param name="mods">The <see cref="Mod"/>s to update with.</param>
/// <param name="cancellationToken">A token that may be used to cancel this update.</param>
/// <param name="linkedCancellationTokenSource">
/// A cancellation token source that may be used to cancel this update.
/// This token will be cancelled in one of two scenarios:
/// <list type="bullet">
/// <item>The owner of the bindable has requested the cancellation.</item>
/// <item>An <see cref="Invalidate"/> call has been issued, and as such ongoing calculations must be aborted to avoid stale values being potentially written to bindables.</item>
/// </list>
/// </param>
/// <param name="computationDelay">In the case a cached lookup was not possible, a value in milliseconds of to wait until performing potentially intensive lookup.</param>
private void updateBindable(BindableStarDifficulty bindable, IRulesetInfo? rulesetInfo, IEnumerable<Mod>? mods, CancellationToken cancellationToken = default, int computationDelay = 0)
private void updateBindable(BindableStarDifficulty bindable, IRulesetInfo? rulesetInfo, IEnumerable<Mod>? mods, CancellationTokenSource linkedCancellationTokenSource, int computationDelay = 0)
{
// GetDifficultyAsync will fall back to existing data from IBeatmapInfo if not locally available
// (contrary to GetAsync)
GetDifficultyAsync(bindable.BeatmapInfo, rulesetInfo, mods, cancellationToken, computationDelay)
GetDifficultyAsync(bindable.BeatmapInfo, rulesetInfo, mods, linkedCancellationTokenSource.Token, computationDelay)
.ContinueWith(task =>
{
// We're on a threadpool thread, but we should exit back to the update thread so consumers can safely handle value-changed events.
Schedule(() =>
{
if (cancellationToken.IsCancellationRequested)
return;
// We're on a threadpool thread, but we should exit back to the update thread so consumers can safely handle value-changed events.
Schedule(() =>
{
if (!linkedCancellationTokenSource.IsCancellationRequested)
{
StarDifficulty? starDifficulty = task.GetResultSafely();
StarDifficulty? starDifficulty = task.GetResultSafely();
if (starDifficulty != null)
bindable.Value = starDifficulty.Value;
}
if (starDifficulty != null)
bindable.Value = starDifficulty.Value;
});
}, cancellationToken);
// Once the linked cancellation token source is of no remaining use to anybody, clean it up.
lock (bindableUpdateLock)
{
linkedCancellationSources.Remove(linkedCancellationTokenSource);
linkedCancellationTokenSource.Dispose();
}
});
},
// This continuation MUST run even if the antecedent `GetDifficultyAsync()` call was canceled in order to clean up `linkedCancellationTokenSource`.
// Due to this, `ContinueWith()` CANNOT accept `linkedCancellationTokenSource.Token` here, because if it did, then in an event of a cancellation,
// the continuation would never be scheduled for execution.
CancellationToken.None);
}
/// <summary>