ppy / osu

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

Disallow running save-related operations concurrently #28420

Closed bdach closed 3 weeks ago

bdach commented 4 weeks ago

Closes https://github.com/ppy/osu/issues/25426.

Different approach to prior ones, this just disables the relevant actions when something related to save/export is going on. Still ends up being convoluted because many things you wouldn't expect to touch save do touch save, so it's not just a concern between export and save specifically.

Can be tested manually with something like

diff --git a/osu.Game/Beatmaps/BeatmapManager.cs b/osu.Game/Beatmaps/BeatmapManager.cs
index 0610f7f6fb..e51fc75681 100644
--- a/osu.Game/Beatmaps/BeatmapManager.cs
+++ b/osu.Game/Beatmaps/BeatmapManager.cs
@@ -415,9 +415,17 @@ public void UndeleteAll()
         public Task<Live<BeatmapSetInfo>?> ImportAsUpdate(ProgressNotification notification, ImportTask importTask, BeatmapSetInfo original) =>
             beatmapImporter.ImportAsUpdate(notification, importTask, original);

-        public Task Export(BeatmapSetInfo beatmap) => beatmapExporter.ExportAsync(beatmap.ToLive(Realm));
+        public async Task Export(BeatmapSetInfo beatmap)
+        {
+            await Task.Delay(30000).ConfigureAwait(false);
+            await beatmapExporter.ExportAsync(beatmap.ToLive(Realm)).ConfigureAwait(false);
+        }

-        public Task ExportLegacy(BeatmapSetInfo beatmap) => legacyBeatmapExporter.ExportAsync(beatmap.ToLive(Realm));
+        public async Task ExportLegacy(BeatmapSetInfo beatmap)
+        {
+            await Task.Delay(30000).ConfigureAwait(false);
+            await legacyBeatmapExporter.ExportAsync(beatmap.ToLive(Realm)).ConfigureAwait(false);
+        }

         private void updateHashAndMarkDirty(BeatmapSetInfo setInfo)
         {

Only the export side can/should be tested like that, doing the same for the save side is kinda pointless since beatmap save is actually hard-sync right now, so there is no real threat of concurrent execution (but all call sites that call save need to be guarded anyway, to protect from async export).

peppy commented 4 weeks ago

This change is a bit hard to swallow in terms of code quality.

I'm wondering if a method like AttemptSaveWithPostOperation(Action postOperation) could help simplify this? where the InProgress check is always performed inside that method rather than at each usage.

bdach commented 3 weeks ago

I'm wondering if a method like AttemptSaveWithPostOperation(Action postOperation) could help simplify this?

I was partially grimacing when writing the initial version of this diff because of similar concerns but also didn't want to try to abstract out too much because all of the weird variants of calling save did not really seem conducive to a common usage pattern.

That said, I tried; https://github.com/ppy/osu/pull/28420/commits/1d6b7e9c9b860a4a7b8efd518b6724146e0d92ca is probably as nice as I'm able to make it.

peppy commented 3 weeks ago

I can live with this now.