Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix several breakages with migration operation #30151

Merged
merged 5 commits into from
Oct 9, 2024

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Oct 8, 2024

Fix any and all migration attempts dying on MusicController

I'm not sure why this was "fine" for as long as it apparently was, but what MusicController was doing was completely incorrect and playing with fire (accessing raw managed realm objects), which went wrong somewhere around - admittedly - #29917, likely because that one started storing these raw managed realm objects to fields, and you know what will happen to those after you do a migration and recycle realms.

To attempt to circumvent this, (ab)use DetachedBeatmapStore instead. Which does necessitate moving it to OsuGameBase, but it's the simplest way out I can see. I guess the alternative would be to faff around with Live<T> but it's ugly and I'm attempting to fix this relatively quick right now.

Fix OsuGameBase.Migrate() eating exception messages for breakfast

Whomst've thought this was an ok thing to do? How did this pass review? Let's leave these as rhetorical questions right now. Huge chances are I'd implicate myself with at least one of them.

Fix DetachedBeatmapStore special condition for detecting resets from blocking all operations failing on empty databases

See https://discord.com/channels/188630481301012481/188630652340404224/1293309912591368244.

Fixes #30155

bdach added 3 commits October 8, 2024 23:16
I'm not sure why this was "fine" for as long as it apparently was,
but what `MusicController` was doing was completely incorrect and
playing with fire (accessing raw managed realm objects), which went
wrong somewhere around - admittedly -
ppy#29917, likely because that one started
*storing* these raw managed realm objects to fields, and you know what
will happen to those after you do a migration and recycle realms.

To attempt to circumvent this, (ab)use `DetachedBeatmapStore` instead.
Which does necessitate moving it to `OsuGameBase`, but it's the simplest
way out I can see. I guess the alternative would be to faff around with
`Live<T>` but it's ugly and I'm attempting to fix this relatively quick
right now.
Whomst've thought this was an ok thing to do? How did this pass review?
Let's leave these as rhetorical questions right now. Huge chances are
I'd implicate myself with at least one of them.
@bdach bdach added realm deals with local realm database type:reliability next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! labels Oct 8, 2024
@bdach bdach requested review from peppy and a team October 8, 2024 21:18
@ZeroAurora
Copy link

It feels so fun that two groups of people reproduce the same issue independently within half a day (in the context of #30155 and the discord link mentiond above).
Checked out this branch and verified that it works for me, at least for my issue.

@peppy
Copy link
Member

peppy commented Oct 9, 2024

I can get behind all the changes here.

As mentioned on discord, how about a rename to something like this?

diff --git a/osu.Game/Database/DetachedBeatmapStore.cs b/osu.Game/Database/DetachedBeatmapStore.cs
index f5c975d9db..5b65f608b2 100644
--- a/osu.Game/Database/DetachedBeatmapStore.cs
+++ b/osu.Game/Database/DetachedBeatmapStore.cs
@@ -44,7 +44,7 @@ private void beatmapSetsChanged(IRealmCollection<BeatmapSetInfo> sender, ChangeS
         {
             if (changes == null)
             {
-                if (sender is EmptyRealmSet<BeatmapSetInfo>)
+                if (sender is RealmResetEmptySet<BeatmapSetInfo>)
                 {
                     // Usually we'd reset stuff here, but doing so triggers a silly flow which ends up deadlocking realm.
                     // Additionally, user should not be at song select when realm is blocking all operations in the first place.
diff --git a/osu.Game/Database/RealmAccess.cs b/osu.Game/Database/RealmAccess.cs
index cb91d6923b..ad0423191d 100644
--- a/osu.Game/Database/RealmAccess.cs
+++ b/osu.Game/Database/RealmAccess.cs
@@ -568,7 +568,7 @@ public IDisposable RegisterForNotifications<T>(Func<Realm, IQueryable<T>> query,
             lock (notificationsResetMap)
             {
                 // Store an action which is used when blocking to ensure consumers don't use results of a stale changeset firing.
-                notificationsResetMap.Add(action, () => callback(new EmptyRealmSet<T>(), null));
+                notificationsResetMap.Add(action, () => callback(new RealmResetEmptySet<T>(), null));
             }
 
             return RegisterCustomSubscription(action);
diff --git a/osu.Game/Database/EmptyRealmSet.cs b/osu.Game/Database/RealmResetEmptySet.cs
similarity index 82%
rename from osu.Game/Database/EmptyRealmSet.cs
rename to osu.Game/Database/RealmResetEmptySet.cs
index c34974cb03..9f9a1ba6d7 100644
--- a/osu.Game/Database/EmptyRealmSet.cs
+++ b/osu.Game/Database/RealmResetEmptySet.cs
@@ -12,7 +12,13 @@
 
 namespace osu.Game.Database
 {
-    public class EmptyRealmSet<T> : IRealmCollection<T>
+    /// <summary>
+    /// This can arrive in <see cref="RealmAccess.RegisterForNotifications{T}"/> callbacks to imply that realm access has been reset.
+    /// </summary>
+    /// <remarks>
+    /// Usually implies that the original database may return soon and the callback can usually be silently ignored.
+    ///</remarks>
+    public class RealmResetEmptySet<T> : IRealmCollection<T>
     {
         private IList<T> emptySet => Array.Empty<T>();
 

Copy link
Member

@peppy peppy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As proposed

@peppy peppy merged commit 270c4c4 into ppy:master Oct 9, 2024
5 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! realm deals with local realm database size/M type:reliability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migration will not work when the installation is empty (i.e. no beatmaps in game) due to realm block failure
3 participants