Merge branch '282-always-destroy-write-tx' into 'dev'

Transaction: close write transactions even if store is closing #282

See merge request objectbox/objectbox-java!166

Author: greenrobot-team

CHANGELOG.md

@@ -5,7 +5,10 @@ Notable changes to the ObjectBox Java library.
 For more insights into what changed in the ObjectBox C++ core, [check the ObjectBox C changelog](https://github.com/objectbox/objectbox-c/blob/main/CHANGELOG.md).
 
 ## 5.0.1 - in development
-                     
+
+- Fixed a race condition with a closing store and still active transactions that kept the store from closing.
+  For Android this may fix some rare ANR issues.
+
 ### Sync
 
 - Add `filterVariables` option to `SyncClient` builder.

objectbox-java/src/main/java/io/objectbox/BoxStore.java

@@ -244,8 +244,11 @@ public static boolean isSyncServerAvailable() {
     private final PrintStream errorOutputStream;
     private final File directory;
     private final String canonicalPath;
+
     /** Reference to the native store. Should probably get through {@link #getNativeStore()} instead. */
     volatile private long handle;
+    volatile private boolean nativeStoreDestroyed = false;
+
     private final Map<Class<?>, String> dbNameByClass = new HashMap<>();
     private final Map<Class<?>, Integer> entityTypeIdByClass = new HashMap<>();
     private final Map<Class<?>, EntityInfo<?>> propertiesByClass = new HashMap<>();
@@ -724,6 +727,7 @@ public void close() {
                 handle = 0;
                 if (handleToDelete != 0) { // failed before native handle was created?
                     nativeDelete(handleToDelete);
+                    nativeStoreDestroyed = true;
                 }
             }
         }
@@ -1405,15 +1409,31 @@ public long getNativeStore() {
     /**
      * For internal use only. This API might change or be removed with a future release.
      * <p>
-     * Returns if the native Store was closed.
+     * Returns {@code true} once the native Store is about to be destroyed.
      * <p>
      * This is {@code true} shortly after {@link #close()} was called and {@link #isClosed()} returns {@code true}.
+     *
+     * @see #isNativeStoreDestroyed()
      */
     @Internal
     public boolean isNativeStoreClosed() {
         return handle == 0;
     }
 
+    /**
+     * For internal use only. This API might change or be removed with a future release.
+     * <p>
+     * Returns {@code true} once the native Store was destroyed.
+     * <p>
+     * This is {@code true} shortly after {@link #isNativeStoreClosed()} returns {@code true}.
+     *
+     * @see #isNativeStoreClosed()
+     */
+    @Internal
+    public boolean isNativeStoreDestroyed() {
+        return nativeStoreDestroyed;
+    }
+
     /**
      * Returns the {@link SyncClient} associated with this store. To create one see {@link io.objectbox.sync.Sync Sync}.
      */

objectbox-java/src/main/java/io/objectbox/Transaction.java

@@ -101,7 +101,9 @@ public synchronized void close() {
             closed = true;
             store.unregisterTransaction(this);
 
-            if (!nativeIsOwnerThread(transaction)) {
+            boolean isOwnerThread = nativeIsOwnerThread(transaction);
+            if (!isOwnerThread) {
+                // Note: don't use isActive(), it returns false here because closed == true already
                 boolean isActive = nativeIsActive(transaction);
                 boolean isRecycled = nativeIsRecycled(transaction);
                 if (isActive || isRecycled) {
@@ -126,10 +128,49 @@ public synchronized void close() {
             // TODO not destroying is probably only a small leak on rare occasions, but still could be fixed
             if (!store.isNativeStoreClosed()) {
                 nativeDestroy(transaction);
+            } else {
+                // Note: don't use isActive(), it returns false here because closed == true already
+                boolean isActive = nativeIsActive(transaction);
+                if (readOnly) {
+                    // Minor leak if TX is active, but still log so the ObjectBox team can check that it only happens
+                    // occasionally.
+                    // Note this cannot assume the store isn't destroyed, yet. The native and Java stores may at best
+                    // briefly wait for read transactions.
+                    System.out.printf(
+                            "Info: closing read transaction after store was closed (isActive=%s, isOwnerThread=%s), this should be avoided.%n",
+                            isActive, isOwnerThread);
+                    System.out.flush();
+
+                    // Note: get fresh active state
+                    if (!nativeIsActive(transaction)) {
+                        nativeDestroy(transaction);
+                    }
+                } else {
+                    // write transaction
+                    System.out.printf(
+                            "WARN: closing write transaction after store was closed (isActive=%s, isOwnerThread=%s), this must be avoided.%n",
+                            isActive, isOwnerThread);
+                    System.out.flush();
+
+                    // Note: get fresh active state
+                    if (nativeIsActive(transaction) && store.isNativeStoreDestroyed()) {
+                        // This is an internal validation: if this is an active write-TX,
+                        // the (native) store will always wait for it, so it must not be destroyed yet.
+                        // If this ever happens, the above assumption is wrong, and throwing likely prevents a SIGSEGV.
+                        throw new IllegalStateException(
+                                "Internal error: cannot close active write transaction for an already destroyed store");
+                    }
+                    // Note: inactive transactions are always safe to destroy, regardless of store state and thread.
+                    // Note: the current native impl panics if the transaction is active AND created in another thread.
+                    nativeDestroy(transaction);
+                }
             }
         }
     }
 
+    /**
+     * For a write transaction commits the changes. For a read transaction throws.
+     */
     public void commit() {
         checkOpen();
         int[] entityTypeIdsAffected = nativeCommit(transaction);
@@ -141,6 +182,9 @@ public void commitAndClose() {
         close();
     }
 
+    /**
+     * For a read or write transaction, aborts it.
+     */
     public void abort() {
         checkOpen();
         nativeAbort(transaction);
@@ -192,6 +236,12 @@ public BoxStore getStore() {
         return store;
     }
 
+    /**
+     * A transaction is active after it was created until {@link #close()}, {@link #abort()}, or, for write
+     * transactions only, {@link #commit()} is called.
+     *
+     * @return If this transaction is active.
+     */
     public boolean isActive() {
         return !closed && nativeIsActive(transaction);
     }

tests/objectbox-java-test/src/test/java/io/objectbox/CursorTest.java

@@ -16,10 +16,10 @@
 
 package io.objectbox;
 
-import org.junit.Ignore;
 import org.junit.Test;
 
 import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ThreadLocalRandom;
 import java.util.concurrent.TimeUnit;
 
 import io.objectbox.annotation.IndexType;
@@ -228,25 +228,54 @@ public void testClose() {
         }
     }
 
-    @Ignore("Temporarily ignore until objectbox-java#282 is resolved")
+    /**
+     * Begin the first write-TX and ensure the second one blocks until the first one is closed.
+     * A secondary test goal is to check races of a closing TX and a closing store.
+     */
     @Test
     public void testWriteTxBlocksOtherWriteTx() throws InterruptedException {
+        // To change the likelihood of the TX vs store closing race, close the store using one of 3 different variants.
+        // Assign and print the randomly chosen variant beforehand so it does not mess with thread timings later.
+        // Warning: test variant 2 only manually, it will close the write-TX from a non-owner thread (in BoxStore.close)
+        // where the native database is expected to panic.
+        int closeStoreVariant = ThreadLocalRandom.current().nextInt(2 /* 3 - test variant 2 manually, see above */);
+        System.out.println("Closing store variant: " + closeStoreVariant);
+
         long time = System.currentTimeMillis();
         Transaction tx = store.beginTx();
         long duration = System.currentTimeMillis() - time; // Usually 0 on desktop
         final CountDownLatch latchBeforeBeginTx = new CountDownLatch(1);
         final CountDownLatch latchAfterBeginTx = new CountDownLatch(1);
+        final CountDownLatch latchCloseStoreInMainThread = closeStoreVariant != 0 ? new CountDownLatch(1) : null;
         new Thread(() -> {
             latchBeforeBeginTx.countDown();
             Transaction tx2 = store.beginTx();
             latchAfterBeginTx.countDown();
+
+            if (closeStoreVariant != 0) {
+                try {
+                    assertTrue(latchCloseStoreInMainThread.await(5, TimeUnit.SECONDS));
+                } catch (InterruptedException e) {
+                    throw new RuntimeException("Interrupted", e);
+                }
+            }
             tx2.close();
         }).start();
         assertTrue(latchBeforeBeginTx.await(1, TimeUnit.SECONDS));
         long waitTime = 100 + duration * 10;
         assertFalse(latchAfterBeginTx.await(waitTime, TimeUnit.MILLISECONDS));
         tx.close();
         assertTrue(latchAfterBeginTx.await(waitTime * 2, TimeUnit.MILLISECONDS));
+
+        // closeStoreVariant == 0: not latch waiting, close store when tearing down test
+        if (closeStoreVariant == 1) {
+            // This variant tries to close the store and the TX at the same time.
+            latchCloseStoreInMainThread.countDown();
+            store.close();  // Maybe this runs a tiny bit before the close() in teardown(?)
+        } else if (closeStoreVariant == 2) {
+            store.close();  // Enforces closing the store before the TX.
+            latchCloseStoreInMainThread.countDown();
+        }
     }
 
     @Test