Fix failing sync tests #3096
Fix failing sync tests #3096
Conversation
nirinchev
changed the title
# Conflicts: # Tests/Realm.Tests/Generated/Realm.SourceGenerator/Realms.SourceGenerator.RealmGenerator/ObjectWithPartitionValue_generated.cs
| if (SharedRealmHandle.OwnsNativeRealm) | ||
| { | ||
| _state.RemoveRealm(this); | ||
| } | ||
| _state.RemoveRealm(this, closeOnEmpty: SharedRealmHandle.OwnsNativeRealm); |
There was a problem hiding this comment.
This changes the behavior to always remove the Realm from the state, even when it's unowned. The reason is that otherwise we ended up not closing the Realm in certain situations. Example:
var config = new RealmConfiguration
{
PopulateInitialData = (r) =>
{
// (1)
}
}
using var realm = Realm.GetInstance(config);
// (2)
At point (1), we create an unowned Realm instance - r. When created, it will create a RealmState and set it to the native instance. At point (2), we get a regular Realm instance, that points to the same native Realm as r. This means that the state now references two Realms - r and realm. When we dispose realm, we remove it from the state, but we don't close the native realm because we see that the _weakRealms collection is not empty.
With this change, we remove unowned realms from the state, but don't explicitly call close on the native Realm, because we don't own it (yet). When an owned instance is created for the native Realm, we're free to close it, because there won't be unowned managed instances that keep it around.
| @@ -146,11 +146,25 @@ protected Realm GetRealm(string path) | |||
| return result; | |||
| } | |||
|
|
|||
| protected async Task<Realm> GetRealmAsync(RealmConfigurationBase config, CancellationToken cancellationToken = default) | |||
| protected async Task<Realm> GetRealmAsync(RealmConfigurationBase config, int timeout = 10000, CancellationToken? cancellationToken = default) | |||
There was a problem hiding this comment.
This change is aimed at making sure we do proper cleanup on timeout. Previously we were leaking sessions for Realms we couldn't open due to a timeout. E.g.:
var r = await GetRealmAsync(config).Timeout(1000);If we timeout here, we wouldn't get a Realm reference that we could enqueue for cleanup, so when the test fails, we won't close the Realm/terminate the session. With this change, we're using the built-in cancellation mechanics to stop the download.
| @@ -102,6 +102,8 @@ void HandleSessionError(object _, ErrorEventArgs errorArgs) | |||
| { | |||
| TestHelpers.RunAsyncTest(testFunc, timeout); | |||
| } | |||
|
|
|||
| Task.Delay(1000).Wait(); | |||
There was a problem hiding this comment.
This is working around realm/realm-core#6052
# Conflicts: # Tests/Realm.Tests/Generated/Realm.SourceGenerator/Realms.SourceGenerator.RealmGenerator/ObjectWithPartitionValue_generated.cs # Tests/Realm.Tests/Sync/SyncTestBase.cs
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
I just have a few questions here and there to get some clarifications. Otherwise, all seems good.
It must have taken a lot of time to figure out all those, more or less, sporadic issues. Thank you for having taken the time.
As a side note, the comments that you left behind were very useful to quickly have context and understand the changes.
| @@ -269,6 +269,10 @@ public async Task<SubscriptionSetState> WaitForStateChangeAsync() | |||
|
|
|||
| return await tcs.Task; | |||
| } | |||
| catch (Exception ex) when (ex.Message == "Active SubscriptionSet without a SubscriptionStore") | |||
| { | |||
| throw new TaskCanceledException("The SubscriptionSet was closed before the wait could complete. This is likely because the Realm it belongs to was disposed."); | |||
There was a problem hiding this comment.
Would make sense to move this message to core?
There was a problem hiding this comment.
This is somewhat c#-specific message, so I don't think the Core team would like to adopt it.
|
|
||
| Logger.Default = Logger.Function(sw.WriteLine); | ||
| var logger = new Logger.InMemoryLogger(); | ||
| Logger.Default = logger; |
There was a problem hiding this comment.
Do we need to reset the original logger?
There was a problem hiding this comment.
This is done automatically in RealmTest.TearDown
* main: Survey ad (#3201) Workaround Mono yield bug (#3200) Bump docker layer caching action to use node16 (#3193) Keypath filtering (#3179) Move realm files created by tests to current directory on CI (#2991) Fix failing sync tests (#3096) Updated example project to MAUI and Source Generators (#3168) Fix uwp workflow when running in debug (#3177) Deprecate push client (#3176) Don't use reflection in DynamicRealmObjectHelper.TryGetPrimaryKeyValue (#3175) Keep track of the active transaction and close it on realm close (#3164) Add more detailed unsupported type error (#3163) Build Xamarin.Android tests in all ABIs (#3165) # Conflicts: # Tests/Realm.Tests/Generated/Realm.SourceGenerator/Realms.SourceGenerator.RealmGenerator/ObjectWithPartitionValue_generated.cs
Description
Try and diagnose the reason for tests timing out.
Fixes #3063
TODO
WriteCopy_CanSynchronizeData- it appears like there's conflict resolution happening there when it shouldn't. Run in isolation and inspect server logs.