Merge branch 'master' into realm-integration/skins-rebase

osu.Game.Tests/Database/RealmLiveTests.cs

@@ -62,53 +62,6 @@ namespace osu.Game.Tests.Database
             Assert.IsFalse(liveBeatmap.PerformRead(l => l.Hidden));
         }
 
-        [Test]
-        public void TestValueAccessWithOpenContext()
-        {
-            RunTestWithRealm((realmFactory, _) =>
-            {
-                ILive<RealmBeatmap>? liveBeatmap = null;
-                Task.Factory.StartNew(() =>
-                {
-                    using (var threadContext = realmFactory.CreateContext())
-                    {
-                        var beatmap = threadContext.Write(r => r.Add(new RealmBeatmap(CreateRuleset(), new RealmBeatmapDifficulty(), new RealmBeatmapMetadata())));
-
-                        liveBeatmap = beatmap.ToLive();
-                    }
-                }, TaskCreationOptions.LongRunning | TaskCreationOptions.HideScheduler).Wait();
-
-                Debug.Assert(liveBeatmap != null);
-
-                Task.Factory.StartNew(() =>
-                {
-                    // TODO: The commented code is the behaviour we hope to obtain, but is temporarily disabled.
-                    // See https://github.com/ppy/osu/pull/15851
-                    using (realmFactory.CreateContext())
-                    {
-                        Assert.Throws<InvalidOperationException>(() =>
-                        {
-                            var __ = liveBeatmap.Value;
-                        });
-                    }
-
-                    // Assert.DoesNotThrow(() =>
-                    // {
-                    //     using (realmFactory.CreateContext())
-                    //     {
-                    //         var resolved = liveBeatmap.Value;
-                    //
-                    //         Assert.IsTrue(resolved.Realm.IsClosed);
-                    //         Assert.IsTrue(resolved.IsValid);
-                    //
-                    //         // can access properties without a crash.
-                    //         Assert.IsFalse(resolved.Hidden);
-                    //     }
-                    // });
-                }, TaskCreationOptions.LongRunning | TaskCreationOptions.HideScheduler).Wait();
-            });
-        }
-
         [Test]
         public void TestScopedReadWithoutContext()
         {
@@ -164,6 +117,60 @@ namespace osu.Game.Tests.Database
             });
         }
 
+        [Test]
+        public void TestValueAccessNonManaged()
+        {
+            RunTestWithRealm((realmFactory, _) =>
+            {
+                var beatmap = new RealmBeatmap(CreateRuleset(), new RealmBeatmapDifficulty(), new RealmBeatmapMetadata());
+                var liveBeatmap = beatmap.ToLive();
+
+                Assert.DoesNotThrow(() =>
+                {
+                    var __ = liveBeatmap.Value;
+                });
+            });
+        }
+
+        [Test]
+        public void TestValueAccessWithOpenContextFails()
+        {
+            RunTestWithRealm((realmFactory, _) =>
+            {
+                ILive<RealmBeatmap>? liveBeatmap = null;
+
+                Task.Factory.StartNew(() =>
+                {
+                    using (var threadContext = realmFactory.CreateContext())
+                    {
+                        var beatmap = threadContext.Write(r => r.Add(new RealmBeatmap(CreateRuleset(), new RealmBeatmapDifficulty(), new RealmBeatmapMetadata())));
+
+                        liveBeatmap = beatmap.ToLive();
+                    }
+                }, TaskCreationOptions.LongRunning | TaskCreationOptions.HideScheduler).Wait();
+
+                Debug.Assert(liveBeatmap != null);
+
+                Task.Factory.StartNew(() =>
+                {
+                    // Can't be used, without a valid context.
+                    Assert.Throws<InvalidOperationException>(() =>
+                    {
+                        var __ = liveBeatmap.Value;
+                    });
+
+                    // Can't be used, even from within a valid context.
+                    using (realmFactory.CreateContext())
+                    {
+                        Assert.Throws<InvalidOperationException>(() =>
+                        {
+                            var __ = liveBeatmap.Value;
+                        });
+                    }
+                }, TaskCreationOptions.LongRunning | TaskCreationOptions.HideScheduler).Wait();
+            });
+        }
+
         [Test]
         public void TestValueAccessWithoutOpenContextFails()
         {
@@ -225,8 +232,6 @@ namespace osu.Game.Tests.Database
                     Assert.AreEqual(0, updateThreadContext.All<RealmBeatmap>().Count());
                     Assert.AreEqual(0, changesTriggered);
 
-                    // TODO: Originally the following was using `liveBeatmap.Value`. This has been temporarily disabled.
-                    // See https://github.com/ppy/osu/pull/15851
                     liveBeatmap.PerformRead(resolved =>
                     {
                         // retrieval causes an implicit refresh. even changes that aren't related to the retrieval are fired at this point.
@@ -234,11 +239,6 @@ namespace osu.Game.Tests.Database
                         Assert.AreEqual(2, updateThreadContext.All<RealmBeatmap>().Count());
                         Assert.AreEqual(1, changesTriggered);
 
-                        // TODO: as above, temporarily disabled as it doesn't make sense inside a `PerformRead`.
-                        // // even though the realm that this instance was resolved for was closed, it's still valid.
-                        // Assert.IsTrue(resolved.Realm.IsClosed);
-                        // Assert.IsTrue(resolved.IsValid);
-
                         // can access properties without a crash.
                         Assert.IsFalse(resolved.Hidden);
 

osu.Game/Database/ILive.cs

@@ -38,10 +38,10 @@ namespace osu.Game.Database
         bool IsManaged { get; }
 
         /// <summary>
-        /// Resolve the value of this instance on the current thread's context.
+        /// Resolve the value of this instance on the update thread.
         /// </summary>
         /// <remarks>
-        /// After resolving the data should not be passed between threads.
+        /// After resolving, the data should not be passed between threads.
         /// </remarks>
         T Value { get; }
     }

osu.Game/Database/RealmLive.cs

@@ -2,7 +2,7 @@
 // See the LICENCE file in the repository root for full licence text.
 
 using System;
-using System.Threading;
+using osu.Framework.Development;
 using Realms;
 
 #nullable enable
@@ -19,9 +19,6 @@ namespace osu.Game.Database
 
         public bool IsManaged => data.IsManaged;
 
-        private readonly SynchronizationContext? fetchedContext;
-        private readonly int fetchedThreadId;
-
         /// <summary>
         /// The original live data used to create this instance.
         /// </summary>
@@ -35,12 +32,6 @@ namespace osu.Game.Database
         {
             this.data = data;
 
-            if (data.IsManaged)
-            {
-                fetchedContext = SynchronizationContext.Current;
-                fetchedThreadId = Thread.CurrentThread.ManagedThreadId;
-            }
-
             ID = data.ID;
         }
 
@@ -50,7 +41,7 @@ namespace osu.Game.Database
         /// <param name="perform">The action to perform.</param>
         public void PerformRead(Action<T> perform)
         {
-            if (originalDataValid)
+            if (!IsManaged)
             {
                 perform(data);
                 return;
@@ -69,7 +60,7 @@ namespace osu.Game.Database
             if (typeof(RealmObjectBase).IsAssignableFrom(typeof(TReturn)))
                 throw new InvalidOperationException($"Realm live objects should not exit the scope of {nameof(PerformRead)}.");
 
-            if (originalDataValid)
+            if (!IsManaged)
                 return perform(data);
 
             using (var realm = Realm.GetInstance(data.Realm.Config))
@@ -97,31 +88,20 @@ namespace osu.Game.Database
         {
             get
             {
-                if (originalDataValid)
+                if (!IsManaged)
                     return data;
 
-                if (!isCorrectThread)
+                if (!ThreadSafety.IsUpdateThread)
-                    throw new InvalidOperationException($"Can't use {nameof(Value)} unless on the same thread the original data was fetched from.");
+                    throw new InvalidOperationException($"Can't use {nameof(Value)} on managed objects from non-update threads");
 
+                // When using Value, we rely on garbage collection for the realm instance used to retrieve the instance.
+                // As we are sure that this is on the update thread, there should always be an open and constantly refreshing realm instance to ensure file size growth is a non-issue.
                 var realm = Realm.GetInstance(data.Realm.Config);
-                var retrieved = realm.Find<T>(ID);
 
-                if (!retrieved.IsValid)
+                return realm.Find<T>(ID);
-                    throw new InvalidOperationException("Attempted to access value without an open context");
-
-                return retrieved;
             }
         }
 
-        // TODO: Revisit adding these conditionals back as an optimisation: || (isCorrectThread && data.IsValid);
-        // They have temporarily been removed due to an oversight involving .AsQueryable, see https://github.com/realm/realm-dotnet/discussions/2734.
-        // This means we are fetching a new context every `PerformRead` or `PerformWrite`, even when on the correct thread.
-        private bool originalDataValid => !IsManaged;
-
-        // this matches realm's internal thread validation (see https://github.com/realm/realm-dotnet/blob/903b4d0b304f887e37e2d905384fb572a6496e70/Realm/Realm/Native/SynchronizationContextScheduler.cs#L72)
-        private bool isCorrectThread
-            => (fetchedContext != null && SynchronizationContext.Current == fetchedContext) || fetchedThreadId == Thread.CurrentThread.ManagedThreadId;
-
         public bool Equals(ILive<T>? other) => ID == other?.ID;
 
         public override string ToString() => PerformRead(i => i.ToString());