From be5cb209e502c1df833ad7dc542c6fc4a1c8a846 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 23 Oct 2024 17:51:29 +0900 Subject: [PATCH 1/5] Update notification text when import is paused due to gameplay Addresses https://github.com/ppy/osu/discussions/30388. --- osu.Game/Database/RealmArchiveModelImporter.cs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/osu.Game/Database/RealmArchiveModelImporter.cs b/osu.Game/Database/RealmArchiveModelImporter.cs index cf0625c51c99..a27e35fc4ee6 100644 --- a/osu.Game/Database/RealmArchiveModelImporter.cs +++ b/osu.Game/Database/RealmArchiveModelImporter.cs @@ -105,7 +105,6 @@ public async Task>> Import(ProgressNotification notific } notification.Progress = 0; - notification.Text = $"{HumanisedModelName.Humanize(LetterCasing.Title)} import is initialising..."; int current = 0; @@ -113,6 +112,18 @@ public async Task>> Import(ProgressNotification notific parameters.Batch |= tasks.Length >= minimum_items_considered_batch_import; + // A paused state could obviously be entered mid-import (during the `Task.WhenAll` below), + // but in order to keep things simple let's focus on the most common scenario. + notification.Text = $"{HumanisedModelName.Humanize(LetterCasing.Title)} import is paused due to gameplay..."; + + try + { + pauseIfNecessary(parameters, notification.CancellationToken); + } + catch { } + + notification.Text = $"{HumanisedModelName.Humanize(LetterCasing.Title)} import is initialising..."; + await Task.WhenAll(tasks.Select(async task => { if (notification.CancellationToken.IsCancellationRequested) From 61f0cfd688f45d8bc02ca24a71566ed662aa8f56 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 24 Oct 2024 17:56:51 +0900 Subject: [PATCH 2/5] Make flow more `async` to avoid any chance of deadlocks --- .../Database/RealmArchiveModelImporter.cs | 38 +++++++++---------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/osu.Game/Database/RealmArchiveModelImporter.cs b/osu.Game/Database/RealmArchiveModelImporter.cs index a27e35fc4ee6..200c2051fcbb 100644 --- a/osu.Game/Database/RealmArchiveModelImporter.cs +++ b/osu.Game/Database/RealmArchiveModelImporter.cs @@ -112,26 +112,20 @@ public async Task>> Import(ProgressNotification notific parameters.Batch |= tasks.Length >= minimum_items_considered_batch_import; - // A paused state could obviously be entered mid-import (during the `Task.WhenAll` below), - // but in order to keep things simple let's focus on the most common scenario. - notification.Text = $"{HumanisedModelName.Humanize(LetterCasing.Title)} import is paused due to gameplay..."; - - try - { - pauseIfNecessary(parameters, notification.CancellationToken); - } - catch { } - notification.Text = $"{HumanisedModelName.Humanize(LetterCasing.Title)} import is initialising..."; + notification.State = ProgressNotificationState.Active; + + await pauseIfNecessaryAsync(parameters, notification, notification.CancellationToken).ConfigureAwait(false); - await Task.WhenAll(tasks.Select(async task => + await Parallel.ForEachAsync(tasks, notification.CancellationToken, async (task, cancellation) => { - if (notification.CancellationToken.IsCancellationRequested) - return; + cancellation.ThrowIfCancellationRequested(); try { - var model = await Import(task, parameters, notification.CancellationToken).ConfigureAwait(false); + await pauseIfNecessaryAsync(parameters, notification, cancellation).ConfigureAwait(false); + + var model = await Import(task, parameters, cancellation).ConfigureAwait(false); lock (imported) { @@ -150,7 +144,7 @@ await Task.WhenAll(tasks.Select(async task => { Logger.Error(e, $@"Could not import ({task})", LoggingTarget.Database); } - })).ConfigureAwait(false); + }).ConfigureAwait(false); if (imported.Count == 0) { @@ -297,8 +291,6 @@ public async Task> BeginExternalEditing(TModel mod /// An optional cancellation token. public virtual Live? ImportModel(TModel item, ArchiveReader? archive = null, ImportParameters parameters = default, CancellationToken cancellationToken = default) => Realm.Run(realm => { - pauseIfNecessary(parameters, cancellationToken); - TModel? existing; if (parameters.Batch && archive != null) @@ -586,21 +578,29 @@ protected virtual void UndeleteForReuse(TModel existing) /// Whether to perform deletion. protected virtual bool ShouldDeleteArchive(string path) => false; - private void pauseIfNecessary(ImportParameters importParameters, CancellationToken cancellationToken) + private async Task pauseIfNecessaryAsync(ImportParameters importParameters, ProgressNotification notification, CancellationToken cancellationToken) { if (!PauseImports || importParameters.ImportImmediately) return; Logger.Log($@"{GetType().Name} is being paused."); + // A paused state could obviously be entered mid-import (during the `Task.WhenAll` below), + // but in order to keep things simple let's focus on the most common scenario. + notification.Text = $"{HumanisedModelName.Humanize(LetterCasing.Title)} import is paused due to gameplay..."; + notification.State = ProgressNotificationState.Queued; + while (PauseImports) { cancellationToken.ThrowIfCancellationRequested(); - Thread.Sleep(500); + await Task.Delay(500, cancellationToken).ConfigureAwait(false); } cancellationToken.ThrowIfCancellationRequested(); Logger.Log($@"{GetType().Name} is being resumed."); + + notification.Text = $"{HumanisedModelName.Humanize(LetterCasing.Title)} import is resuming..."; + notification.State = ProgressNotificationState.Active; } private IEnumerable getIDs(IEnumerable files) From 48212dfaebe9a5e941f3a1eac413b6ccfe488cf6 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 12 Nov 2024 21:08:06 +0900 Subject: [PATCH 3/5] Fix test failures due to early disposal of import stream --- .../Online/TestSceneReplayMissingBeatmap.cs | 20 ++++++++----------- 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/osu.Game.Tests/Visual/Online/TestSceneReplayMissingBeatmap.cs b/osu.Game.Tests/Visual/Online/TestSceneReplayMissingBeatmap.cs index 60197e0eb7fe..b986901dcf56 100644 --- a/osu.Game.Tests/Visual/Online/TestSceneReplayMissingBeatmap.cs +++ b/osu.Game.Tests/Visual/Online/TestSceneReplayMissingBeatmap.cs @@ -40,15 +40,13 @@ public void TestSceneMissingBeatmapWithOnlineAvailable() AddStep("import score", () => { - using (var resourceStream = TestResources.OpenResource("Replays/mania-replay.osr")) - { - var importTask = new ImportTask(resourceStream, "replay.osr"); + var resourceStream = TestResources.OpenResource("Replays/mania-replay.osr"); + var importTask = new ImportTask(resourceStream, "replay.osr"); - Game.ScoreManager.Import(new[] { importTask }); - } + Game.ScoreManager.Import(new[] { importTask }); }); - AddUntilStep("Replay missing notification show", () => Game.Notifications.ChildrenOfType().Any()); + AddUntilStep("Replay missing notification shown", () => Game.Notifications.ChildrenOfType().Any()); } [Test] @@ -58,15 +56,13 @@ public void TestSceneMissingBeatmapWithOnlineUnavailable() AddStep("import score", () => { - using (var resourceStream = TestResources.OpenResource("Replays/mania-replay.osr")) - { - var importTask = new ImportTask(resourceStream, "replay.osr"); + var resourceStream = TestResources.OpenResource("Replays/mania-replay.osr"); + var importTask = new ImportTask(resourceStream, "replay.osr"); - Game.ScoreManager.Import(new[] { importTask }); - } + Game.ScoreManager.Import(new[] { importTask }); }); - AddUntilStep("Replay missing notification not show", () => !Game.Notifications.ChildrenOfType().Any()); + AddUntilStep("Replay missing notification not shown", () => !Game.Notifications.ChildrenOfType().Any()); } private void setupBeatmapResponse(APIBeatmap b) From 2afd3579015f7b88c1aaf0aa5a8e42638729e41d Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 14 Nov 2024 13:14:35 +0900 Subject: [PATCH 4/5] Re-throw `OperationCanceledException` for consistency? Mostly to see if it breaks anything. --- osu.Game/Database/RealmArchiveModelImporter.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/osu.Game/Database/RealmArchiveModelImporter.cs b/osu.Game/Database/RealmArchiveModelImporter.cs index c4eb93b754d4..22bcc622e978 100644 --- a/osu.Game/Database/RealmArchiveModelImporter.cs +++ b/osu.Game/Database/RealmArchiveModelImporter.cs @@ -139,6 +139,7 @@ await Parallel.ForEachAsync(tasks, notification.CancellationToken, async (task, } catch (OperationCanceledException) { + throw; } catch (Exception e) { @@ -531,7 +532,8 @@ protected virtual void PostImport(TModel model, Realm realm, ImportParameters pa /// The new model proposed for import. /// The current realm context. /// An existing model which matches the criteria to skip importing, else null. - protected TModel? CheckForExisting(TModel model, Realm realm) => string.IsNullOrEmpty(model.Hash) ? null : realm.All().OrderBy(b => b.DeletePending).FirstOrDefault(b => b.Hash == model.Hash); + protected TModel? CheckForExisting(TModel model, Realm realm) => + string.IsNullOrEmpty(model.Hash) ? null : realm.All().OrderBy(b => b.DeletePending).FirstOrDefault(b => b.Hash == model.Hash); /// /// Whether import can be skipped after finding an existing import early in the process. From ebc2cc35700415a2b242c03f65880d7371278462 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 18 Nov 2024 16:51:34 +0900 Subject: [PATCH 5/5] Ensure cleanup tasks are run even on a cancelled / exceptioned import task --- osu.Game/Database/ModelDownloader.cs | 25 +++-- .../Database/RealmArchiveModelImporter.cs | 102 +++++++++--------- 2 files changed, 69 insertions(+), 58 deletions(-) diff --git a/osu.Game/Database/ModelDownloader.cs b/osu.Game/Database/ModelDownloader.cs index 8aece748a8e8..dfeec259fe5c 100644 --- a/osu.Game/Database/ModelDownloader.cs +++ b/osu.Game/Database/ModelDownloader.cs @@ -68,18 +68,23 @@ protected bool Download(T model, bool minimiseDownloadSize, TModel? originalMode { Task.Factory.StartNew(async () => { - bool importSuccessful; + bool importSuccessful = false; - if (originalModel != null) - importSuccessful = (await importer.ImportAsUpdate(notification, new ImportTask(filename), originalModel).ConfigureAwait(false)) != null; - else - importSuccessful = (await importer.Import(notification, new[] { new ImportTask(filename) }).ConfigureAwait(false)).Any(); - - // for now a failed import will be marked as a failed download for simplicity. - if (!importSuccessful) - DownloadFailed?.Invoke(request); + try + { + if (originalModel != null) + importSuccessful = (await importer.ImportAsUpdate(notification, new ImportTask(filename), originalModel).ConfigureAwait(false)) != null; + else + importSuccessful = (await importer.Import(notification, new[] { new ImportTask(filename) }).ConfigureAwait(false)).Any(); + } + finally + { + // for now a failed import will be marked as a failed download for simplicity. + if (!importSuccessful) + DownloadFailed?.Invoke(request); - CurrentDownloads.Remove(request); + CurrentDownloads.Remove(request); + } }, TaskCreationOptions.LongRunning); }; diff --git a/osu.Game/Database/RealmArchiveModelImporter.cs b/osu.Game/Database/RealmArchiveModelImporter.cs index 22bcc622e978..e538530b793f 100644 --- a/osu.Game/Database/RealmArchiveModelImporter.cs +++ b/osu.Game/Database/RealmArchiveModelImporter.cs @@ -117,67 +117,73 @@ public async Task>> Import(ProgressNotification notific await pauseIfNecessaryAsync(parameters, notification, notification.CancellationToken).ConfigureAwait(false); - await Parallel.ForEachAsync(tasks, notification.CancellationToken, async (task, cancellation) => + try { - cancellation.ThrowIfCancellationRequested(); - - try + await Parallel.ForEachAsync(tasks, notification.CancellationToken, async (task, cancellation) => { - await pauseIfNecessaryAsync(parameters, notification, cancellation).ConfigureAwait(false); + cancellation.ThrowIfCancellationRequested(); - var model = await Import(task, parameters, cancellation).ConfigureAwait(false); - - lock (imported) + try { - if (model != null) - imported.Add(model); - current++; + await pauseIfNecessaryAsync(parameters, notification, cancellation).ConfigureAwait(false); - notification.Text = $"Imported {current} of {tasks.Length} {HumanisedModelName}s"; - notification.Progress = (float)current / tasks.Length; - } - } - catch (OperationCanceledException) - { - throw; - } - catch (Exception e) - { - Logger.Error(e, $@"Could not import ({task})", LoggingTarget.Database); - } - }).ConfigureAwait(false); + var model = await Import(task, parameters, cancellation).ConfigureAwait(false); - if (imported.Count == 0) - { - if (notification.CancellationToken.IsCancellationRequested) - { - notification.State = ProgressNotificationState.Cancelled; - return imported; - } + lock (imported) + { + if (model != null) + imported.Add(model); + current++; - notification.Text = $"{HumanisedModelName.Humanize(LetterCasing.Title)} import failed! Check logs for more information."; - notification.State = ProgressNotificationState.Cancelled; + notification.Text = $"Imported {current} of {tasks.Length} {HumanisedModelName}s"; + notification.Progress = (float)current / tasks.Length; + } + } + catch (OperationCanceledException) + { + throw; + } + catch (Exception e) + { + Logger.Error(e, $@"Could not import ({task})", LoggingTarget.Database); + } + }).ConfigureAwait(false); } - else + finally { - if (tasks.Length > imported.Count) - notification.CompletionText = $"Imported {imported.Count} of {tasks.Length} {HumanisedModelName}s."; - else if (imported.Count > 1) - notification.CompletionText = $"Imported {imported.Count} {HumanisedModelName}s!"; - else - notification.CompletionText = $"Imported {imported.First().GetDisplayString()}!"; - - if (imported.Count > 0 && PresentImport != null) + if (imported.Count == 0) { - notification.CompletionText += " Click to view."; - notification.CompletionClickAction = () => + if (notification.CancellationToken.IsCancellationRequested) + { + notification.State = ProgressNotificationState.Cancelled; + } + else { - PresentImport?.Invoke(imported); - return true; - }; + notification.Text = $"{HumanisedModelName.Humanize(LetterCasing.Title)} import failed! Check logs for more information."; + notification.State = ProgressNotificationState.Cancelled; + } } + else + { + if (tasks.Length > imported.Count) + notification.CompletionText = $"Imported {imported.Count} of {tasks.Length} {HumanisedModelName}s."; + else if (imported.Count > 1) + notification.CompletionText = $"Imported {imported.Count} {HumanisedModelName}s!"; + else + notification.CompletionText = $"Imported {imported.First().GetDisplayString()}!"; + + if (imported.Count > 0 && PresentImport != null) + { + notification.CompletionText += " Click to view."; + notification.CompletionClickAction = () => + { + PresentImport?.Invoke(imported); + return true; + }; + } - notification.State = ProgressNotificationState.Completed; + notification.State = ProgressNotificationState.Completed; + } } return imported;