From c9a203cc11f12532054493629617e21daaba4b21 Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Wed, 23 Jun 2021 15:03:34 +0900 Subject: [PATCH 1/5] Move collections db in-place --- osu.Game/Collections/CollectionManager.cs | 41 ++++++++++++++++------- 1 file changed, 28 insertions(+), 13 deletions(-) diff --git a/osu.Game/Collections/CollectionManager.cs b/osu.Game/Collections/CollectionManager.cs index b53cc659f7..b2f36c75f4 100644 --- a/osu.Game/Collections/CollectionManager.cs +++ b/osu.Game/Collections/CollectionManager.cs @@ -257,27 +257,42 @@ namespace osu.Game.Collections { Interlocked.Increment(ref lastSave); + // This is NOT thread-safe!! try { - // This is NOT thread-safe!! + var tempPath = Path.GetTempFileName(); - using (var sw = new SerializationWriter(storage.GetStream(database_name, FileAccess.Write))) + using (var ms = new MemoryStream()) { - sw.Write(database_version); - - var collectionsCopy = Collections.ToArray(); - sw.Write(collectionsCopy.Length); - - foreach (var c in collectionsCopy) + using (var sw = new SerializationWriter(ms, true)) { - sw.Write(c.Name.Value); + sw.Write(database_version); - var beatmapsCopy = c.Beatmaps.ToArray(); - sw.Write(beatmapsCopy.Length); + var collectionsCopy = Collections.ToArray(); + sw.Write(collectionsCopy.Length); - foreach (var b in beatmapsCopy) - sw.Write(b.MD5Hash); + foreach (var c in collectionsCopy) + { + sw.Write(c.Name.Value); + + var beatmapsCopy = c.Beatmaps.ToArray(); + sw.Write(beatmapsCopy.Length); + + foreach (var b in beatmapsCopy) + sw.Write(b.MD5Hash); + } } + + using (var fs = File.OpenWrite(tempPath)) + ms.WriteTo(fs); + + var storagePath = storage.GetFullPath(database_name); + var storageBackupPath = storage.GetFullPath($"{database_name}.bak"); + if (File.Exists(storageBackupPath)) + File.Delete(storageBackupPath); + if (File.Exists(storagePath)) + File.Move(storagePath, storageBackupPath); + File.Move(tempPath, storagePath); } if (saveFailures < 10) From 5828606c1332152b71c942ae2b06d7d98dc445ad Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Wed, 23 Jun 2021 15:09:42 +0900 Subject: [PATCH 2/5] Prefer using the backup file if it exists --- osu.Game/Collections/CollectionManager.cs | 30 ++++++++++++++++------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/osu.Game/Collections/CollectionManager.cs b/osu.Game/Collections/CollectionManager.cs index b2f36c75f4..04ea7eaebf 100644 --- a/osu.Game/Collections/CollectionManager.cs +++ b/osu.Game/Collections/CollectionManager.cs @@ -35,6 +35,7 @@ namespace osu.Game.Collections private const int database_version = 30000000; private const string database_name = "collection.db"; + private const string database_backup_name = "collection.db.bak"; public readonly BindableList Collections = new BindableList(); @@ -56,11 +57,14 @@ namespace osu.Game.Collections { Collections.CollectionChanged += collectionsChanged; - if (storage.Exists(database_name)) + // If a backup file exists, it means the previous write operation didn't complete successfully. Always prefer the backup file in such a case. + string filename = storage.Exists(database_backup_name) ? database_backup_name : database_name; + + if (storage.Exists(filename)) { List beatmapCollections; - using (var stream = storage.GetStream(database_name)) + using (var stream = storage.GetStream(filename)) beatmapCollections = readCollections(stream); // intentionally fire-and-forget async. @@ -286,13 +290,21 @@ namespace osu.Game.Collections using (var fs = File.OpenWrite(tempPath)) ms.WriteTo(fs); - var storagePath = storage.GetFullPath(database_name); - var storageBackupPath = storage.GetFullPath($"{database_name}.bak"); - if (File.Exists(storageBackupPath)) - File.Delete(storageBackupPath); - if (File.Exists(storagePath)) - File.Move(storagePath, storageBackupPath); - File.Move(tempPath, storagePath); + var databasePath = storage.GetFullPath(database_name); + var databaseBackupPath = storage.GetFullPath(database_backup_name); + + // Back up the existing database, clearing any existing backup. + if (File.Exists(databaseBackupPath)) + File.Delete(databaseBackupPath); + if (File.Exists(databasePath)) + File.Move(databasePath, databaseBackupPath); + + // Move the new database in-place of the existing one. + File.Move(tempPath, databasePath); + + // If everything succeeded up to this point, remove the backup file. + if (File.Exists(databaseBackupPath)) + File.Delete(databaseBackupPath); } if (saveFailures < 10) From 0510282dcbe1a3ad581d01ed57d1bfcfea431c18 Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Wed, 23 Jun 2021 15:10:03 +0900 Subject: [PATCH 3/5] Add missing ctor --- osu.Game/IO/Legacy/SerializationWriter.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/osu.Game/IO/Legacy/SerializationWriter.cs b/osu.Game/IO/Legacy/SerializationWriter.cs index bb8014fe54..9ebeaf616e 100644 --- a/osu.Game/IO/Legacy/SerializationWriter.cs +++ b/osu.Game/IO/Legacy/SerializationWriter.cs @@ -18,8 +18,8 @@ namespace osu.Game.IO.Legacy /// handle null strings and simplify use with ISerializable. public class SerializationWriter : BinaryWriter { - public SerializationWriter(Stream s) - : base(s, Encoding.UTF8) + public SerializationWriter(Stream s, bool leaveOpen = false) + : base(s, Encoding.UTF8, leaveOpen) { } From e7981eae9bbca5e40afbe9103628cfe17f9af9ba Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Wed, 23 Jun 2021 16:14:05 +0900 Subject: [PATCH 4/5] Safeguard load procedure a bit more --- osu.Game/Collections/CollectionManager.cs | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/osu.Game/Collections/CollectionManager.cs b/osu.Game/Collections/CollectionManager.cs index 04ea7eaebf..e6e8bedcf4 100644 --- a/osu.Game/Collections/CollectionManager.cs +++ b/osu.Game/Collections/CollectionManager.cs @@ -55,16 +55,26 @@ namespace osu.Game.Collections [BackgroundDependencyLoader] private void load() { + if (storage.Exists(database_backup_name)) + { + // If a backup file exists, it means the previous write operation didn't run to completion. + // Always prefer the backup file in such a case as it's the most recent copy that is guaranteed to not be malformed. + // + // The database is saved 100ms after any change, and again when the game is closed, so there shouldn't be a large diff between the two files in the worst case. + if (storage.Exists(database_name)) + storage.Delete(database_name); + File.Copy(storage.GetFullPath(database_backup_name), storage.GetFullPath(database_name)); + } + + // Only accept changes after/if the above succeeds to prevent simultaneously accessing either file. + // Todo: This really should not be delayed like this, but is unlikely to cause issues because CollectionManager loads very early in execution. Collections.CollectionChanged += collectionsChanged; - // If a backup file exists, it means the previous write operation didn't complete successfully. Always prefer the backup file in such a case. - string filename = storage.Exists(database_backup_name) ? database_backup_name : database_name; - - if (storage.Exists(filename)) + if (storage.Exists(database_name)) { List beatmapCollections; - using (var stream = storage.GetStream(filename)) + using (var stream = storage.GetStream(database_name)) beatmapCollections = readCollections(stream); // intentionally fire-and-forget async. From 8a2026e3053c9c8bbc7d46c25029fa3d7f7f0506 Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Wed, 23 Jun 2021 21:26:52 +0900 Subject: [PATCH 5/5] Schedule callback instead --- osu.Game/Collections/CollectionManager.cs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/osu.Game/Collections/CollectionManager.cs b/osu.Game/Collections/CollectionManager.cs index e6e8bedcf4..fe04c70d62 100644 --- a/osu.Game/Collections/CollectionManager.cs +++ b/osu.Game/Collections/CollectionManager.cs @@ -55,6 +55,8 @@ namespace osu.Game.Collections [BackgroundDependencyLoader] private void load() { + Collections.CollectionChanged += collectionsChanged; + if (storage.Exists(database_backup_name)) { // If a backup file exists, it means the previous write operation didn't run to completion. @@ -66,10 +68,6 @@ namespace osu.Game.Collections File.Copy(storage.GetFullPath(database_backup_name), storage.GetFullPath(database_name)); } - // Only accept changes after/if the above succeeds to prevent simultaneously accessing either file. - // Todo: This really should not be delayed like this, but is unlikely to cause issues because CollectionManager loads very early in execution. - Collections.CollectionChanged += collectionsChanged; - if (storage.Exists(database_name)) { List beatmapCollections; @@ -82,7 +80,7 @@ namespace osu.Game.Collections } } - private void collectionsChanged(object sender, NotifyCollectionChangedEventArgs e) + private void collectionsChanged(object sender, NotifyCollectionChangedEventArgs e) => Schedule(() => { switch (e.Action) { @@ -106,7 +104,7 @@ namespace osu.Game.Collections } backgroundSave(); - } + }); /// /// Set an endpoint for notifications to be posted to.