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

CHANGELOG.md

@@ -7,8 +7,14 @@
 * 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
+* `Realm.close()` is now idempotent.
 * Fix error in `RealmAny.equals` that would sometimes return `true` when comparing RealmAnys wrapping same type but different values. (Issue [#1523](https://github.com/realm/realm-kotlin/pull/1523))
 * [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))
+* [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] `ClientResetRequiredException.executeClientReset()` now returns a boolean indicating if the manual reset fully succeded or not. (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))
+* [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
 * File format: Generates Realms with file format v23.

dependencies.list

@@ -1,11 +1,11 @@
 # Version of MongoDB Realm used by integration tests
 # See https://github.com/realm/ci/packages/147854 for available versions
-MONGODB_REALM_SERVER=2023-09-07
+MONGODB_REALM_SERVER=2023-09-22
 
 # `BAAS` and `BAAS-UI` projects commit hashes matching MONGODB_REALM_SERVER image version
 # note that the MONGODB_REALM_SERVER image is a nightly build, find the matching commits
 # for that date within the following repositories:
 # https://github.com/10gen/baas/
 # https://github.com/10gen/baas-ui/
-REALM_BAAS_GIT_HASH=fc070ea7e874f58eb5b4f7a9d344fa7cdd755817
-REALM_BAAS_UI_GIT_HASH=ca722f5bb7a7485404a68874991ba8e50787ea9a
+REALM_BAAS_GIT_HASH=0b7562d0401d72c909369030dc29332542614ba3
+REALM_BAAS_UI_GIT_HASH=24baee4eb0e9736969a00a7bfac849565bca17f4

packages/cinterop/src/nativeDarwin/kotlin/io/realm/kotlin/internal/interop/RealmInterop.kt

@@ -2544,6 +2544,8 @@ actual object RealmInterop {
                 } catch (e: Throwable) {
                     println(e.message)
                     false
+                } finally {
+                    realm_wrapper.realm_close(afterRealmPtr)
                 }
             },
             StableRef.create(afterHandler).asCPointer(),

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

@@ -46,7 +46,6 @@ import kotlinx.coroutines.flow.Flow
 import kotlinx.coroutines.flow.MutableSharedFlow
 import kotlinx.coroutines.flow.onStart
 import kotlinx.coroutines.launch
-import kotlinx.coroutines.sync.Mutex
 import kotlinx.coroutines.sync.withLock
 import kotlin.reflect.KClass
 
@@ -56,8 +55,6 @@ public class RealmImpl private constructor(
     configuration: InternalConfiguration,
 ) : BaseRealmImpl(configuration), Realm, InternalTypedRealm, Flowable<RealmChange<Realm>> {
 
-    private val realmPointerMutex = Mutex()
-
     public val notificationScheduler: LiveRealmContext =
         configuration.notificationDispatcherFactory.createLiveRealmContext()
 
@@ -85,6 +82,7 @@ public class RealmImpl private constructor(
 
     private var _realmReference: AtomicRef<FrozenRealmReference?> = atomic(null)
     private val realmReferenceLock = SynchronizableObject()
+    private val isClosed = atomic(false)
 
     /**
      * The current Realm reference that points to the underlying frozen C++ SharedRealm.
@@ -103,7 +101,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")
@@ -261,27 +260,39 @@ public class RealmImpl private constructor(
         return VersionInfo(mainVersions, notifier.versions(), writer.versions())
     }
 
+    override fun isClosed(): Boolean {
+        // We cannot rely on `realmReference()` here. If something happens during open, this might
+        // not be available and will throw, so we need to track closed state separately.
+        return isClosed.value
+    }
+
     override fun close() {
         // TODO Reconsider this constraint. We have the primitives to check is we are on the
         //  writer thread and just close the realm in writer.close()
         writer.checkInTransaction("Cannot close the Realm while inside a transaction block")
-        runBlocking {
-            realmPointerMutex.withLock {
+        realmReferenceLock.withLock {
+            if (isClosed()) {
+                return
+            }
+            isClosed.value = true
+            runBlocking {
                 writer.close()
                 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()
             }
-        }
-        if (!realmStateFlow.tryEmit(State.CLOSED)) {
-            log.warn("Cannot signal internal close")
-        }
+            if (!realmStateFlow.tryEmit(State.CLOSED)) {
+                log.warn("Cannot signal internal close")
+            }
 
-        notificationScheduler.close()
-        writeScheduler.close()
+            notificationScheduler.close()
+            writeScheduler.close()
+        }
     }
 
     internal companion object {

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/SyncSessionImpl.kt

@@ -247,6 +247,10 @@ internal open class SyncSessionImpl(
         }
     }
 
+    fun close() {
+        nativePointer.release()
+    }
+
     internal companion object {
         internal fun stateFrom(coreState: CoreSyncSessionState): SyncSession.State {
             return when (coreState) {

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).close()
+        }
+    }
 }
 
 /**
@@ -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-base/src/commonTest/kotlin/io/realm/kotlin/test/common/RealmTests.kt

@@ -429,6 +429,14 @@ class RealmTests {
             }
     }
 
+    @Test
+    fun close_idempotent() {
+        realm.close()
+        assertTrue(realm.isClosed())
+        realm.close()
+        assertTrue(realm.isClosed())
+    }
+
     @Test
     @Suppress("LongMethod")
     fun deleteRealm() {

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/AsymmetricSyncTests.kt

@@ -104,12 +104,7 @@ class AsymmetricSyncTests {
         }
         config = SyncConfiguration.Builder(
             user,
-            schema = setOf(
-                Measurement::class,
-                Device::class,
-                BackupDevice::class,
-                DeviceParent::class
-            )
+            schema = FLX_SYNC_SCHEMA
         ).initialSubscriptions {
             it.query<DeviceParent>().subscribe()
         }.build()
@@ -299,11 +294,7 @@ class AsymmetricSyncTests {
     fun asymmetricSchema() = runBlocking {
         config = SyncConfiguration.Builder(
             app.login(Credentials.anonymous()),
-            schema = setOf(
-                AsymmetricA::class,
-                EmbeddedB::class,
-                StandardC::class,
-            )
+            schema = FLX_SYNC_SCHEMA
         ).build()
         Realm.open(config).use {
             it.write {