From 1bb1366c9f1c7e3d6d0b43f0bdf7d7f0ce3fe551 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 25 Jan 2022 16:26:06 +0900 Subject: [PATCH 01/10] Fix notification reset events potentially arriving out of order if a block operation times out --- osu.Game/Database/RealmAccess.cs | 40 +++++++++++++++----------------- 1 file changed, 19 insertions(+), 21 deletions(-) diff --git a/osu.Game/Database/RealmAccess.cs b/osu.Game/Database/RealmAccess.cs index e6e3c9ee41..bf88313663 100644 --- a/osu.Game/Database/RealmAccess.cs +++ b/osu.Game/Database/RealmAccess.cs @@ -334,25 +334,6 @@ namespace osu.Game.Database } } - /// - /// 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. - /// - 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) @@ -605,9 +586,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); @@ -615,6 +603,16 @@ namespace osu.Game.Database updateRealm = null; } + // 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?.Post(_ => + { + foreach (var action in notificationsResetMap.Values) + action(); + }, null); + const int sleep_length = 200; int timeout = 5000; From ffd7877a1e8b6b822fd5f5922111b3d1ae7c027b Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 25 Jan 2022 17:41:22 +0900 Subject: [PATCH 02/10] Remove synchronization context hacks in realm tests --- osu.Game.Tests/Database/GeneralUsageTests.cs | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/osu.Game.Tests/Database/GeneralUsageTests.cs b/osu.Game.Tests/Database/GeneralUsageTests.cs index dc0d42595b..8262ef18d4 100644 --- a/osu.Game.Tests/Database/GeneralUsageTests.cs +++ b/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(() => { 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); - } } } From 4a9f4eecba45e66428de23094f1d1387199bb450 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 25 Jan 2022 20:49:52 +0900 Subject: [PATCH 03/10] Use blocking calls to `SynchronizationContext` to guarantee order of execution --- osu.Game/Database/RealmAccess.cs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/osu.Game/Database/RealmAccess.cs b/osu.Game/Database/RealmAccess.cs index bf88313663..ffaae9b4ae 100644 --- a/osu.Game/Database/RealmAccess.cs +++ b/osu.Game/Database/RealmAccess.cs @@ -607,7 +607,7 @@ namespace osu.Game.Database // 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?.Post(_ => + syncContext?.Send(_ => { foreach (var action in notificationsResetMap.Values) action(); @@ -647,8 +647,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); } } From 4fe3d83fc40a176edf7ba9a1e3785c0fb57d24f4 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 26 Jan 2022 15:21:14 +0900 Subject: [PATCH 04/10] Update framework --- osu.Android.props | 2 +- osu.Game/osu.Game.csproj | 2 +- osu.iOS.props | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/osu.Android.props b/osu.Android.props index 4e5b9fdbb1..f85a96f819 100644 --- a/osu.Android.props +++ b/osu.Android.props @@ -52,7 +52,7 @@ - + diff --git a/osu.Game/osu.Game.csproj b/osu.Game/osu.Game.csproj index af5d8a5920..aa6fb93aa0 100644 --- a/osu.Game/osu.Game.csproj +++ b/osu.Game/osu.Game.csproj @@ -36,7 +36,7 @@ runtime; build; native; contentfiles; analyzers; buildtransitive - + diff --git a/osu.iOS.props b/osu.iOS.props index 2bcdea61b3..fbb4688588 100644 --- a/osu.iOS.props +++ b/osu.iOS.props @@ -60,7 +60,7 @@ - + @@ -83,7 +83,7 @@ - + From 5ea781faef9a4fe2de3be7a72ce84ad2306ecf23 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 26 Jan 2022 15:24:53 +0900 Subject: [PATCH 05/10] `Send` unsubscribe actions to synchronization context for consistency and safety --- osu.Game/Database/RealmAccess.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Database/RealmAccess.cs b/osu.Game/Database/RealmAccess.cs index 3866138d40..3572446aef 100644 --- a/osu.Game/Database/RealmAccess.cs +++ b/osu.Game/Database/RealmAccess.cs @@ -300,7 +300,7 @@ namespace osu.Game.Database return new InvokeOnDisposal(() => { if (ThreadSafety.IsUpdateThread) - unsubscribe(); + syncContext.Send(_ => unsubscribe(), null); else syncContext.Post(_ => unsubscribe(), null); From 24bcba64183a7bf404eca503a3c31a24762bfdac Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 26 Jan 2022 15:57:05 +0900 Subject: [PATCH 06/10] Move final result set firing to before the update realm is disposed Without this, if any registered callback attempts to access `RealmAccess.Realm` when handling the empty set callback, it will deadlock the game. --- osu.Game/Database/RealmAccess.cs | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/osu.Game/Database/RealmAccess.cs b/osu.Game/Database/RealmAccess.cs index 3572446aef..1a40176c1d 100644 --- a/osu.Game/Database/RealmAccess.cs +++ b/osu.Game/Database/RealmAccess.cs @@ -604,20 +604,18 @@ namespace osu.Game.Database Logger.Log(@"Blocking realm operations.", LoggingTarget.Database); + // Force a flush of any pending callbacks in the synchronization context. + // We want to ensure that the empty set callbacks are the last thing to arrive. + syncContext?.Send(_ => + { + foreach (var action in notificationsResetMap.Values) + action(); + }, null); + updateRealm?.Dispose(); updateRealm = null; } - // 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(_ => - { - foreach (var action in notificationsResetMap.Values) - action(); - }, null); - const int sleep_length = 200; int timeout = 5000; From 11f0f3c17de6986c7aa6f6a3915b239bf2b5543e Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 26 Jan 2022 16:21:24 +0900 Subject: [PATCH 07/10] Revert "Move final result set firing to before the update realm is disposed" This reverts commit 24bcba64183a7bf404eca503a3c31a24762bfdac. --- osu.Game/Database/RealmAccess.cs | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/osu.Game/Database/RealmAccess.cs b/osu.Game/Database/RealmAccess.cs index 1a40176c1d..3572446aef 100644 --- a/osu.Game/Database/RealmAccess.cs +++ b/osu.Game/Database/RealmAccess.cs @@ -604,18 +604,20 @@ namespace osu.Game.Database Logger.Log(@"Blocking realm operations.", LoggingTarget.Database); - // Force a flush of any pending callbacks in the synchronization context. - // We want to ensure that the empty set callbacks are the last thing to arrive. - syncContext?.Send(_ => - { - foreach (var action in notificationsResetMap.Values) - action(); - }, null); - updateRealm?.Dispose(); updateRealm = null; } + // 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(_ => + { + foreach (var action in notificationsResetMap.Values) + action(); + }, null); + const int sleep_length = 200; int timeout = 5000; From 791ea0308f8026c127d26ee2118b083ffa72abf9 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 26 Jan 2022 17:09:28 +0900 Subject: [PATCH 08/10] Add flag to guard against deadlocks during blocking operations --- osu.Game/Database/RealmAccess.cs | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/osu.Game/Database/RealmAccess.cs b/osu.Game/Database/RealmAccess.cs index 3572446aef..dc7c8bf668 100644 --- a/osu.Game/Database/RealmAccess.cs +++ b/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"); @@ -614,8 +619,20 @@ namespace osu.Game.Database // calls above. syncContext?.Send(_ => { - foreach (var action in notificationsResetMap.Values) - action(); + // 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); const int sleep_length = 200; From 885fb92aada99e4dfccb0904f544e16a093787ba Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 26 Jan 2022 17:21:57 +0900 Subject: [PATCH 09/10] Move final empty result set sending to post-compact --- osu.Game/Database/RealmAccess.cs | 44 ++++++++++++++++---------------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/osu.Game/Database/RealmAccess.cs b/osu.Game/Database/RealmAccess.cs index dc7c8bf668..9bdbebfe89 100644 --- a/osu.Game/Database/RealmAccess.cs +++ b/osu.Game/Database/RealmAccess.cs @@ -613,28 +613,6 @@ namespace osu.Game.Database updateRealm = null; } - // 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); - const int sleep_length = 200; int timeout = 5000; @@ -656,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 { From abe2cccaaecd1a01b5f3855705b4195958ef11da Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 26 Jan 2022 19:03:26 +0900 Subject: [PATCH 10/10] Fix completely invalid method of testing realm migration --- osu.Game.Tests/Database/RealmLiveTests.cs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/osu.Game.Tests/Database/RealmLiveTests.cs b/osu.Game.Tests/Database/RealmLiveTests.cs index 3f81b36378..4bc1f5078a 100644 --- a/osu.Game.Tests/Database/RealmLiveTests.cs +++ b/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)); }