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 crash on accessing settings after moving game folder #16077

Merged
merged 7 commits into from Dec 16, 2021

Conversation

peppy
Copy link
Sponsor Member

@peppy peppy commented Dec 14, 2021

Bit of an unfortunate one, but in theory the usage of ToLive should be quite isolated. If anything, i can see us reducing the number of places it is used from the current usages.

Closes #16074

Also changes the realm filename to use `client` to match the ignore
rules in `OsuStorage`. Without doing this, migration will fail in an
indefinite mutex wait when attempting to delete the realm `.note` file.
@bdach
Copy link
Collaborator

bdach commented Dec 14, 2021

CI looks quite red here...

@bdach bdach added the realm deals with local realm database label Dec 14, 2021
osu.Game/Database/RealmLive.cs Show resolved Hide resolved
@@ -255,10 +255,10 @@ private void load()
if (skinInfo == null)
{
if (guid == SkinInfo.CLASSIC_SKIN)
skinInfo = DefaultLegacySkin.CreateInfo().ToLive();
skinInfo = DefaultLegacySkin.CreateInfo().ToLiveUnmanaged();
Copy link
Collaborator

Choose a reason for hiding this comment

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

To expand on the the previous point, I'm having a difficult time reviewing these changes because I'm not super sure where it is fine to use .ToLiveUnmanaged() and where it is fine to use .ToLive(). Looks like the only way to validate this would be to check all references and make sure there are no writes to this skinInfo, which is prone to error, and each missed usage is a runtime crash. The compiler could do that for me, but only if IReadOnlyLive<T> existed.

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

It's definitely a bit of an uncertain one. In these specific cases, the instances are only used for ID based comparison, where the final requery is done on changing skins, but I can understand how this doesn't read great.

diff --git a/osu.Game/OsuGame.cs b/osu.Game/OsuGame.cs
index 9c379de683..a56b25d6c2 100644
--- a/osu.Game/OsuGame.cs
+++ b/osu.Game/OsuGame.cs
@@ -255,10 +255,10 @@ private void load()
                 if (skinInfo == null)
                 {
                     if (guid == SkinInfo.CLASSIC_SKIN)
-                        skinInfo = DefaultLegacySkin.CreateInfo().ToLiveUnmanaged();
+                        skinInfo = DefaultLegacySkin.CreateInfo().ToLive(RealmFactory);
                 }
 
-                SkinManager.CurrentSkinInfo.Value = skinInfo ?? DefaultSkin.CreateInfo().ToLiveUnmanaged();
+                SkinManager.CurrentSkinInfo.Value = skinInfo ?? DefaultSkin.CreateInfo().ToLive(RealmFactory);
             };
             configSkin.TriggerChange();
 
diff --git a/osu.Game/OsuGameBase.cs b/osu.Game/OsuGameBase.cs
index 2e266e32ff..e1ba4ccabf 100644
--- a/osu.Game/OsuGameBase.cs
+++ b/osu.Game/OsuGameBase.cs
@@ -154,7 +154,7 @@ public virtual string Version
 
         private DatabaseContextFactory contextFactory;
 
-        private RealmContextFactory realmFactory;
+        protected RealmContextFactory RealmFactory { get; private set; }
 
         protected override Container<Drawable> Content => content;
 
@@ -198,9 +198,9 @@ private void load(ReadableKeyCombinationProvider keyCombinationProvider)
             dependencies.Cache(RulesetStore = new RulesetStore(contextFactory, Storage));
             dependencies.CacheAs<IRulesetStore>(RulesetStore);
 
-            dependencies.Cache(realmFactory = new RealmContextFactory(Storage, "client", contextFactory));
+            dependencies.Cache(RealmFactory = new RealmContextFactory(Storage, "client", contextFactory));
 
-            new EFToRealmMigrator(contextFactory, realmFactory, LocalConfig).Run();
+            new EFToRealmMigrator(contextFactory, RealmFactory, LocalConfig).Run();
 
             dependencies.CacheAs(Storage);
 
@@ -215,7 +215,7 @@ private void load(ReadableKeyCombinationProvider keyCombinationProvider)
 
             Audio.Samples.PlaybackConcurrency = SAMPLE_CONCURRENCY;
 
-            dependencies.Cache(SkinManager = new SkinManager(Storage, realmFactory, Host, Resources, Audio, Scheduler));
+            dependencies.Cache(SkinManager = new SkinManager(Storage, RealmFactory, Host, Resources, Audio, Scheduler));
             dependencies.CacheAs<ISkinSource>(SkinManager);
 
             EndpointConfiguration endpoints = UseDevelopmentServer ? (EndpointConfiguration)new DevelopmentEndpointConfiguration() : new ProductionEndpointConfiguration();
@@ -239,7 +239,7 @@ private void load(ReadableKeyCombinationProvider keyCombinationProvider)
             dependencies.Cache(ScoreDownloader = new ScoreModelDownloader(ScoreManager, API));
 
             // the following realm components are not actively used yet, but initialised and kept up to date for initial testing.
-            realmRulesetStore = new RealmRulesetStore(realmFactory, Storage);
+            realmRulesetStore = new RealmRulesetStore(RealmFactory, Storage);
 
             dependencies.Cache(realmRulesetStore);
 
@@ -268,7 +268,7 @@ List<ScoreInfo> getBeatmapScores(BeatmapSetInfo set)
             dependencies.Cache(scorePerformanceManager);
             AddInternal(scorePerformanceManager);
 
-            dependencies.Cache(rulesetConfigCache = new RulesetConfigCache(realmFactory, RulesetStore));
+            dependencies.Cache(rulesetConfigCache = new RulesetConfigCache(RealmFactory, RulesetStore));
 
             var powerStatus = CreateBatteryInfo();
             if (powerStatus != null)
@@ -314,7 +314,7 @@ List<ScoreInfo> getBeatmapScores(BeatmapSetInfo set)
 
             base.Content.Add(CreateScalingContainer().WithChildren(mainContent));
 
-            KeyBindingStore = new RealmKeyBindingStore(realmFactory, keyCombinationProvider);
+            KeyBindingStore = new RealmKeyBindingStore(RealmFactory, keyCombinationProvider);
             KeyBindingStore.Register(globalBindings, RulesetStore.AvailableRulesets);
 
             dependencies.Cache(globalBindings);
@@ -380,7 +380,7 @@ protected override void Update()
         {
             base.Update();
 
-            realmFactory.Refresh();
+            RealmFactory.Refresh();
         }
 
         protected override IReadOnlyDependencyContainer CreateChildDependencies(IReadOnlyDependencyContainer parent) =>
@@ -422,7 +422,7 @@ public void Migrate(string path)
 
                 Scheduler.Add(() =>
                 {
-                    realmBlocker = realmFactory.BlockAllOperations();
+                    realmBlocker = RealmFactory.BlockAllOperations();
                     contextFactory.FlushConnections();
 
                     readyToRun.Set();
@@ -502,7 +502,7 @@ protected override void Dispose(bool isDisposing)
             contextFactory?.FlushConnections();
 
             realmRulesetStore?.Dispose();
-            realmFactory?.Dispose();
+            RealmFactory?.Dispose();
         }
     }
 }

OsuGame can be amicably fixed, but the other two cases require a bit more refactoring which potentially makes the class uglier than before. Not sure if it's worth making the change (ie. removing ToLiveUnmanaged usage) just yet, or wait until there's a reason to do so.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, the fact that these usages are only used for equality checks assuages my doubts somewhat. I suppose let's roll with this and see if it backfires, and revisit if it ever does.

osu.Game/Database/RealmLive.cs Outdated Show resolved Hide resolved
Copy link
Contributor

@smoogipoo smoogipoo left a comment

Choose a reason for hiding this comment

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

I think this is fine?

@smoogipoo smoogipoo requested a review from bdach December 16, 2021 07:52
@bdach bdach enabled auto-merge December 16, 2021 23:19
@bdach bdach merged commit 187cd80 into ppy:master Dec 16, 2021
@peppy peppy deleted the fix-realm-post-storage-migration-failure branch December 17, 2021 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
realm deals with local realm database size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash when accessing settings after moving osu! data location
3 participants