Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Cleanup resources better during Client Reset and closing Realms. #1515

Merged
merged 16 commits into from
Sep 22, 2023
Merged

Cleanup resources better during Client Reset and closing Realms. #1515

merged 16 commits into from
Sep 22, 2023

Conversation

cmelchior
Copy link
Contributor

This PR fixes some problems I discovered on Windows.

  • During Client Reset we were not closing a realm instance correctly, which will prevent the Realm file from being deleted later. This fixed most of our Client Reset Tests on Windows, which all timed out during tearDown.
  • Manually closing the SyncSession Pointer when closing the Realm make shutdown more predictable. Before this, it was GC dependant.
  • I switched to a more accurate pattern for testing client resets rather than us "faking" it.
  • I haven't added a changelog entry since we are still leaking something during a manual client reset. I'm still trying to hunt that one down.

@cla-bot cla-bot bot added the cla: yes label Sep 13, 2023
@cmelchior cmelchior marked this pull request as ready for review September 15, 2023 10:15
rorbech
rorbech reviewed Sep 15, 2023
View reviewed changes
packages/test-sync/build.gradle.kts Outdated Show resolved Hide resolved
packages/test-base/src/androidMain/kotlin/io/realm/kotlin/test/platform/PlatformUtils.kt
PosixFilePermission.OWNER_READ
)
)
// Use the File API as it works across Windows and POSIX.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or does it ... since you disabled the test on windows because it doesn't work 🤪

...es/library-sync/src/commonMain/kotlin/io/realm/kotlin/mongodb/internal/SyncedRealmContext.kt Outdated Show resolved Hide resolved
...ync/src/commonMain/kotlin/io/realm/kotlin/mongodb/exceptions/ClientResetRequiredException.kt Show resolved Hide resolved
...src/commonTest/kotlin/io/realm/kotlin/test/mongodb/common/SyncClientResetIntegrationTests.kt Outdated Show resolved Hide resolved
...src/commonTest/kotlin/io/realm/kotlin/test/mongodb/common/SyncClientResetIntegrationTests.kt Show resolved Hide resolved
nhachicha
nhachicha approved these changes Sep 19, 2023
View reviewed changes
Copy link
Collaborator

@nhachicha nhachicha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice fix 🎉 minor comments

packages/jni-swig-stub/src/main/jni/realm_api_helpers.cpp
@@ -906,7 +906,7 @@ after_client_reset(void* userdata, realm_t* before_realm,
realm_t* after_realm_ptr = realm_from_thread_safe_reference(after_realm, &scheduler);
auto after_pointer = wrap_pointer(env, reinterpret_cast<jlong>(after_realm_ptr), false);
env->CallVoidMethod(static_cast<jobject>(userdata), java_after_callback_function, before_pointer, after_pointer, did_recover);

realm_close(after_realm_ptr);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we also close the pointers for Darwin? https://github.com/realm/realm-kotlin/blob/main/packages/cinterop/src/nativeDarwin/kotlin/io/realm/kotlin/internal/interop/RealmInterop.kt#L2534-L2538

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, we probably should. I'm only closing them here because it prevents Windows from removing the file, but you are correct. We should also behave nice on Darwin.

packages/library-base/src/commonMain/kotlin/io/realm/kotlin/internal/RealmImpl.kt
runBlocking {
realmPointerMutex.withLock {
realmReferenceLock.withLock {
if (isClosed()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (isClosed()) {
if (isClosed.value) {

...nc/src/commonTest/kotlin/io/realm/kotlin/test/mongodb/common/FlexibleSyncIntegrationTests.kt
@@ -64,7 +63,7 @@ class FlexibleSyncIntegrationTests {

@BeforeTest
fun setup() {
app = TestApp(this::class.simpleName, appName = TEST_APP_FLEX, logLevel = LogLevel.ALL)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be helpful to keep it all for CI failures?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't remember having all the logs available was ever useful on CI. Just having the final exception seems to do the trick?

@cmelchior cmelchior merged commit 941d1fb into main Sep 22, 2023
1 check passed
@cmelchior cmelchior deleted the cm/improve-client-reset-resource-management branch September 22, 2023 07:35
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rorbech rorbech rorbech left review comments

@nhachicha nhachicha nhachicha approved these changes

@clementetb clementetb Awaiting requested review from clementetb

Assignees

@cmelchior cmelchior

Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cmelchior @nhachicha @rorbech