Merge pull request #13767 from peppy/fix-realm-refresh-race

Fix thread safety of realm `Refresh` operation

osu.Game.Tests/Database/TestRealmKeyBindingStore.cs

@@ -38,19 +38,28 @@ namespace osu.Game.Tests.Database
         [Test]
         public void TestDefaultsPopulationAndQuery()
         {
-            Assert.That(query().Count, Is.EqualTo(0));
+            Assert.That(queryCount(), Is.EqualTo(0));
 
             KeyBindingContainer testContainer = new TestKeyBindingContainer();
 
             keyBindingStore.Register(testContainer);
 
-            Assert.That(query().Count, Is.EqualTo(3));
+            Assert.That(queryCount(), Is.EqualTo(3));
 
-            Assert.That(query().Where(k => k.ActionInt == (int)GlobalAction.Back).Count, Is.EqualTo(1));
+            Assert.That(queryCount(GlobalAction.Back), Is.EqualTo(1));
-            Assert.That(query().Where(k => k.ActionInt == (int)GlobalAction.Select).Count, Is.EqualTo(2));
+            Assert.That(queryCount(GlobalAction.Select), Is.EqualTo(2));
         }
 
-        private IQueryable<RealmKeyBinding> query() => realmContextFactory.Context.All<RealmKeyBinding>();
+        private int queryCount(GlobalAction? match = null)
+        {
+            using (var usage = realmContextFactory.GetForRead())
+            {
+                var results = usage.Realm.All<RealmKeyBinding>();
+                if (match.HasValue)
+                    results = results.Where(k => k.ActionInt == (int)match.Value);
+                return results.Count();
+            }
+        }
 
         [Test]
         public void TestUpdateViaQueriedReference()
@@ -59,25 +68,28 @@ namespace osu.Game.Tests.Database
 
             keyBindingStore.Register(testContainer);
 
-            var backBinding = query().Single(k => k.ActionInt == (int)GlobalAction.Back);
+            using (var primaryUsage = realmContextFactory.GetForRead())
-
-            Assert.That(backBinding.KeyCombination.Keys, Is.EquivalentTo(new[] { InputKey.Escape }));
-
-            var tsr = ThreadSafeReference.Create(backBinding);
-
-            using (var usage = realmContextFactory.GetForWrite())
             {
-                var binding = usage.Realm.ResolveReference(tsr);
+                var backBinding = primaryUsage.Realm.All<RealmKeyBinding>().Single(k => k.ActionInt == (int)GlobalAction.Back);
-                binding.KeyCombination = new KeyCombination(InputKey.BackSpace);
 
-                usage.Commit();
+                Assert.That(backBinding.KeyCombination.Keys, Is.EquivalentTo(new[] { InputKey.Escape }));
+
+                var tsr = ThreadSafeReference.Create(backBinding);
+
+                using (var usage = realmContextFactory.GetForWrite())
+                {
+                    var binding = usage.Realm.ResolveReference(tsr);
+                    binding.KeyCombination = new KeyCombination(InputKey.BackSpace);
+
+                    usage.Commit();
+                }
+
+                Assert.That(backBinding.KeyCombination.Keys, Is.EquivalentTo(new[] { InputKey.BackSpace }));
+
+                // check still correct after re-query.
+                backBinding = primaryUsage.Realm.All<RealmKeyBinding>().Single(k => k.ActionInt == (int)GlobalAction.Back);
+                Assert.That(backBinding.KeyCombination.Keys, Is.EquivalentTo(new[] { InputKey.BackSpace }));
             }
-
-            Assert.That(backBinding.KeyCombination.Keys, Is.EquivalentTo(new[] { InputKey.BackSpace }));
-
-            // check still correct after re-query.
-            backBinding = query().Single(k => k.ActionInt == (int)GlobalAction.Back);
-            Assert.That(backBinding.KeyCombination.Keys, Is.EquivalentTo(new[] { InputKey.BackSpace }));
         }
 
         [TearDown]

osu.Game/Database/IRealmFactory.cs

@@ -9,6 +9,7 @@ namespace osu.Game.Database
     {
         /// <summary>
         /// The main realm context, bound to the update thread.
+        /// If querying from a non-update thread is needed, use <see cref="GetForRead"/> or <see cref="GetForWrite"/> to receive a context instead.
         /// </summary>
         Realm Context { get; }
 

osu.Game/Database/RealmContextFactory.cs

@@ -2,8 +2,10 @@
 // See the LICENCE file in the repository root for full licence text.
 
 using System;
+using System.Diagnostics;
 using System.Threading;
 using osu.Framework.Allocation;
+using osu.Framework.Development;
 using osu.Framework.Graphics;
 using osu.Framework.Logging;
 using osu.Framework.Platform;
@@ -38,21 +40,29 @@ namespace osu.Game.Database
         private static readonly GlobalStatistic<int> pending_writes = GlobalStatistics.Get<int>("Realm", "Pending writes");
         private static readonly GlobalStatistic<int> active_usages = GlobalStatistics.Get<int>("Realm", "Active usages");
 
+        private readonly object updateContextLock = new object();
+
         private Realm context;
 
         public Realm Context
         {
             get
             {
-                if (context == null)
+                if (!ThreadSafety.IsUpdateThread)
+                    throw new InvalidOperationException($"Use {nameof(GetForRead)} or {nameof(GetForWrite)} when performing realm operations from a non-update thread");
+
+                lock (updateContextLock)
                 {
-                    context = createContext();
+                    if (context == null)
-                    Logger.Log($"Opened realm \"{context.Config.DatabasePath}\" at version {context.Config.SchemaVersion}");
+                    {
+                        context = createContext();
+                        Logger.Log($"Opened realm \"{context.Config.DatabasePath}\" at version {context.Config.SchemaVersion}");
+                    }
+
+                    // creating a context will ensure our schema is up-to-date and migrated.
+
+                    return context;
                 }
-
-                // creating a context will ensure our schema is up-to-date and migrated.
-
-                return context;
             }
         }
 
@@ -107,8 +117,11 @@ namespace osu.Game.Database
         {
             base.Update();
 
-            if (context?.Refresh() == true)
+            lock (updateContextLock)
-                refreshes.Value++;
+            {
+                if (context?.Refresh() == true)
+                    refreshes.Value++;
+            }
         }
 
         private Realm createContext()
@@ -154,9 +167,15 @@ namespace osu.Game.Database
         private void flushContexts()
         {
             Logger.Log(@"Flushing realm contexts...", LoggingTarget.Database);
+            Debug.Assert(blockingLock.CurrentCount == 0);
 
-            var previousContext = context;
+            Realm previousContext;
-            context = null;
+
+            lock (updateContextLock)
+            {
+                previousContext = context;
+                context = null;
+            }
 
             // wait for all threaded usages to finish
             while (active_usages.Value > 0)