Process safe deleteRealm and assetFile

- 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.
realm · Oct 13, 2017 · ed30b60 · ed30b60
1 parent 2cfc1ec
commit ed30b60
Show file tree

Hide file tree

Showing 18 changed files with 230 additions and 93 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,15 +2,19 @@
 
 ## Breaking Changes
 
+* `Realm.deleteRealm()` doesn't support synced Realm for now. See issue #5416.
+
 ## Enhancements
 
 * All Realm annotations are now kept at runtime, allowing runtime tools access to them (#5344).
 * Speedup schema initialization when a Realm file is first accessed (#5391).
+* `Realm.deleteRealm()` and `RealmConfiguration.assetFile()` are multi-processes safe now.
 
 ## 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.
 
 ## Internal
 

diff --git a/realm/realm-library/src/androidTest/java/io/realm/RealmTests.java b/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.
-        assertEquals(0, tempDir.listFiles().length);
+        assertEquals(1, tempDir.listFiles().length);
+
+        // Lock file should never be deleted
+        File lockFile = new File(configuration.getPath() + ".lock");
+        assertTrue(lockFile.exists());
     }
 
     // Tests that all methods that require a transaction. (ie. any function that mutates Realm data)

diff --git a/realm/realm-library/src/androidTest/java/io/realm/internal/CollectionTests.java b/realm/realm-library/src/androidTest/java/io/realm/internal/CollectionTests.java
@@ -57,15 +57,13 @@ public class CollectionTests {
 
     private final long[] oneNullTable = new long[] {NativeObject.NULLPTR};
 
-    private RealmConfiguration config;
     private SharedRealm sharedRealm;
     private Table table;
 
     @Before
     public void setUp() {
-        config = configFactory.createConfiguration();
         sharedRealm = getSharedRealm();
-        populateData();
+        populateData(sharedRealm);
     }
 
     @After
@@ -74,6 +72,18 @@ public void tearDown() {
     }
 
     private SharedRealm getSharedRealm() {
+        RealmConfiguration config;
+        config = configFactory.createConfiguration();
+        return getSharedRealm(config);
+    }
+
+    private SharedRealm getSharedRealmForLooper() {
+        RealmConfiguration config;
+        config = looperThread.createConfiguration();
+        return getSharedRealm(config);
+    }
+
+    private SharedRealm getSharedRealm(RealmConfiguration config) {
         OsRealmConfig.Builder configBuilder = new OsRealmConfig.Builder(config)
                 .autoUpdateNotification(true);
         SharedRealm sharedRealm = SharedRealm.getInstance(configBuilder);
@@ -87,7 +97,7 @@ private Table getTable(SharedRealm sharedRealm) {
         return sharedRealm.getTable(Table.getTableNameForClass("test_table"));
     }
 
-    private void populateData() {
+    private void populateData(SharedRealm sharedRealm) {
         sharedRealm.beginTransaction();
         table = sharedRealm.createTable(Table.getTableNameForClass("test_table"));
         // Specify the column types and names
@@ -119,12 +129,13 @@ private void populateData() {
         sharedRealm.commitTransaction();
     }
 
-    private void addRowAsync() {
+    private void addRowAsync(final SharedRealm sharedRealm) {
         final CountDownLatch latch = new CountDownLatch(1);
+        final RealmConfiguration configuration = sharedRealm.getConfiguration();
         new Thread(new Runnable() {
             @Override
             public void run() {
-                SharedRealm sharedRealm = getSharedRealm();
+                SharedRealm sharedRealm = getSharedRealm(configuration);
                 addRow(sharedRealm);
                 sharedRealm.close();
                 latch.countDown();
@@ -253,7 +264,8 @@ public void distinct() {
     @Test
     @RunTestInLooperThread
     public void addListener_shouldBeCalledToReturnTheQueryResults() {
-        final SharedRealm sharedRealm = getSharedRealm();
+        final SharedRealm sharedRealm = getSharedRealmForLooper();
+        populateData(sharedRealm);
         Table table = getTable(sharedRealm);
 
         final Collection collection = new Collection(sharedRealm, table.where());
@@ -332,7 +344,7 @@ public void onChange(Collection element) {
             }
         });
 
-        addRowAsync();
+        addRowAsync(sharedRealm);
 
         sharedRealm.waitForChange();
         sharedRealm.refresh();
@@ -342,7 +354,8 @@ public void onChange(Collection element) {
     @Test
     @RunTestInLooperThread
     public void addListener_queryNotReturned() {
-        final SharedRealm sharedRealm = getSharedRealm();
+        final SharedRealm sharedRealm = getSharedRealmForLooper();
+        populateData(sharedRealm);
         Table table = getTable(sharedRealm);
 
         final Collection collection = new Collection(sharedRealm, table.where());
@@ -357,13 +370,14 @@ public void onChange(Collection collection1) {
             }
         });
 
-        addRowAsync();
+        addRowAsync(sharedRealm);
     }
 
     @Test
     @RunTestInLooperThread
     public void addListener_queryReturned() {
-        final SharedRealm sharedRealm = getSharedRealm();
+        final SharedRealm sharedRealm = getSharedRealmForLooper();
+        populateData(sharedRealm);
         Table table = getTable(sharedRealm);
 
         final Collection collection = new Collection(sharedRealm, table.where());
@@ -379,15 +393,16 @@ public void onChange(Collection collection1) {
             }
         });
 
-        addRowAsync();
+        addRowAsync(sharedRealm);
     }
 
     // Local commit will trigger the listener first when beginTransaction gets called then again when transaction
     // committed.
     @Test
     @RunTestInLooperThread
     public void addListener_triggeredByLocalCommit() {
-        final SharedRealm sharedRealm = getSharedRealm();
+        final SharedRealm sharedRealm = getSharedRealmForLooper();
+        populateData(sharedRealm);
         Table table = getTable(sharedRealm);
         final AtomicInteger listenerCounter = new AtomicInteger(0);
 
@@ -468,7 +483,8 @@ public void collectionIterator_invalid_nonLooperThread_byRefresh() {
     @Test
     @RunTestInLooperThread
     public void collectionIterator_invalid_looperThread_byRemoteTransaction() {
-        final SharedRealm sharedRealm = getSharedRealm();
+        final SharedRealm sharedRealm = getSharedRealmForLooper();
+        populateData(sharedRealm);
         Table table = getTable(sharedRealm);
         final Collection collection = new Collection(sharedRealm, table.where());
         final TestIterator iterator = new TestIterator(collection);
@@ -487,7 +503,7 @@ public void onChange(Collection element) {
             }
         });
 
-        addRowAsync();
+        addRowAsync(sharedRealm);
     }
 
     @Test
@@ -522,6 +538,9 @@ public void onChange(Collection element) {
     @Test
     @RunTestInLooperThread
     public void load() {
+        final SharedRealm sharedRealm = getSharedRealmForLooper();
+        looperThread.closeAfterTest(sharedRealm);
+        populateData(sharedRealm);
         final Collection collection = new Collection(sharedRealm, table.where());
         collection.addListener(collection, new RealmChangeListener<Collection>() {
             @Override

diff --git a/realm/realm-library/src/androidTest/java/io/realm/internal/JNIColumnInfoTest.java b/realm/realm-library/src/androidTest/java/io/realm/internal/JNIColumnInfoTest.java
@@ -19,6 +19,7 @@
 import android.support.test.InstrumentationRegistry;
 import android.support.test.runner.AndroidJUnit4;
 
+import org.junit.After;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
@@ -39,16 +40,13 @@ public class JNIColumnInfoTest {
     @Rule
     public final TestRealmConfigurationFactory configFactory = new TestRealmConfigurationFactory();
 
-    @SuppressWarnings("FieldCanBeLocal")
-    private RealmConfiguration config;
-    @SuppressWarnings("FieldCanBeLocal")
     private SharedRealm sharedRealm;
     private Table table;
 
     @Before
     public void setUp() {
         Realm.init(InstrumentationRegistry.getInstrumentation().getContext());
-        config = configFactory.createConfiguration();
+        RealmConfiguration config = configFactory.createConfiguration();
         sharedRealm = SharedRealm.getInstance(config);
 
         table = TestHelper.createTable(sharedRealm, "temp", new TestHelper.AdditionalTableSetup() {
@@ -60,6 +58,13 @@ public void execute(Table table) {
         });
     }
 
+    @After
+    public void tearDown() {
+        if (sharedRealm != null) {
+            sharedRealm.close();
+        }
+    }
+
     @Test
     public void shouldGetColumnInformation() {
 

diff --git a/realm/realm-library/src/androidTest/java/io/realm/internal/JNITableTest.java b/realm/realm-library/src/androidTest/java/io/realm/internal/JNITableTest.java
@@ -19,6 +19,7 @@
 import android.support.test.runner.AndroidJUnit4;
 import android.util.Pair;
 
+import org.junit.After;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
@@ -60,6 +61,13 @@ public void setUp() {
         sharedRealm = SharedRealm.getInstance(config);
     }
 
+    @After
+    public void tearDown() {
+        if (sharedRealm != null)  {
+            sharedRealm.close();
+        }
+    }
+
     @Test
     public void tableToString() {
         Table t = TestHelper.createTable(sharedRealm, "temp", new TestHelper.AdditionalTableSetup() {
@@ -307,10 +315,6 @@ public void execute(Table t) {
     @Test
     public void getName() {
         String TABLE_NAME = "tableName";
-        RealmConfiguration configuration = configFactory.createConfiguration();
-        Realm.deleteRealm(configuration);
-
-        SharedRealm sharedRealm = SharedRealm.getInstance(configuration);
         //noinspection TryFinallyCanBeTryWithResources
         try {
 

diff --git a/realm/realm-library/src/androidTest/java/io/realm/rule/TestRealmConfigurationFactory.java b/realm/realm-library/src/androidTest/java/io/realm/rule/TestRealmConfigurationFactory.java
@@ -28,6 +28,7 @@
 import java.io.IOException;
 import java.io.InputStream;
 import java.util.Collections;
+import java.util.Locale;
 import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
@@ -163,8 +164,9 @@ public void copyRealmFromAssets(Context context, String realmPath, String newNam
     }
 
     public void copyRealmFromAssets(Context context, String realmPath, RealmConfiguration config) throws IOException {
-        // Deletes the existing file before copy
-        Realm.deleteRealm(config);
+        if (new File(config.getPath()).exists()) {
+            throw new IllegalStateException(String.format(Locale.ENGLISH, "%s exists!", config.getPath()));
+        }
 
         File outFile = new File(config.getRealmDirectory(), config.getRealmFileName());
 

diff --git a/realm/realm-library/src/main/cpp/io_realm_internal_OsObjectStore.cpp b/realm/realm-library/src/main/cpp/io_realm_internal_OsObjectStore.cpp
@@ -20,6 +20,7 @@
 #include <shared_realm.hpp>
 
 #include "util.hpp"
+#include "jni_util/java_method.hpp"
 #include "jni_util/java_exception_thrower.hpp"
 #include "jni_util/java_exception_thrower.hpp"
 
@@ -140,3 +141,25 @@ JNIEXPORT jboolean JNICALL Java_io_realm_internal_OsObjectStore_nativeDeleteTabl
     CATCH_STD()
     return JNI_FALSE;
 }
+
+JNIEXPORT jboolean JNICALL Java_io_realm_internal_OsObjectStore_nativeCallWithLock(JNIEnv* env, jclass,
+                                                                                   jstring j_realm_path,
+                                                                                   jobject j_runnable)
+{
+    TR_ENTER();
+    try {
+        JStringAccessor path_accessor(env, j_realm_path);
+        std::string realm_path(path_accessor);
+        static JavaClass runnable_class(env, "java/lang/Runnable");
+        static JavaMethod run_method(env, runnable_class, "run", "()V");
+        bool exception_thrown = false;
+        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();
+        });
+        return exception_thrown ? false : result;
+    }
+    CATCH_STD()
+    return false;
+}
diff --git a/realm/realm-library/src/main/java/io/realm/BaseRealm.java b/realm/realm-library/src/main/java/io/realm/BaseRealm.java
@@ -35,6 +35,7 @@
 import io.realm.internal.CheckedRow;
 import io.realm.internal.ColumnInfo;
 import io.realm.internal.InvalidRow;
+import io.realm.internal.ObjectServerFacade;
 import io.realm.internal.OsObjectStore;
 import io.realm.internal.OsRealmConfig;
 import io.realm.internal.OsSchemaInfo;
@@ -609,20 +610,20 @@ public void deleteAll() {
      */
     static boolean deleteRealm(final RealmConfiguration configuration) {
         final AtomicBoolean realmDeleted = new AtomicBoolean(true);
-        RealmCache.invokeWithGlobalRefCount(configuration, new RealmCache.Callback() {
+        boolean callbackExecuted = OsObjectStore.callWithLock(configuration, new Runnable() {
             @Override
-            public void onResult(int count) {
-                if (count != 0) {
-                    throw new IllegalStateException("It's not allowed to delete the file associated with an open Realm. " +
-                            "Remember to close() all the instances of the Realm before deleting its file: " + configuration.getPath());
-                }
-
+            public void run() {
                 String canonicalPath = configuration.getPath();
                 File realmFolder = configuration.getRealmDirectory();
                 String realmFileName = configuration.getRealmFileName();
                 realmDeleted.set(Util.deleteRealm(canonicalPath, realmFolder, realmFileName));
             }
         });
+        if (!callbackExecuted) {
+            throw new IllegalStateException("It's not allowed to delete the file associated with an open Realm. " +
+                    "Remember to close() all the instances of the Realm before deleting its file: "
+                    + configuration.getPath());
+        }
         return realmDeleted.get();
     }
 

diff --git a/realm/realm-library/src/main/java/io/realm/Realm.java b/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.
+     * <p>
      * All Realm instances must be closed before calling this method.
+     * <p>
+     * This doesn't support deleting synced Realm for now.
      *
      * @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
+     * the return value. All of the failing file will be logged.
      * @throws IllegalStateException if not all realm instances are closed.
+     * @throws IllegalStateException if the {@code configuration} is a {@link SyncConfiguration}.
      */
     public static boolean deleteRealm(RealmConfiguration configuration) {
+        // FIXME: We don't know when the Realm instance on the sync client thread will be closed. Disable it for now.
+        if (configuration.isSyncConfiguration()) {
+            throw new IllegalStateException("'deleteRealm() is not supported for sync for now.");
+        }
         return BaseRealm.deleteRealm(configuration);
     }