Merge pull request #24562 from peppy/fix-unsafe-realm-access

Fix incorrect access to realm object in beatmap editor
This commit is contained in:
Bartłomiej Dach 2023-08-16 10:31:35 +02:00 committed by GitHub
commit e624f603b3
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 36 additions and 40 deletions

View File

@ -92,25 +92,6 @@ namespace osu.Game.Tests.Visual.Editing
}
[Test]
[FlakyTest]
/*
* Fail rate around 1.2%.
*
* Failing with realm refetch occasionally being null.
* My only guess is that the WorkingBeatmap at SetupScreen is dummy instead of the true one.
* If it's something else, we have larger issues with realm, but I don't think that's the case.
*
* at osu.Framework.Logging.ThrowingTraceListener.Fail(String message1, String message2)
* at System.Diagnostics.TraceInternal.Fail(String message, String detailMessage)
* at System.Diagnostics.TraceInternal.TraceProvider.Fail(String message, String detailMessage)
* at System.Diagnostics.Debug.Fail(String message, String detailMessage)
* at osu.Game.Database.ModelManager`1.<>c__DisplayClass8_0.<performFileOperation>b__0(Realm realm) ModelManager.cs:line 50
* at osu.Game.Database.RealmExtensions.Write(Realm realm, Action`1 function) RealmExtensions.cs:line 14
* at osu.Game.Database.ModelManager`1.performFileOperation(TModel item, Action`1 operation) ModelManager.cs:line 47
* at osu.Game.Database.ModelManager`1.AddFile(TModel item, Stream contents, String filename) ModelManager.cs:line 37
* at osu.Game.Screens.Edit.Setup.ResourcesSection.ChangeAudioTrack(FileInfo source) ResourcesSection.cs:line 115
* at osu.Game.Tests.Visual.Editing.TestSceneEditorBeatmapCreation.<TestAddAudioTrack>b__11_0() TestSceneEditorBeatmapCreation.cs:line 101
*/
public void TestAddAudioTrack()
{
AddAssert("track is virtual", () => Beatmap.Value.Track is TrackVirtual);

View File

@ -52,7 +52,7 @@ namespace osu.Game.Database
// (ie. if an async import finished very recently).
Realm.Realm.Write(realm =>
{
var managed = realm.Find<TModel>(item.ID);
var managed = realm.FindWithRefresh<TModel>(item.ID);
Debug.Assert(managed != null);
operation(managed);

View File

@ -8,6 +8,34 @@ namespace osu.Game.Database
{
public static class RealmExtensions
{
/// <summary>
/// Performs a <see cref="Realm.Find{T}(System.Nullable{long})"/>.
/// If a match was not found, a <see cref="Realm.Refresh"/> is performed before trying a second time.
/// This ensures that an instance is found even if the realm requested against was not in a consistent state.
/// </summary>
/// <param name="realm">The realm to operate on.</param>
/// <param name="id">The ID of the entity to find in the realm.</param>
/// <typeparam name="T">The type of the entity to find in the realm.</typeparam>
/// <returns>
/// The retrieved entity of type <typeparamref name="T"/>.
/// Can be <see langword="null"/> if the entity is still not found by <paramref name="id"/> even after a refresh.
/// </returns>
public static T? FindWithRefresh<T>(this Realm realm, Guid id) where T : IRealmObject
{
var found = realm.Find<T>(id);
if (found == null)
{
// It may be that we access this from the update thread before a refresh has taken place.
// To ensure that behaviour matches what we'd expect (the object generally *should be* available), force
// a refresh to bring in any off-thread changes immediately.
realm.Refresh();
found = realm.Find<T>(id);
}
return found;
}
/// <summary>
/// Perform a write operation against the provided realm instance.
/// </summary>

View File

@ -30,7 +30,7 @@ namespace osu.Game.Database
/// <summary>
/// Construct a new instance of live realm data.
/// </summary>
/// <param name="data">The realm data.</param>
/// <param name="data">The realm data. Must be managed (see <see cref="IRealmObjectBase.IsManaged"/>).</param>
/// <param name="realm">The realm factory the data was sourced from. May be null for an unmanaged object.</param>
public RealmLive(T data, RealmAccess realm)
: base(data.ID)
@ -62,7 +62,7 @@ namespace osu.Game.Database
return;
}
perform(retrieveFromID(r));
perform(r.FindWithRefresh<T>(ID)!);
RealmLiveStatistics.USAGE_ASYNC.Value++;
});
}
@ -84,7 +84,7 @@ namespace osu.Game.Database
return realm.Run(r =>
{
var returnData = perform(retrieveFromID(r));
var returnData = perform(r.FindWithRefresh<T>(ID)!);
RealmLiveStatistics.USAGE_ASYNC.Value++;
if (returnData is RealmObjectBase realmObject && realmObject.IsManaged)
@ -141,25 +141,10 @@ namespace osu.Game.Database
}
dataIsFromUpdateThread = true;
data = retrieveFromID(realm.Realm);
data = realm.Realm.FindWithRefresh<T>(ID)!;
RealmLiveStatistics.USAGE_UPDATE_REFETCH.Value++;
}
private T retrieveFromID(Realm realm)
{
var found = realm.Find<T>(ID);
if (found == null)
{
// It may be that we access this from the update thread before a refresh has taken place.
// To ensure that behaviour matches what we'd expect (the object *is* available), force
// a refresh to bring in any off-thread changes immediately.
realm.Refresh();
found = realm.Find<T>(ID)!;
}
return found;
}
}
internal static class RealmLiveStatistics

View File

@ -199,6 +199,8 @@ namespace osu.Game.Screens.Edit
if (loadableBeatmap is DummyWorkingBeatmap)
{
Logger.Log("Editor was loaded without a valid beatmap; creating a new beatmap.");
isNewBeatmap = true;
loadableBeatmap = beatmapManager.CreateNew(Ruleset.Value, api.LocalUser.Value);