Merge pull request #16606 from peppy/fix-out-of-order-events-on-block-fail

Fix notification reset events potentially arriving out of order if a block operation times out

osu.Android.props

@@ -52,7 +52,7 @@
   </ItemGroup>
   <ItemGroup>
     <PackageReference Include="ppy.osu.Game.Resources" Version="2022.115.0" />
-    <PackageReference Include="ppy.osu.Framework.Android" Version="2022.125.0" />
+    <PackageReference Include="ppy.osu.Framework.Android" Version="2022.126.0" />
   </ItemGroup>
   <ItemGroup Label="Transitive Dependencies">
     <!-- Realm needs to be directly referenced in all Xamarin projects, as it will not pull in its transitive dependencies otherwise. -->

osu.Game.Tests/Database/GeneralUsageTests.cs

@@ -87,11 +87,6 @@ namespace osu.Game.Tests.Database
 
                 hasThreadedUsage.Wait();
 
-                // Usually the host would run the synchronization context work per frame.
-                // For the sake of keeping this test simple (there's only one update invocation),
-                // let's replace it so we can ensure work is run immediately.
-                SynchronizationContext.SetSynchronizationContext(new ImmediateExecuteSynchronizationContext());
-
                 Assert.Throws<TimeoutException>(() =>
                 {
                     using (realm.BlockAllOperations())
@@ -107,10 +102,5 @@ namespace osu.Game.Tests.Database
                 }
             });
         }
-
-        private class ImmediateExecuteSynchronizationContext : SynchronizationContext
-        {
-            public override void Post(SendOrPostCallback d, object? state) => d(state);
-        }
     }
 }

osu.Game.Tests/Database/RealmLiveTests.cs

@@ -47,16 +47,14 @@ namespace osu.Game.Tests.Database
                     liveBeatmap = beatmap.ToLive(realm);
                 });
 
-                using (realm.BlockAllOperations())
-                {
-                    // recycle realm before migrating
-                }
-
                 using (var migratedStorage = new TemporaryNativeStorage("realm-test-migration-target"))
                 {
                     migratedStorage.DeleteDirectory(string.Empty);
 
-                    storage.Migrate(migratedStorage);
+                    using (realm.BlockAllOperations())
+                    {
+                        storage.Migrate(migratedStorage);
+                    }
 
                     Assert.IsFalse(liveBeatmap?.PerformRead(l => l.Hidden));
                 }

osu.Game/Database/RealmAccess.cs

@@ -89,10 +89,15 @@ namespace osu.Game.Database
 
         private Realm? updateRealm;
 
+        private bool isSendingNotificationResetEvents;
+
         public Realm Realm => ensureUpdateRealm();
 
         private Realm ensureUpdateRealm()
         {
+            if (isSendingNotificationResetEvents)
+                throw new InvalidOperationException("Cannot retrieve a realm context from a notification callback during a blocking operation.");
+
             if (!ThreadSafety.IsUpdateThread)
                 throw new InvalidOperationException(@$"Use {nameof(getRealmInstance)} when performing realm operations from a non-update thread");
 
@@ -300,7 +305,7 @@ namespace osu.Game.Database
             return new InvokeOnDisposal(() =>
             {
                 if (ThreadSafety.IsUpdateThread)
-                    unsubscribe();
+                    syncContext.Send(_ => unsubscribe(), null);
                 else
                     syncContext.Post(_ => unsubscribe(), null);
 
@@ -339,25 +344,6 @@ namespace osu.Game.Database
             }
         }
 
-        /// <summary>
-        /// Unregister all subscriptions when the realm instance is to be recycled.
-        /// Subscriptions will still remain and will be re-subscribed when the realm instance returns.
-        /// </summary>
-        private void unregisterAllSubscriptions()
-        {
-            lock (realmLock)
-            {
-                foreach (var action in notificationsResetMap.Values)
-                    action();
-
-                foreach (var action in customSubscriptionsResetMap)
-                {
-                    action.Value?.Dispose();
-                    customSubscriptionsResetMap[action.Key] = null;
-                }
-            }
-        }
-
         private Realm getRealmInstance()
         {
             if (isDisposed)
@@ -610,9 +596,16 @@ namespace osu.Game.Database
                             throw new InvalidOperationException(@$"{nameof(BlockAllOperations)} must be called from the update thread.");
 
                         syncContext = SynchronizationContext.Current;
-                    }
 
-                    unregisterAllSubscriptions();
+                        // Before disposing the update context, clean up all subscriptions.
+                        // Note that in the case of realm notification subscriptions, this is not really required (they will be cleaned up by disposal).
+                        // In the case of custom subscriptions, we want them to fire before the update realm is disposed in case they do any follow-up work.
+                        foreach (var action in customSubscriptionsResetMap)
+                        {
+                            action.Value?.Dispose();
+                            customSubscriptionsResetMap[action.Key] = null;
+                        }
+                    }
 
                     Logger.Log(@"Blocking realm operations.", LoggingTarget.Database);
 
@@ -641,6 +634,28 @@ namespace osu.Game.Database
                     // We still want to continue with the blocking operation, though.
                     Logger.Log($"Realm compact failed with error {e}", LoggingTarget.Database);
                 }
+
+                // In order to ensure events arrive in the correct order, these *must* be fired post disposal of the update realm,
+                // and must be posted to the synchronization context.
+                // This is because realm may fire event callbacks between the `unregisterAllSubscriptions` and `updateRealm.Dispose`
+                // calls above.
+                syncContext?.Send(_ =>
+                {
+                    // Flag ensures that we don't get in a deadlocked scenario due to a callback attempting to access `RealmAccess.Realm` or `RealmAccess.Run`
+                    // and hitting `realmRetrievalLock` a second time. Generally such usages should not exist, and as such we throw when an attempt is made
+                    // to use in this fashion.
+                    isSendingNotificationResetEvents = true;
+
+                    try
+                    {
+                        foreach (var action in notificationsResetMap.Values)
+                            action();
+                    }
+                    finally
+                    {
+                        isSendingNotificationResetEvents = false;
+                    }
+                }, null);
             }
             catch
             {
@@ -654,8 +669,14 @@ namespace osu.Game.Database
             {
                 Logger.Log(@"Restoring realm operations.", LoggingTarget.Database);
                 realmRetrievalLock.Release();
+
                 // Post back to the update thread to revive any subscriptions.
-                syncContext?.Post(_ => ensureUpdateRealm(), null);
+                // In the case we are on the update thread, let's also require this to run synchronously.
+                // This requirement is mostly due to test coverage, but shouldn't cause any harm.
+                if (ThreadSafety.IsUpdateThread)
+                    syncContext?.Send(_ => ensureUpdateRealm(), null);
+                else
+                    syncContext?.Post(_ => ensureUpdateRealm(), null);
             }
         }
 

osu.Game/osu.Game.csproj

@@ -36,7 +36,7 @@
       <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
     </PackageReference>
     <PackageReference Include="Realm" Version="10.8.0" />
-    <PackageReference Include="ppy.osu.Framework" Version="2022.125.0" />
+    <PackageReference Include="ppy.osu.Framework" Version="2022.126.0" />
     <PackageReference Include="ppy.osu.Game.Resources" Version="2022.115.0" />
     <PackageReference Include="Sentry" Version="3.13.0" />
     <PackageReference Include="SharpCompress" Version="0.30.1" />

osu.iOS.props

@@ -60,7 +60,7 @@
     <Reference Include="System.Net.Http" />
   </ItemGroup>
   <ItemGroup Label="Package References">
-    <PackageReference Include="ppy.osu.Framework.iOS" Version="2022.125.0" />
+    <PackageReference Include="ppy.osu.Framework.iOS" Version="2022.126.0" />
     <PackageReference Include="ppy.osu.Game.Resources" Version="2022.115.0" />
   </ItemGroup>
   <!-- See https://github.com/dotnet/runtime/issues/35988 (can be removed after Xamarin uses net5.0 / net6.0) -->
@@ -83,7 +83,7 @@
     <PackageReference Include="Microsoft.EntityFrameworkCore.Sqlite" Version="2.2.6" />
     <PackageReference Include="Microsoft.EntityFrameworkCore.Sqlite.Core" Version="2.2.6" />
     <PackageReference Include="Newtonsoft.Json" Version="13.0.1" />
-    <PackageReference Include="ppy.osu.Framework" Version="2022.125.0" />
+    <PackageReference Include="ppy.osu.Framework" Version="2022.126.0" />
     <PackageReference Include="SharpCompress" Version="0.30.0" />
     <PackageReference Include="NUnit" Version="3.13.2" />
     <PackageReference Include="System.ComponentModel.Annotations" Version="5.0.0" />