Cleanup resources better during Client Reset and closing Realms.

CHANGELOG.md

@@ -7,6 +7,7 @@
 * Realm will no longer set the JVM bytecode to 1.8 when applying the Realm plugin. ([#1513](https://github.com/realm/realm-kotlin/issues/1513))
 
 ### Fixed
+* [Sync] Manual client reset on Windows would not trigger correctly when run inside `onManualResetFallback`. (Issue [#1515](https://github.com/realm/realm-kotlin/pull/1515))  
 * [Sync] If calling a function on App Services that resulted in a redirect, it would only redirect for GET requests. (Issue [#1517](https://github.com/realm/realm-kotlin/pull/1517))
 
 ### Compatibility

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);
     if (env->ExceptionCheck()) {
         std::string exception_message = get_exception_message(env);
         std::string message_template = "An error has occurred in the 'onAfter' callback: ";

packages/library-base/src/commonMain/kotlin/io/realm/kotlin/internal/RealmImpl.kt

@@ -103,7 +103,8 @@ public class RealmImpl private constructor(
 
     // Injection point for synchronized Realms. This property should only be used to hold state
     // required by synchronized realms. See `SyncedRealmContext` for more details.
-    public var syncContext: AtomicRef<Any?> = atomic(null)
+    @OptIn(ExperimentalStdlibApi::class)
+    public var syncContext: AtomicRef<AutoCloseable?> = atomic(null)
 
     init {
         @Suppress("TooGenericExceptionCaught")
@@ -271,6 +272,8 @@ public class RealmImpl private constructor(
                 realmScope.cancel()
                 notifier.close()
                 versionTracker.close()
+                @OptIn(ExperimentalStdlibApi::class)
+                syncContext.value?.close()
                 // The local realmReference is pointing to a realm reference managed by either the
                 // version tracker, writer or notifier, so it is already closed
                 super.close()

packages/library-sync/src/commonMain/kotlin/io/realm/kotlin/mongodb/exceptions/ClientResetRequiredException.kt

@@ -56,9 +56,10 @@ public class ClientResetRequiredException constructor(
      * associated to the session in which this error is generated **must be closed**. Not doing so
      * might result in unexpected file system errors.
      *
+     * @return `true` if the Client Reset succeeded, `false` if not.
      * @throws IllegalStateException if not all instances have been closed.
      */
-    public fun executeClientReset() {
-        RealmInterop.realm_sync_immediately_run_file_actions(appPointer, originalFilePath)
+    public fun executeClientReset(): Boolean {
+        return RealmInterop.realm_sync_immediately_run_file_actions(appPointer, originalFilePath)
     }
 }

packages/library-sync/src/commonMain/kotlin/io/realm/kotlin/mongodb/internal/SyncConfigurationImpl.kt

@@ -36,6 +36,7 @@ import io.realm.kotlin.internal.interop.SyncErrorCallback
 import io.realm.kotlin.internal.interop.sync.SyncError
 import io.realm.kotlin.internal.interop.sync.SyncSessionResyncMode
 import io.realm.kotlin.internal.platform.fileExists
+import io.realm.kotlin.log.RealmLog
 import io.realm.kotlin.mongodb.exceptions.ClientResetRequiredException
 import io.realm.kotlin.mongodb.exceptions.DownloadingRealmTimeOutException
 import io.realm.kotlin.mongodb.subscriptions
@@ -52,8 +53,11 @@ import io.realm.kotlin.mongodb.sync.SyncSession
 import kotlinx.atomicfu.AtomicBoolean
 import kotlinx.atomicfu.AtomicRef
 import kotlinx.atomicfu.atomic
+import kotlinx.coroutines.DelicateCoroutinesApi
+import kotlinx.coroutines.GlobalScope
 import kotlinx.coroutines.TimeoutCancellationException
 import kotlinx.coroutines.channels.Channel
+import kotlinx.coroutines.launch
 import kotlinx.coroutines.withContext
 import kotlinx.coroutines.withTimeout
 import org.mongodb.kbson.BsonValue
@@ -194,10 +198,33 @@ internal class SyncConfigurationImpl(
             SyncErrorCallback { pointer: RealmSyncSessionPointer, error: SyncError ->
                 val session = SyncSessionImpl(pointer)
                 val syncError = convertSyncError(error)
-
-                // Notify before/after callbacks too if error is client reset
                 if (error.isClientResetRequested) {
-                    initializerHelper.onSyncError(session, frozenAppPointer, error)
+                    // If a Client Reset happened, we only get here if `onManualResetFallback` needs
+                    // to be called. This means there is a high likelihood that users will want to
+                    // call ClientResetRequiredException.executeClientReset() inside the callback.
+                    //
+                    // In order to do that, they will need to close the Realm first.
+                    //
+                    // On POSIX this will work fine, but on Windows this will fail as the
+                    // C++ session still holds a DBPointer preventing the release of the file during
+                    // the callback.
+                    //
+                    // So, in order to prevent errors on Windows, we are running the Kotlin callback
+                    // on a separate worker thread. This will allow Core to finish its callback so
+                    // when we close the Realm from the worker thread, the underlying
+                    // session can also be fully freed.
+                    //
+                    // Given that we do not make any promises regarding which thread the callback
+                    // is running on. This should be fine.
+                    @OptIn(DelicateCoroutinesApi::class)
+                    try {
+                        GlobalScope.launch {
+                            initializerHelper.onSyncError(session, frozenAppPointer, error)
+                        }
+                    } catch (ex: Exception) {
+                        @Suppress("invisible_member")
+                        RealmLog.error("Error thrown and ignored in `onManualResetFallback`: $ex")
+                    }
                 } else {
                     userErrorHandler.onError(session, syncError)
                 }

packages/library-sync/src/commonMain/kotlin/io/realm/kotlin/mongodb/internal/SyncedRealmContext.kt

@@ -32,26 +32,36 @@ import io.realm.kotlin.mongodb.sync.SyncSession
  * In order to work around the bootstrap problem, all public API entry points that access this
  * class must do so through the [executeInSyncContext] closure.
  */
-internal class SyncedRealmContext<T : BaseRealm>(realm: T) {
+@OptIn(ExperimentalStdlibApi::class)
+internal class SyncedRealmContext<T : BaseRealm>(realm: T) : AutoCloseable {
     // TODO For now this can only be a RealmImpl, which is required by the SyncSessionImpl
     //  When we introduce a public DynamicRealm, this can also be a `DynamicRealmImpl`
     //  And we probably need to modify the SyncSessionImpl to take either of these two.
     private val baseRealm = realm as RealmImpl
     internal val config: SyncConfiguration = baseRealm.configuration as SyncConfiguration
     // Note: Session and Subscriptions only need a valid dbPointer when being created, after that, they
     // have their own lifecycle and can be cached.
-    internal val session: SyncSession by lazy {
+    private val sessionDelegate: Lazy<SyncSessionImpl> = lazy {
         SyncSessionImpl(
             baseRealm,
             RealmInterop.realm_sync_session_get(baseRealm.realmReference.dbPointer)
         )
     }
-    internal val subscriptions: SubscriptionSet<T> by lazy {
+    internal val session: SyncSession by sessionDelegate
+
+    private val subscriptionsDelegate: Lazy<SubscriptionSetImpl<T>> = lazy {
         SubscriptionSetImpl(
             realm,
             RealmInterop.realm_sync_get_latest_subscriptionset(baseRealm.realmReference.dbPointer)
         )
     }
+    internal val subscriptions: SubscriptionSet<T> by subscriptionsDelegate
+
+    override fun close() {
+        if (sessionDelegate.isInitialized()) {
+            (session as SyncSessionImpl).nativePointer.release()
+        }
+    }
 }
 
 /**
@@ -77,6 +87,7 @@ internal fun <T, R : BaseRealm> executeInSyncContext(realm: R, block: (context:
     }
 }
 
+@OptIn(ExperimentalStdlibApi::class)
 private fun <T : BaseRealm> initSyncContextIfNeeded(realm: T): SyncedRealmContext<T> {
     // INVARIANT: `syncContext` is only ever set once, and never to `null`.
     // This code works around the fact that `Mutex`'s can only be locked inside suspend functions on

packages/test-base/src/androidMain/kotlin/io/realm/kotlin/test/platform/PlatformUtils.kt

@@ -21,7 +21,6 @@ import android.os.SystemClock
 import java.io.File
 import java.nio.file.Files
 import java.nio.file.Path
-import java.nio.file.attribute.PosixFilePermission
 import kotlin.io.path.absolutePathString
 import kotlin.time.Duration
 
@@ -30,14 +29,8 @@ actual object PlatformUtils {
     actual fun createTempDir(prefix: String, readOnly: Boolean): String {
         val dir: Path = Files.createTempDirectory("$prefix-android_tests")
         if (readOnly) {
-            Files.setPosixFilePermissions(
-                dir,
-                setOf(
-                    PosixFilePermission.GROUP_READ,
-                    PosixFilePermission.OTHERS_READ,
-                    PosixFilePermission.OWNER_READ
-                )
-            )
+            // Use the File API as it works across Windows and POSIX.
+            dir.toFile().setReadOnly()
         }
         return dir.absolutePathString()
     }

packages/test-sync/build.gradle.kts

@@ -286,27 +286,33 @@ kotlin {
 // - 'syncTestUrl` defines the root URL for the App Services server. Default is `http://localhost:9090`
 // - 'syncTestAppNamePrefix' is added a differentiator for all apps created by tests. This makes
 //   it possible for builds in parallel to run against the same test server. Default is `test-app`.
+fun getPropertyValue(propertyName: String): String? {
+    if (project.hasProperty(propertyName)) {
+        return project.property(propertyName) as String
+    }
+   return System.getenv(propertyName)
+}
 buildkonfig {
     packageName = "io.realm.kotlin.test.mongodb"
     objectName = "SyncServerConfig"
     defaultConfigs {
-        buildConfigField(Type.STRING, "url", properties["syncTestUrl"]!! as String)
-        buildConfigField(Type.STRING, "appPrefix", properties["syncTestAppNamePrefix"]!! as String)
-        if (properties.containsKey("syncTestLoginEmail") && properties.containsKey("syncTestLoginPassword")) {
-            buildConfigField(Type.STRING, "email", properties["syncTestLoginEmail"]!! as String)
-            buildConfigField(Type.STRING, "password", properties["syncTestLoginPassword"]!! as String)
+        buildConfigField(Type.STRING, "url", getPropertyValue("syncTestUrl"))
+        buildConfigField(Type.STRING, "appPrefix", getPropertyValue("syncTestAppNamePrefix"))
+        if (project.hasProperty("syncTestLoginEmail") && project.hasProperty("syncTestLoginPassword")) {
+            buildConfigField(Type.STRING, "email", getPropertyValue("syncTestLoginEmail"))
+            buildConfigField(Type.STRING, "password", getPropertyValue("syncTestLoginPassword"))
         } else {
             buildConfigField(Type.STRING, "email", "")
             buildConfigField(Type.STRING, "password", "")
         }
-        if (properties.containsKey("syncTestLoginPublicApiKey") && properties.containsKey("syncTestLoginPrivateApiKey")) {
-            buildConfigField(Type.STRING, "publicApiKey", properties["syncTestLoginPublicApiKey"]!! as String)
-            buildConfigField(Type.STRING, "privateApiKey", properties["syncTestLoginPrivateApiKey"]!! as String)
+        if (project.hasProperty("syncTestLoginPublicApiKey") && project.hasProperty("syncTestLoginPrivateApiKey")) {
+            buildConfigField(Type.STRING, "publicApiKey", getPropertyValue("syncTestLoginPublicApiKey"))
+            buildConfigField(Type.STRING, "privateApiKey", getPropertyValue("syncTestLoginPrivateApiKey"))
         } else {
             buildConfigField(Type.STRING, "publicApiKey", "")
             buildConfigField(Type.STRING, "privateApiKey", "")
         }
-        buildConfigField(Type.STRING, "clusterName", properties["syncTestClusterName"] as String? ?: "")
+        buildConfigField(Type.STRING, "clusterName", getPropertyValue("syncTestClusterName") ?: "")
     }
 }
 

packages/test-sync/src/commonTest/kotlin/io/realm/kotlin/test/mongodb/common/AppConfigurationTests.kt

@@ -18,6 +18,7 @@
 package io.realm.kotlin.test.mongodb.common
 
 import io.realm.kotlin.internal.platform.appFilesDirectory
+import io.realm.kotlin.internal.platform.isWindows
 import io.realm.kotlin.internal.platform.pathOf
 import io.realm.kotlin.internal.platform.runBlocking
 import io.realm.kotlin.log.LogLevel
@@ -151,9 +152,13 @@ class AppConfigurationTests {
 
     @Test
     fun syncRootDirectory_writeProtectedDir() {
-        val builder: AppConfiguration.Builder = AppConfiguration.Builder(APP_ID)
-        val dir = PlatformUtils.createTempDir(readOnly = true)
-        assertFailsWith<IllegalArgumentException> { builder.syncRootDirectory(dir) }
+        // It looks like we cannot create a readOnly directory on Windows, so ignore this test
+        // there for now.
+        if (!isWindows()) {
+            val builder: AppConfiguration.Builder = AppConfiguration.Builder(APP_ID)
+            val dir = PlatformUtils.createTempDir(readOnly = true)
+            assertFailsWith<IllegalArgumentException> { builder.syncRootDirectory(dir) }
+        }
     }
 
     // When creating the full path for a synced Realm, we will always append `/mongodb-realm` to

packages/test-sync/src/commonTest/kotlin/io/realm/kotlin/test/mongodb/common/FlexibleSyncIntegrationTests.kt

@@ -22,7 +22,6 @@ import io.realm.kotlin.entities.sync.flx.FlexEmbeddedObject
 import io.realm.kotlin.entities.sync.flx.FlexParentObject
 import io.realm.kotlin.ext.query
 import io.realm.kotlin.internal.platform.runBlocking
-import io.realm.kotlin.log.LogLevel
 import io.realm.kotlin.mongodb.exceptions.CompensatingWriteException
 import io.realm.kotlin.mongodb.exceptions.DownloadingRealmTimeOutException
 import io.realm.kotlin.mongodb.exceptions.SyncException
@@ -64,7 +63,7 @@ class FlexibleSyncIntegrationTests {
 
     @BeforeTest
     fun setup() {
-        app = TestApp(this::class.simpleName, appName = TEST_APP_FLEX, logLevel = LogLevel.ALL)
+        app = TestApp(this::class.simpleName, appName = TEST_APP_FLEX)
         val (email, password) = TestHelper.randomEmail() to "password1234"
         runBlocking {
             app.createUserAndLogIn(email, password)