Process safe deleteRealm and assetFile #5417

beeender · 2017-10-13T08:54:22Z

Use SharedRealm::call_with_lock() to make sure deleteRealm and
assetFile calls are process safe.
Also those calls will throw if the Realm instance on the sync client
thread is still opened.
Disallow deleteRealm on a synced Realm file. See Need a way to ensure the Realm instance on the sync client thread has been closed #5416 .
Manually delete SharedRealm pointer if the SharedRealm was created for
callbacks, eg.: initialization_data_callback and migration_callback.

- Use SharedRealm::call_with_lock() to make sure deleteRealm and assetFile calls are process safe. - Also those calls will throw if the Realm instance on the sync client thread is still opened. - Disallow deleteRealm on a synced Realm file. See #5416 . - Manually delete SharedRealm pointer if the SharedRealm was created for callbacks, eg.: initialization_data_callback and migration_callback.

Zhuinden · 2017-10-13T09:12:12Z

CHANGELOG.md


 ## Bug Fixes

 * Assigning a managed object's own list to itself would accidentally clear it (#5395).
 * Don't try to acquire `ApplicationContext` if not available in `Realm.init(Context)` (#5389).
+* Fix some potential database corruption caused by deleting realm file while a Realm instance are still opened on the other process or sync client thread.


in another process?

nhachicha · 2017-10-13T09:58:13Z

CHANGELOG.md


 ## Bug Fixes

 * Assigning a managed object's own list to itself would accidentally clear it (#5395).
 * Don't try to acquire `ApplicationContext` if not available in `Realm.init(Context)` (#5389).
+* Fix some potential database corruption caused by deleting realm file while a Realm instance are still opened on the other process or sync client thread.


is still open in another process

nhachicha · 2017-10-13T10:01:58Z

realm/realm-library/src/androidTest/java/io/realm/internal/CollectionTests.java

@@ -74,6 +72,18 @@ public void tearDown() {
    }

    private SharedRealm getSharedRealm() {
+        RealmConfiguration config;
+        config = configFactory.createConfiguration();


why not RealmConfiguration config = configFactory.createConfiguration(); ?

nhachicha · 2017-10-13T10:02:09Z

realm/realm-library/src/androidTest/java/io/realm/internal/CollectionTests.java

+
+    private SharedRealm getSharedRealmForLooper() {
+        RealmConfiguration config;
+        config = looperThread.createConfiguration();


nhachicha · 2017-10-13T10:11:03Z

realm/realm-library/src/main/java/io/realm/Realm.java

@@ -1657,14 +1657,24 @@ public static void migrateRealm(RealmConfiguration configuration, @Nullable Real
    }

    /**
-     * Deletes the Realm file specified by the given {@link RealmConfiguration} from the filesystem.
+     * Deletes the Realm file along with the related temporary files specified by the given {@link RealmConfiguration}
+     * from the filesystem. Temporary file with ".lock" extension won't be deleted.


is Core responsible for deleting the lock file? and when?

no, lock file should never be deleted. There is no reliable way to delete it for multi processes case.

nhachicha · 2017-10-13T10:15:41Z

realm/realm-library/src/main/java/io/realm/RealmCache.java

-        if (!Util.isEmptyString(syncServerCertificateAssetName)) {
-            String syncServerCertificateFilePath = ObjectServerFacade.getFacade(configuration.isSyncConfiguration()).getSyncServerCertificateFilePath(configuration);
+                    // Copy Sync Server certificate path if available
+                    if (!Util.isEmptyString(syncServerCertificateAssetName)) {


use a final boolean in https://github.com/realm/realm-java/pull/5417/files#diff-79e6ca3beee0a83dd76772285b606694R504 to the redundant check again

nhachicha · 2017-10-13T10:22:27Z

realm/realm-library/src/main/java/io/realm/internal/OsObjectStore.java

+     * @param runnable to run with lock.
+     * @return true if the lock can be acquired and the {@code runnable} has been executed.
+     */
+    public static boolean callWithLock(RealmConfiguration configuration, Runnable runnable) {


What happens in case of an exception (within the Runnable) is the lock released? How are we reporting unchecked exception?

yes, the lock will be released.
I planned to make a test case for this but forgot it finally ... will add a test case.

nhachicha · 2017-10-13T10:27:35Z

realm/realm-library/src/main/java/io/realm/internal/SharedRealm.java

     */
    private SharedRealm(long nativeSharedRealmPtr, OsRealmConfig osRealmConfig) {
        this.nativePtr = nativeSharedRealmPtr;
        this.osRealmConfig = osRealmConfig;
        this.schemaInfo = new OsSchemaInfo(nativeGetSchemaInfo(nativePtr), this);
        this.context = osRealmConfig.getContext();
-        this.context.addReference(this);


does it mean we shouldn't this.context.addReference(this); when isPhantomRef is true?

what's the purpose of keeping the NativeObject interface if we don't add the reference(addReference)

See https://github.com/realm/realm-java/pull/5417/files#diff-6e1f09495610b341d4f834a4e13002ccR199

In normal case it will still be managed by phantom reference.
This constructor is a special case and only handled in this file.

*isPhantomRef is no longer used? ( you removed the delete method) ...?

I removed both in the yesterday's change.
So the pointer itself will be managed like a normal SharedRealm pointer. So there is no chance for users to meet a invalid pointer anymore. (before yeterday's change, there was still a problem.)

See commit ed305d2

nhachicha · 2017-10-13T10:30:15Z

realm/realm-library/src/main/java/io/realm/internal/Util.java

-                realmDeleted = realmDeleted && file.delete();
+                boolean deleteResult = file.delete();
+                if (!deleteResult) {
+                    RealmLog.warn( String.format(Locale.ENGLISH,"Realm temporary file at %s cannot deleted",


cannot be deleted

nhachicha · 2017-10-13T10:30:42Z

realm/realm-library/src/main/java/io/realm/internal/Util.java

+            realmDeleted = realmFile.delete();
+            if (!realmDeleted) {
+                RealmLog.warn(
+                        String.format(Locale.ENGLISH,"Realm file at %s cannot deleted", realmFile.getAbsolutePath()));


cannot be deleted

nhachicha · 2017-10-13T10:32:01Z

realm/realm-library/src/syncIntegrationTest/java/io/realm/SSLConfigurationTests.java


        // 2. Local state should now be completely reset. Open the Realm again with a new configuration which should
        // download the uploaded changes.
        user = SyncUser.login(SyncCredentials.usernamePassword(username, password), Constants.AUTH_URL);
        SyncConfiguration config = configurationFactory.createSyncConfigurationBuilder(user, Constants.USER_REALM_SECURE)
+                .name("newRealm")


These tests are disabled for now, https://github.com/realm/ros/issues/512 any reason to add the name?

because of we sadly don't have a reliable way to know when the sync client thread will close the Realm instance.

https://github.com/realm/realm-java/pull/5417/files#diff-c4f8c4a7fed4b6347565a6e587f5da61L68

cmelchior

Apart from minor comments 👍

cmelchior · 2017-10-13T11:09:04Z

CHANGELOG.md

@@ -2,15 +2,19 @@

 ## Breaking Changes

+* `Realm.deleteRealm()` doesn't support synced Realm for now. See issue #5416.


Realm -> Realms

cmelchior · 2017-10-13T11:09:44Z

CHANGELOG.md


 ## Bug Fixes

 * Assigning a managed object's own list to itself would accidentally clear it (#5395).
 * Don't try to acquire `ApplicationContext` if not available in `Realm.init(Context)` (#5389).
+* Fix some potential database corruption caused by deleting realm file while a Realm instance are still opened in another process or the sync client thread.


deleting realm file -> deleting the Realm file

cmelchior · 2017-10-13T11:10:28Z

realm/realm-library/src/androidTest/java/io/realm/RealmTests.java

@@ -2302,7 +2302,11 @@ public void run() {
        assertTrue(Realm.deleteRealm(configuration));

        // Directory should be empty now.


That note doesn't seem correct given that the length is 0. I assume it is the .lock file. Clarify this or just move the comment below up instead of this one?

cmelchior · 2017-10-13T11:18:00Z

realm/realm-library/src/main/cpp/io_realm_internal_OsObjectStore.cpp

+        bool result = SharedGroup::call_with_lock(realm_path, [&](std::string path) {
+            REALM_ASSERT_RELEASE_EX(realm_path.compare(path) == 0, realm_path.c_str(), path.c_str());
+            env->CallVoidMethod(j_runnable, run_method);
+            exception_thrown = env->ExceptionCheck();


This callback is made on the same thread right, so using the env should be safe here?

yes, it is a synchronized callback. It will return before the outer function returns.

cmelchior · 2017-10-13T11:19:50Z

realm/realm-library/src/main/java/io/realm/Realm.java

     *
     * @param configuration a {@link RealmConfiguration}.
-     * @return {@code false} if a file could not be deleted. The failing file will be logged.
+     * @return {@code false} if the realm file could not be deleted. Temporary files deletion failure won't impact on


remove on

realm -> Realm

cmelchior · 2017-10-13T11:22:27Z

realm/realm-library/src/main/java/io/realm/internal/OsObjectStore.java

@@ -71,6 +73,19 @@ public static boolean deleteTableForObject(SharedRealm sharedRealm, String class
        return nativeDeleteTableForObject(sharedRealm.getNativePtr(), className);
    }

+    /**
+     * Try to grab a exclusive lock on the given Realm file. If the lock can be acquired, the {@code runnable} will


cmelchior · 2017-10-13T11:22:46Z

realm/realm-library/src/main/java/io/realm/internal/OsObjectStore.java

@@ -71,6 +73,19 @@ public static boolean deleteTableForObject(SharedRealm sharedRealm, String class
        return nativeDeleteTableForObject(sharedRealm.getNativePtr(), className);
    }

+    /**
+     * Try to grab a exclusive lock on the given Realm file. If the lock can be acquired, the {@code runnable} will
+     * executed while the lock is hold. The lock will ensure no one else can read or write the Realm file at the same


to the Realm file

cmelchior · 2017-10-13T11:24:12Z

realm/realm-library/src/main/java/io/realm/internal/SharedRealm.java

@@ -196,6 +197,7 @@ private SharedRealm(OsRealmConfig osRealmConfig) {
        this.schemaInfo = new OsSchemaInfo(nativeGetSchemaInfo(nativePtr), this);
        this.context = osRealmConfig.getContext();
        this.context.addReference(this);
+        this.isPhantomRef = true;


Would isManagedByPhantomRef be more clear? Right now it feels a bit unclear to me

cmelchior · 2017-10-13T11:25:49Z

realm/realm-library/src/main/java/io/realm/internal/SharedRealm.java

@@ -482,8 +499,13 @@ private void executePendingRowQueries() {
    @SuppressWarnings("unused")
    private static void runMigrationCallback(long nativeSharedRealmPtr, OsRealmConfig osRealmConfig, MigrationCallback callback,
                                             long oldVersion) {
-        callback.onMigrationNeeded(new SharedRealm(nativeSharedRealmPtr, osRealmConfig), oldVersion,
-                osRealmConfig.getRealmConfiguration().getSchemaVersion());
+        SharedRealm sharedRealm = new SharedRealm(nativeSharedRealmPtr, osRealmConfig);


Not sure I understand the reason for making this change?

complex story.
take the migration for example, ObjectStore will pass a SharedRealm instance to the callback
to make the life control easier, i created a new pointer from that instance and pass it to java, so it can be managed like a normal SharedRealm, user will never meet a invalid pointer of SharedRealm even when he passed that SharedRealm instance out.

But then it is a problem when exception happens in migration block. after exception thrown, getInstance() hasn't return anything yet, so user doesn't have to call close(). But there is a SharedRealm pointer created in the migration callback which is waiting for the GC to be deleted. That means, from OS's RealmCoordinator pointer of view, there is still one SharedRealm is not dealloced. -- call deleteRealm() at this time will throw.

But i think i still miss one thing in this modification, SharedRealm.isClosed() should return true after delete() called to avoid use pass the Realm/DyanmicRealm instance out of the callback and see a deleted pointer.

cmelchior · 2017-10-13T11:26:20Z

realm/realm-library/src/main/java/io/realm/internal/Util.java

+     * To delete Realm and related temporary files. This must be called in
+     * {@link OsObjectStore#callWithLock(RealmConfiguration, Runnable)}'s callback.
+     *
+     * @return {@code true} if the realm file is deleted. Temporary file deletion failure will not impact on return


on return -> the return

Also, it will impact the return value right? false is returned if they cannot be deleted?

no, it will always return true if the realm file has been deleted.

I think it is just good enough to ensure the realm file is deleted since they take most of disk spaces.

Also, for the Realm on external storage, the temporary files are not deleted since we don't know where are they. (the logic of this part is decided by core at runtiem :( where all the fifo permission nightmare come from)

Since it is not necessary for doing that. The newly created SharedRealm should be immediately deleted before the callback returns. Set the BaseRealm.sharedRealm to null before callback returns.

…ete-realm

So the Java OsSharedRealm instance will still never hold a invalid SharedRealm pointer.

…ete-realm

nhachicha · 2017-10-17T09:43:27Z

realm/realm-library/src/androidTest/java/io/realm/rule/TestRealmConfigurationFactory.java

+    @Override
+    public void create() throws IOException {
+        super.create();
+        tempFolder = new File(super.getRoot(), testName);


nice move 👍

nhachicha · 2017-10-17T09:46:09Z

realm/realm-library/src/main/cpp/io_realm_internal_OsRealmConfig.cpp

+                // Close the SharedRealm. Otherwise it will only be closed when the Java OsSharedRealm gets GCed. And
+                // that will be too late.
+                TERMINATE_JNI_IF_JAVA_EXCEPTION_OCCURRED(
+                    env, [&new_shared_realm_ptr]() { (*new_shared_realm_ptr)->close(); });


do we need to delete new_shared_realm_ptr as well?

nope~ it will be still handled by the GC and phantom refs.

nhachicha · 2017-10-17T09:49:17Z

realm/realm-library/src/main/cpp/io_realm_internal_OsRealmConfig.cpp

+                // Close the SharedRealm. Otherwise it will only be closed when the Java OsSharedRealm gets GCed. And
+                // that will be too late.
+                TERMINATE_JNI_IF_JAVA_EXCEPTION_OCCURRED(
+                    env, [&new_shared_realm_ptr]() { (*new_shared_realm_ptr)->close(); });


And fix a flaky test.

realm-ci assigned beeender Oct 13, 2017

beeender added the T-Enhancement label Oct 13, 2017

Zhuinden reviewed Oct 13, 2017

View reviewed changes

Rephrase the changelog

2b4cf32

nhachicha reviewed Oct 13, 2017

View reviewed changes

cmelchior approved these changes Oct 13, 2017

View reviewed changes

beeender added 7 commits October 13, 2017 20:26

PR fix

6e8ebcb

Don't alloc new SharedRealm for callbacks

ed305d2

Since it is not necessary for doing that. The newly created SharedRealm should be immediately deleted before the callback returns. Set the BaseRealm.sharedRealm to null before callback returns.

Merge remote-tracking branch 'origin/master' into mc/process-safe-del…

1de0047

…ete-realm

Add the test name to the temp Realm path

127fbf2

Merge remote-tracking branch 'origin/master' into mc/process-safe-del…

e5d47d1

…ete-realm

Close realm if exception happened in the callback

6241231

So the Java OsSharedRealm instance will still never hold a invalid SharedRealm pointer.

Merge remote-tracking branch 'origin/master' into mc/process-safe-del…

f391359

…ete-realm

nhachicha reviewed Oct 17, 2017

View reviewed changes

nhachicha approved these changes Oct 17, 2017

View reviewed changes

beeender added 3 commits October 17, 2017 18:00

Use TERMINATE_JNI_IF_JAVA_EXCEPTION_OCCURRED

a770f2a

Enable deleteRealm for sync

5718d9b

Temp Realm path for looper thread

be525b7

And fix a flaky test.

beeender merged commit f80f06d into master Oct 17, 2017

beeender deleted the mc/process-safe-delete-realm branch October 17, 2017 13:54

github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024

		@@ -2,15 +2,19 @@

		## Breaking Changes

		* `Realm.deleteRealm()` doesn't support synced Realm for now. See issue #5416.

		@@ -2302,7 +2302,11 @@ public void run() {
		assertTrue(Realm.deleteRealm(configuration));

		// Directory should be empty now.

Process safe deleteRealm and assetFile #5417

Process safe deleteRealm and assetFile #5417

Conversation

beeender commented Oct 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmelchior left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment