From 502844a8583e6ee8537f0479d71f2e6a8ab62178 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 16 Aug 2023 14:23:01 +0900 Subject: [PATCH 1/8] Add ability to construct `RealmLive` from `ID` --- osu.Game/Database/RealmLive.cs | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/osu.Game/Database/RealmLive.cs b/osu.Game/Database/RealmLive.cs index 509fabec59..1134636756 100644 --- a/osu.Game/Database/RealmLive.cs +++ b/osu.Game/Database/RealmLive.cs @@ -30,7 +30,7 @@ namespace osu.Game.Database /// /// Construct a new instance of live realm data. /// - /// The realm data. + /// The realm data. Must be managed (see ). /// The realm factory the data was sourced from. May be null for an unmanaged object. public RealmLive(T data, RealmAccess realm) : base(data.ID) @@ -41,6 +41,24 @@ namespace osu.Game.Database dataIsFromUpdateThread = ThreadSafety.IsUpdateThread; } + /// + /// Construct a new instance of live realm data from an ID. + /// + /// The ID of an already-persisting realm instance. + /// The realm factory the data was sourced from. May be null for an unmanaged object. + public RealmLive(Guid id, RealmAccess realm) + : base(id) + { + data = retrieveFromID(realm.Realm); + + if (data.IsNull()) + throw new ArgumentException("Realm instance for provided ID could not be found.", nameof(id)); + + this.realm = realm; + + dataIsFromUpdateThread = ThreadSafety.IsUpdateThread; + } + /// /// Perform a read operation on this live object. /// From 5bd7370439c1e79a739be0d46dce060f57444ffe Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 16 Aug 2023 14:23:17 +0900 Subject: [PATCH 2/8] Add log output when editor is creating a fresh beatmap --- osu.Game/Screens/Edit/Editor.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/osu.Game/Screens/Edit/Editor.cs b/osu.Game/Screens/Edit/Editor.cs index 35d8bb4ab7..1cdca5754d 100644 --- a/osu.Game/Screens/Edit/Editor.cs +++ b/osu.Game/Screens/Edit/Editor.cs @@ -199,6 +199,8 @@ namespace osu.Game.Screens.Edit if (loadableBeatmap is DummyWorkingBeatmap) { + Logger.Log("Editor was loaded without a valid beatmap; creating a new beatmap."); + isNewBeatmap = true; loadableBeatmap = beatmapManager.CreateNew(Ruleset.Value, api.LocalUser.Value); From 531794b26b6af72f3f25d609694e135fc960cf64 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 16 Aug 2023 14:23:30 +0900 Subject: [PATCH 3/8] Fix `ModelManager` not correctly re-retrieving realm objects before performing operations Falls into the age-old trap of attempting to retrieve an item from realm without first ensuring that realm is in an up-to-date state. Consider this scenario: - Editor is entered from main menu, causing it to create a new beatmap from its async `load()` method. - Editor opens correctly, then main thread performs a file operations on the same beatmap. - Main thread is potentially not refreshed yet, and will result in `null` instance when performing the re-fetch in `performFileOperation`. I've fixed this by using the safe implementation inside `RealmLive`. Feels like we want this is one place which is always used as the correct method. On a quick search, there are 10-20 other usages of `Realm.Find` which could also have similar issues, but it'll be a bit of a pain to go through and fix each of these. In 99.9% of cases, the accesses are on instances which couldn't have just been created (or the usage of recently-imported/created is blocked by realm subscription flows, ie. baetmap import) so I'm not touching them for now. Something to keep in mind when working with realm going forward though. --- osu.Game/Database/ModelManager.cs | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/osu.Game/Database/ModelManager.cs b/osu.Game/Database/ModelManager.cs index 47feb8a8f9..6b538e62ea 100644 --- a/osu.Game/Database/ModelManager.cs +++ b/osu.Game/Database/ModelManager.cs @@ -48,16 +48,13 @@ namespace osu.Game.Database // This method should be removed as soon as all the surrounding pieces support non-detached operations. if (!item.IsManaged) { - // Importantly, begin the realm write *before* re-fetching, else the update realm may not be in a consistent state - // (ie. if an async import finished very recently). - Realm.Realm.Write(realm => + // We use RealmLive here as it handled re-retrieval and refreshing of realm if required. + new RealmLive(item.ID, Realm).PerformWrite(i => { - var managed = realm.Find(item.ID); - Debug.Assert(managed != null); - operation(managed); + operation(i); item.Files.Clear(); - item.Files.AddRange(managed.Files.Detach()); + item.Files.AddRange(i.Files.Detach()); }); } else From a8824c8c8ad4b6d8184f68a34ebc2672fb299716 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 16 Aug 2023 14:30:10 +0900 Subject: [PATCH 4/8] Remove flaky test documentation for fixed test --- .../Editing/TestSceneEditorBeatmapCreation.cs | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/osu.Game.Tests/Visual/Editing/TestSceneEditorBeatmapCreation.cs b/osu.Game.Tests/Visual/Editing/TestSceneEditorBeatmapCreation.cs index 37cb43a43a..920e560018 100644 --- a/osu.Game.Tests/Visual/Editing/TestSceneEditorBeatmapCreation.cs +++ b/osu.Game.Tests/Visual/Editing/TestSceneEditorBeatmapCreation.cs @@ -92,25 +92,6 @@ namespace osu.Game.Tests.Visual.Editing } [Test] - [FlakyTest] - /* - * Fail rate around 1.2%. - * - * Failing with realm refetch occasionally being null. - * My only guess is that the WorkingBeatmap at SetupScreen is dummy instead of the true one. - * If it's something else, we have larger issues with realm, but I don't think that's the case. - * - * at osu.Framework.Logging.ThrowingTraceListener.Fail(String message1, String message2) - * at System.Diagnostics.TraceInternal.Fail(String message, String detailMessage) - * at System.Diagnostics.TraceInternal.TraceProvider.Fail(String message, String detailMessage) - * at System.Diagnostics.Debug.Fail(String message, String detailMessage) - * at osu.Game.Database.ModelManager`1.<>c__DisplayClass8_0.b__0(Realm realm) ModelManager.cs:line 50 - * at osu.Game.Database.RealmExtensions.Write(Realm realm, Action`1 function) RealmExtensions.cs:line 14 - * at osu.Game.Database.ModelManager`1.performFileOperation(TModel item, Action`1 operation) ModelManager.cs:line 47 - * at osu.Game.Database.ModelManager`1.AddFile(TModel item, Stream contents, String filename) ModelManager.cs:line 37 - * at osu.Game.Screens.Edit.Setup.ResourcesSection.ChangeAudioTrack(FileInfo source) ResourcesSection.cs:line 115 - * at osu.Game.Tests.Visual.Editing.TestSceneEditorBeatmapCreation.b__11_0() TestSceneEditorBeatmapCreation.cs:line 101 - */ public void TestAddAudioTrack() { AddAssert("track is virtual", () => Beatmap.Value.Track is TrackVirtual); From 6c4c76350fbecd1032709e2724b9abd9ee979e7f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 16 Aug 2023 07:36:56 +0200 Subject: [PATCH 5/8] Remove unused using directive --- osu.Game/Database/ModelManager.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/osu.Game/Database/ModelManager.cs b/osu.Game/Database/ModelManager.cs index 6b538e62ea..56aa0843a0 100644 --- a/osu.Game/Database/ModelManager.cs +++ b/osu.Game/Database/ModelManager.cs @@ -3,7 +3,6 @@ using System; using System.Collections.Generic; -using System.Diagnostics; using System.IO; using System.Linq; using osu.Framework.Platform; From 88295a49aa4655fcf48c317c1efac5a19dbc7342 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 16 Aug 2023 07:38:31 +0200 Subject: [PATCH 6/8] Fix invalid reference in xmldoc --- osu.Game/Database/RealmLive.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Database/RealmLive.cs b/osu.Game/Database/RealmLive.cs index 1134636756..bfb755c42a 100644 --- a/osu.Game/Database/RealmLive.cs +++ b/osu.Game/Database/RealmLive.cs @@ -30,7 +30,7 @@ namespace osu.Game.Database /// /// Construct a new instance of live realm data. /// - /// The realm data. Must be managed (see ). + /// The realm data. Must be managed (see ). /// The realm factory the data was sourced from. May be null for an unmanaged object. public RealmLive(T data, RealmAccess realm) : base(data.ID) From 6e11162ab10734b86b88f13b4967ae0bf68b6abd Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 16 Aug 2023 15:36:31 +0900 Subject: [PATCH 7/8] Add helper method for safer realm `Find` --- osu.Game/Database/ModelManager.cs | 12 +++++--- osu.Game/Database/RealmExtensions.cs | 25 +++++++++++++++++ osu.Game/Database/RealmLive.cs | 41 +++------------------------- 3 files changed, 37 insertions(+), 41 deletions(-) diff --git a/osu.Game/Database/ModelManager.cs b/osu.Game/Database/ModelManager.cs index 56aa0843a0..39dae61d36 100644 --- a/osu.Game/Database/ModelManager.cs +++ b/osu.Game/Database/ModelManager.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using System.Diagnostics; using System.IO; using System.Linq; using osu.Framework.Platform; @@ -47,13 +48,16 @@ namespace osu.Game.Database // This method should be removed as soon as all the surrounding pieces support non-detached operations. if (!item.IsManaged) { - // We use RealmLive here as it handled re-retrieval and refreshing of realm if required. - new RealmLive(item.ID, Realm).PerformWrite(i => + // Importantly, begin the realm write *before* re-fetching, else the update realm may not be in a consistent state + // (ie. if an async import finished very recently). + Realm.Realm.Write(realm => { - operation(i); + var managed = realm.FindWithRefresh(item.ID); + Debug.Assert(managed != null); + operation(managed); item.Files.Clear(); - item.Files.AddRange(i.Files.Detach()); + item.Files.AddRange(managed.Files.Detach()); }); } else diff --git a/osu.Game/Database/RealmExtensions.cs b/osu.Game/Database/RealmExtensions.cs index 13c4defb83..ee76f1aa79 100644 --- a/osu.Game/Database/RealmExtensions.cs +++ b/osu.Game/Database/RealmExtensions.cs @@ -8,6 +8,31 @@ namespace osu.Game.Database { public static class RealmExtensions { + /// + /// Performs a . + /// If a match was not found, a is performed before trying a second time. + /// This ensures that an instance is found even if the realm requested against was not in a consistent state. + /// + /// + /// + /// + /// + public static T? FindWithRefresh(this Realm realm, Guid id) where T : IRealmObject + { + var found = realm.Find(id); + + if (found == null) + { + // It may be that we access this from the update thread before a refresh has taken place. + // To ensure that behaviour matches what we'd expect (the object *is* available), force + // a refresh to bring in any off-thread changes immediately. + realm.Refresh(); + found = realm.Find(id); + } + + return found; + } + /// /// Perform a write operation against the provided realm instance. /// diff --git a/osu.Game/Database/RealmLive.cs b/osu.Game/Database/RealmLive.cs index bfb755c42a..9e99cba45c 100644 --- a/osu.Game/Database/RealmLive.cs +++ b/osu.Game/Database/RealmLive.cs @@ -41,24 +41,6 @@ namespace osu.Game.Database dataIsFromUpdateThread = ThreadSafety.IsUpdateThread; } - /// - /// Construct a new instance of live realm data from an ID. - /// - /// The ID of an already-persisting realm instance. - /// The realm factory the data was sourced from. May be null for an unmanaged object. - public RealmLive(Guid id, RealmAccess realm) - : base(id) - { - data = retrieveFromID(realm.Realm); - - if (data.IsNull()) - throw new ArgumentException("Realm instance for provided ID could not be found.", nameof(id)); - - this.realm = realm; - - dataIsFromUpdateThread = ThreadSafety.IsUpdateThread; - } - /// /// Perform a read operation on this live object. /// @@ -80,7 +62,7 @@ namespace osu.Game.Database return; } - perform(retrieveFromID(r)); + perform(r.FindWithRefresh(ID)!); RealmLiveStatistics.USAGE_ASYNC.Value++; }); } @@ -102,7 +84,7 @@ namespace osu.Game.Database return realm.Run(r => { - var returnData = perform(retrieveFromID(r)); + var returnData = perform(r.FindWithRefresh(ID)!); RealmLiveStatistics.USAGE_ASYNC.Value++; if (returnData is RealmObjectBase realmObject && realmObject.IsManaged) @@ -159,25 +141,10 @@ namespace osu.Game.Database } dataIsFromUpdateThread = true; - data = retrieveFromID(realm.Realm); + data = realm.Realm.FindWithRefresh(ID)!; + RealmLiveStatistics.USAGE_UPDATE_REFETCH.Value++; } - - private T retrieveFromID(Realm realm) - { - var found = realm.Find(ID); - - if (found == null) - { - // It may be that we access this from the update thread before a refresh has taken place. - // To ensure that behaviour matches what we'd expect (the object *is* available), force - // a refresh to bring in any off-thread changes immediately. - realm.Refresh(); - found = realm.Find(ID)!; - } - - return found; - } } internal static class RealmLiveStatistics From d70a9a5bc414c538ba67340c9a21d0666bd932d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 16 Aug 2023 09:40:46 +0200 Subject: [PATCH 8/8] Fill out xmldoc and adjust inline commentary --- osu.Game/Database/RealmExtensions.cs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/osu.Game/Database/RealmExtensions.cs b/osu.Game/Database/RealmExtensions.cs index ee76f1aa79..c84e1e35b8 100644 --- a/osu.Game/Database/RealmExtensions.cs +++ b/osu.Game/Database/RealmExtensions.cs @@ -13,10 +13,13 @@ namespace osu.Game.Database /// If a match was not found, a is performed before trying a second time. /// This ensures that an instance is found even if the realm requested against was not in a consistent state. /// - /// - /// - /// - /// + /// The realm to operate on. + /// The ID of the entity to find in the realm. + /// The type of the entity to find in the realm. + /// + /// The retrieved entity of type . + /// Can be if the entity is still not found by even after a refresh. + /// public static T? FindWithRefresh(this Realm realm, Guid id) where T : IRealmObject { var found = realm.Find(id); @@ -24,7 +27,7 @@ namespace osu.Game.Database if (found == null) { // It may be that we access this from the update thread before a refresh has taken place. - // To ensure that behaviour matches what we'd expect (the object *is* available), force + // To ensure that behaviour matches what we'd expect (the object generally *should be* available), force // a refresh to bring in any off-thread changes immediately. realm.Refresh(); found = realm.Find(id);