From 901235b53b0286367dc3244ab294bb05b493bb7a Mon Sep 17 00:00:00 2001 From: Uwe <13865709+greenrobot-team@users.noreply.github.com> Date: Tue, 6 Aug 2024 11:01:33 +0200 Subject: [PATCH 1/3] Tests: closing Store while Transaction is active should not crash --- .../java/io/objectbox/InternalAccess.java | 8 ++- .../objectbox/query/InternalQueryAccess.java | 37 +++++++++++ .../java/io/objectbox/TransactionTest.java | 63 +++++++++++++++++-- 3 files changed, 103 insertions(+), 5 deletions(-) create mode 100644 objectbox-java/src/main/java/io/objectbox/query/InternalQueryAccess.java diff --git a/objectbox-java/src/main/java/io/objectbox/InternalAccess.java b/objectbox-java/src/main/java/io/objectbox/InternalAccess.java index 84f23601..572db0f4 100644 --- a/objectbox-java/src/main/java/io/objectbox/InternalAccess.java +++ b/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. + *

+ * To avoid this, future APIs should be exposed via interfaces with an internal implementation that can be used by + * tests. + */ @Internal public class InternalAccess { diff --git a/objectbox-java/src/main/java/io/objectbox/query/InternalQueryAccess.java b/objectbox-java/src/main/java/io/objectbox/query/InternalQueryAccess.java new file mode 100644 index 00000000..01be4fd8 --- /dev/null +++ b/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. + *

+ * 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 void nativeFindFirst(Query query, long cursorHandle) { + query.nativeFindFirst(query.handle, cursorHandle); + } + +} diff --git a/tests/objectbox-java-test/src/test/java/io/objectbox/TransactionTest.java b/tests/objectbox-java-test/src/test/java/io/objectbox/TransactionTest.java index bb233fa9..def0e0f2 100644 --- a/tests/objectbox-java-test/src/test/java/io/objectbox/TransactionTest.java +++ b/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 { @@ -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 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 waitingCallable = () -> { + Box box = store.boxFor(TestEntity.class); + Query 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 box = getTestEntityBox(); From c7ac7ed40937abb635307a9ca9177ada3f9a05aa Mon Sep 17 00:00:00 2001 From: Uwe <13865709+greenrobot-team@users.noreply.github.com> Date: Tue, 6 Aug 2024 13:05:06 +0200 Subject: [PATCH 2/3] BoxStore: on close, wait briefly on open transactions To enable this, change Transaction.isActive() to not throw if transaction is closed. --- .../src/main/java/io/objectbox/BoxStore.java | 66 +++++++++++++++++-- .../main/java/io/objectbox/Transaction.java | 7 +- .../java/io/objectbox/TransactionTest.java | 2 +- 3 files changed, 64 insertions(+), 11 deletions(-) diff --git a/objectbox-java/src/main/java/io/objectbox/BoxStore.java b/objectbox-java/src/main/java/io/objectbox/BoxStore.java index 33ba756b..7ab6ce48 100644 --- a/objectbox-java/src/main/java/io/objectbox/BoxStore.java +++ b/objectbox-java/src/main/java/io/objectbox/BoxStore.java @@ -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. *

- * 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, all database operations must have finished (there are no more active transactions). + *

+ * 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,14 +659,37 @@ 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 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 @@ -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()}. + *

+ * 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. + *

+ * Returns if the native Store was closed. + *

+ * 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}. */ diff --git a/objectbox-java/src/main/java/io/objectbox/Transaction.java b/objectbox-java/src/main/java/io/objectbox/Transaction.java index 5e2035f7..8939e2cf 100644 --- a/objectbox-java/src/main/java/io/objectbox/Transaction.java +++ b/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() { diff --git a/tests/objectbox-java-test/src/test/java/io/objectbox/TransactionTest.java b/tests/objectbox-java-test/src/test/java/io/objectbox/TransactionTest.java index def0e0f2..41260424 100644 --- a/tests/objectbox-java-test/src/test/java/io/objectbox/TransactionTest.java +++ b/tests/objectbox-java-test/src/test/java/io/objectbox/TransactionTest.java @@ -299,6 +299,7 @@ public void testClose() { assertFalse(tx.isClosed()); tx.close(); assertTrue(tx.isClosed()); + assertFalse(tx.isActive()); // Double close should be fine tx.close(); @@ -312,7 +313,6 @@ public void testClose() { assertThrowsTxClosed(tx::renew); assertThrowsTxClosed(tx::createKeyValueCursor); assertThrowsTxClosed(() -> tx.createCursor(TestEntity.class)); - assertThrowsTxClosed(tx::isActive); assertThrowsTxClosed(tx::isRecycled); } From acc26bf7ea4fa82e8df621640d524e1e48a3b5d2 Mon Sep 17 00:00:00 2001 From: Markus Date: Tue, 13 Aug 2024 15:22:06 +0200 Subject: [PATCH 3/3] BoxStore: make handle volatile Also, set handle to 0 before calling nativeDelete() to mitigate races --- .../src/main/java/io/objectbox/BoxStore.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/objectbox-java/src/main/java/io/objectbox/BoxStore.java b/objectbox-java/src/main/java/io/objectbox/BoxStore.java index 7ab6ce48..762b2e12 100644 --- a/objectbox-java/src/main/java/io/objectbox/BoxStore.java +++ b/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, String> dbNameByClass = new HashMap<>(); private final Map, Integer> entityTypeIdByClass = new HashMap<>(); private final Map, EntityInfo> propertiesByClass = new HashMap<>(); @@ -690,11 +690,11 @@ public void close() { 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: