Merge branch '1068-close-while-read-tx-active' into 'dev'

BoxStore: before closing, briefly wait on, then try to close active transactions objectbox/objectbox#1068

See merge request objectbox/objectbox-java!140

Author: greenrobot-team

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

@@ -233,7 +233,7 @@ public static boolean isSyncServerAvailable() {
     private final File directory;
     private final String canonicalPath;
     /** Reference to the native store. Should probably get through {@link #getNativeStore()} instead. */
-    private long handle;
+    volatile private long handle;
     private final Map<Class<?>, String> dbNameByClass = new HashMap<>();
     private final Map<Class<?>, Integer> entityTypeIdByClass = new HashMap<>();
     private final Map<Class<?>, EntityInfo<?>> propertiesByClass = new HashMap<>();
@@ -636,13 +636,14 @@ public boolean isReadOnly() {
     }
 
     /**
-     * Closes the BoxStore and frees associated resources.
-     * This method is useful for unit tests;
-     * most real applications should open a BoxStore once and keep it open until the app dies.
+     * Closes this BoxStore and releases associated resources.
      * <p>
-     * WARNING:
-     * This is a somewhat delicate thing to do if you have threads running that may potentially still use the BoxStore.
-     * This results in undefined behavior, including the possibility of crashing.
+     * Before calling, <b>all database operations must have finished</b> (there are no more active transactions).
+     * <p>
+     * If that is not the case, the method will briefly wait on any active transactions, but then will forcefully close
+     * them to avoid crashes and print warning messages ("Transactions are still active"). If this occurs,
+     * analyze your code to make sure all database operations, notably in other threads or data observers,
+     * are properly finished.
      */
     public void close() {
         boolean oldClosedState;
@@ -658,19 +659,42 @@ public void close() {
                 }
 
                 // Closeable recommendation: mark as closed before any code that might throw.
+                // Also, before checking on transactions to avoid any new transactions from getting created
+                // (due to all Java APIs doing closed checks).
                 closed = true;
+
                 List<Transaction> transactionsToClose;
                 synchronized (transactions) {
+                    // Give open transactions some time to close (BoxStore.unregisterTransaction() calls notify),
+                    // 1000 ms should be long enough for most small operations and short enough to avoid ANRs on Android.
+                    if (hasActiveTransaction()) {
+                        System.out.println("Briefly waiting for active transactions before closing the Store...");
+                        try {
+                            // It is fine to hold a lock on BoxStore.this as well as BoxStore.unregisterTransaction()
+                            // only synchronizes on "transactions".
+                            //noinspection WaitWhileHoldingTwoLocks
+                            transactions.wait(1000);
+                        } catch (InterruptedException e) {
+                            // If interrupted, continue with releasing native resources
+                        }
+                        if (hasActiveTransaction()) {
+                            System.err.println("Transactions are still active:"
+                                    + " ensure that all database operations are finished before closing the Store!");
+                        }
+                    }
                     transactionsToClose = new ArrayList<>(this.transactions);
                 }
+                // Close all transactions, including recycled (not active) ones stored in Box threadLocalReader.
+                // It is expected that this prints a warning if a transaction is not owned by the current thread.
                 for (Transaction t : transactionsToClose) {
                     t.close();
                 }
-                if (handle != 0) { // failed before native handle was created?
-                    nativeDelete(handle);
-                    // The Java API has open checks, but just in case re-set the handle so any native methods will
-                    // not crash due to an invalid pointer.
-                    handle = 0;
+
+                long handleToDelete = handle;
+                // Make isNativeStoreClosed() return true before actually closing to avoid Transaction.close() crash
+                handle = 0;
+                if (handleToDelete != 0) { // failed before native handle was created?
+                    nativeDelete(handleToDelete);
                 }
 
                 // When running the full unit test suite, we had 100+ threads before, hope this helps:
@@ -814,9 +838,27 @@ public void removeAllObjects() {
     public void unregisterTransaction(Transaction transaction) {
         synchronized (transactions) {
             transactions.remove(transaction);
+            // For close(): notify if there are no more open transactions
+            if (!hasActiveTransaction()) {
+                transactions.notifyAll();
+            }
         }
     }
 
+    /**
+     * Returns if {@link #transactions} has a single transaction that {@link Transaction#isActive() isActive()}.
+     * <p>
+     * Callers must synchronize on {@link #transactions}.
+     */
+    private boolean hasActiveTransaction() {
+        for (Transaction tx : transactions) {
+            if (tx.isActive()) {
+                return true;
+            }
+        }
+        return false;
+    }
+
     void txCommitted(Transaction tx, @Nullable int[] entityTypeIdsAffected) {
         // Only one write TX at a time, but there is a chance two writers race after commit: thus synchronize
         synchronized (txCommitCountLock) {
@@ -1290,6 +1332,18 @@ public long getNativeStore() {
         return handle;
     }
 
+    /**
+     * For internal use only. This API might change or be removed with a future release.
+     * <p>
+     * Returns if the native Store was closed.
+     * <p>
+     * This is {@code true} shortly after {@link #close()} was called and {@link #isClosed()} returns {@code true}.
+     */
+    @Internal
+    public boolean isNativeStoreClosed() {
+        return handle == 0;
+    }
+
     /**
      * 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/InternalAccess.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2017 ObjectBox Ltd. All rights reserved.
+ * Copyright 2017-2024 ObjectBox Ltd. All rights reserved.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -21,6 +21,12 @@
 import io.objectbox.annotation.apihint.Internal;
 import io.objectbox.sync.SyncClient;
 
+/**
+ * This is a workaround to access internal APIs, notably for tests.
+ * <p>
+ * To avoid this, future APIs should be exposed via interfaces with an internal implementation that can be used by
+ * tests.
+ */
 @Internal
 public class InternalAccess {
 

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2017 ObjectBox Ltd. All rights reserved.
+ * Copyright 2017-2024 ObjectBox Ltd. All rights reserved.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -124,7 +124,7 @@ public synchronized void close() {
 
             // If store is already closed natively, destroying the tx would cause EXCEPTION_ACCESS_VIOLATION
             // TODO not destroying is probably only a small leak on rare occasions, but still could be fixed
-            if (!store.isClosed()) {
+            if (!store.isNativeStoreClosed()) {
                 nativeDestroy(transaction);
             }
         }
@@ -193,8 +193,7 @@ public BoxStore getStore() {
     }
 
     public boolean isActive() {
-        checkOpen();
-        return nativeIsActive(transaction);
+        return !closed && nativeIsActive(transaction);
     }
 
     public boolean isRecycled() {

objectbox-java/src/main/java/io/objectbox/query/InternalQueryAccess.java

@@ -0,0 +1,37 @@
+/*
+ * Copyright 2024 ObjectBox Ltd. All rights reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package io.objectbox.query;
+
+import io.objectbox.annotation.apihint.Internal;
+
+/**
+ * This is a workaround to access internal APIs for tests.
+ * <p>
+ * To avoid this, future APIs should be exposed via interfaces with an internal implementation that can be used by
+ * tests.
+ */
+@Internal
+public class InternalQueryAccess {
+
+    /**
+     * For testing only.
+     */
+    public static <T> void nativeFindFirst(Query<T> query, long cursorHandle) {
+        query.nativeFindFirst(query.handle, cursorHandle);
+    }
+
+}

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2017 ObjectBox Ltd. All rights reserved.
+ * Copyright 2017-2024 ObjectBox Ltd. All rights reserved.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -16,6 +16,10 @@
 
 package io.objectbox;
 
+import org.junit.Ignore;
+import org.junit.Test;
+import org.junit.function.ThrowingRunnable;
+
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.concurrent.Callable;
@@ -27,13 +31,14 @@
 import java.util.concurrent.ThreadPoolExecutor;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.atomic.AtomicReference;
 
 import io.objectbox.exception.DbException;
 import io.objectbox.exception.DbExceptionListener;
 import io.objectbox.exception.DbMaxReadersExceededException;
-import org.junit.Ignore;
-import org.junit.Test;
-import org.junit.function.ThrowingRunnable;
+import io.objectbox.query.InternalQueryAccess;
+import io.objectbox.query.Query;
+
 
 import static org.junit.Assert.assertArrayEquals;
 import static org.junit.Assert.assertEquals;
@@ -44,6 +49,7 @@
 import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
+import static org.junit.Assume.assumeFalse;
 
 public class TransactionTest extends AbstractObjectBoxTest {
 
@@ -293,6 +299,7 @@ public void testClose() {
         assertFalse(tx.isClosed());
         tx.close();
         assertTrue(tx.isClosed());
+        assertFalse(tx.isActive());
 
         // Double close should be fine
         tx.close();
@@ -306,7 +313,6 @@ public void testClose() {
         assertThrowsTxClosed(tx::renew);
         assertThrowsTxClosed(tx::createKeyValueCursor);
         assertThrowsTxClosed(() -> tx.createCursor(TestEntity.class));
-        assertThrowsTxClosed(tx::isActive);
         assertThrowsTxClosed(tx::isRecycled);
     }
 
@@ -315,6 +321,55 @@ private void assertThrowsTxClosed(ThrowingRunnable runnable) {
         assertEquals("Transaction is closed", ex.getMessage());
     }
 
+    @Test
+    public void nativeCallInTx_storeIsClosed_throws() throws InterruptedException {
+        // Ignore test on Windows, it was observed to crash with EXCEPTION_ACCESS_VIOLATION
+        assumeFalse(TestUtils.isWindows());
+
+        System.out.println("NOTE This test will cause \"Transaction is still active\" and \"Irrecoverable memory error\" error logs!");
+
+        CountDownLatch callableIsReady = new CountDownLatch(1);
+        CountDownLatch storeIsClosed = new CountDownLatch(1);
+        CountDownLatch callableIsDone = new CountDownLatch(1);
+        AtomicReference<Exception> callableException = new AtomicReference<>();
+
+        // Goal: be just passed closed checks on the Java side, about to call a native query API.
+        // Then close the Store, then call the native API. The native API call should not crash the VM.
+        Callable<Void> waitingCallable = () -> {
+            Box<TestEntity> box = store.boxFor(TestEntity.class);
+            Query<TestEntity> query = box.query().build();
+            // Obtain Cursor handle before closing the Store as getActiveTxCursor() has a closed check
+            long cursorHandle = io.objectbox.InternalAccess.getActiveTxCursorHandle(box);
+            
+            callableIsReady.countDown();
+            try {
+                if (!storeIsClosed.await(5, TimeUnit.SECONDS)) {
+                    throw new IllegalStateException("Store did not close within 5 seconds");
+                }
+                // Call native query API within the transaction (opened by callInReadTx below)
+                InternalQueryAccess.nativeFindFirst(query, cursorHandle);
+                query.close();
+            } catch (Exception e) {
+                callableException.set(e);
+            }
+            callableIsDone.countDown();
+            return null;
+        };
+        new Thread(() -> store.callInReadTx(waitingCallable)).start();
+
+        callableIsReady.await();
+        store.close();
+        storeIsClosed.countDown();
+
+        if (!callableIsDone.await(10, TimeUnit.SECONDS)) {
+            fail("Callable did not finish within 10 seconds");
+        }
+        Exception exception = callableException.get();
+        assertTrue(exception instanceof IllegalStateException);
+        // Note: the "State" at the end of the message may be different depending on platform, so only assert prefix
+        assertTrue(exception.getMessage().startsWith("Illegal Store instance detected! This is a severe usage error that must be fixed."));
+    }
+
     @Test
     public void testRunInTxRecursive() {
         final Box<TestEntity> box = getTestEntityBox();