Skip to content

Commit c7ac7ed

Browse files
BoxStore: on close, wait briefly on open transactions
To enable this, change Transaction.isActive() to not throw if transaction is closed.
1 parent 901235b commit c7ac7ed

File tree

3 files changed

+64
-11
lines changed

3 files changed

+64
-11
lines changed

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

Lines changed: 60 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -636,13 +636,14 @@ public boolean isReadOnly() {
636636
}
637637

638638
/**
639-
* Closes the BoxStore and frees associated resources.
640-
* This method is useful for unit tests;
641-
* most real applications should open a BoxStore once and keep it open until the app dies.
639+
* Closes this BoxStore and releases associated resources.
642640
* <p>
643-
* WARNING:
644-
* This is a somewhat delicate thing to do if you have threads running that may potentially still use the BoxStore.
645-
* This results in undefined behavior, including the possibility of crashing.
641+
* Before calling, <b>all database operations must have finished</b> (there are no more active transactions).
642+
* <p>
643+
* If that is not the case, the method will briefly wait on any active transactions, but then will forcefully close
644+
* them to avoid crashes and print warning messages ("Transactions are still active"). If this occurs,
645+
* analyze your code to make sure all database operations, notably in other threads or data observers,
646+
* are properly finished.
646647
*/
647648
public void close() {
648649
boolean oldClosedState;
@@ -658,14 +659,37 @@ public void close() {
658659
}
659660

660661
// Closeable recommendation: mark as closed before any code that might throw.
662+
// Also, before checking on transactions to avoid any new transactions from getting created
663+
// (due to all Java APIs doing closed checks).
661664
closed = true;
665+
662666
List<Transaction> transactionsToClose;
663667
synchronized (transactions) {
668+
// Give open transactions some time to close (BoxStore.unregisterTransaction() calls notify),
669+
// 1000 ms should be long enough for most small operations and short enough to avoid ANRs on Android.
670+
if (hasActiveTransaction()) {
671+
System.out.println("Briefly waiting for active transactions before closing the Store...");
672+
try {
673+
// It is fine to hold a lock on BoxStore.this as well as BoxStore.unregisterTransaction()
674+
// only synchronizes on "transactions".
675+
//noinspection WaitWhileHoldingTwoLocks
676+
transactions.wait(1000);
677+
} catch (InterruptedException e) {
678+
// If interrupted, continue with releasing native resources
679+
}
680+
if (hasActiveTransaction()) {
681+
System.err.println("Transactions are still active:"
682+
+ " ensure that all database operations are finished before closing the Store!");
683+
}
684+
}
664685
transactionsToClose = new ArrayList<>(this.transactions);
665686
}
687+
// Close all transactions, including recycled (not active) ones stored in Box threadLocalReader.
688+
// It is expected that this prints a warning if a transaction is not owned by the current thread.
666689
for (Transaction t : transactionsToClose) {
667690
t.close();
668691
}
692+
669693
if (handle != 0) { // failed before native handle was created?
670694
nativeDelete(handle);
671695
// 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() {
814838
public void unregisterTransaction(Transaction transaction) {
815839
synchronized (transactions) {
816840
transactions.remove(transaction);
841+
// For close(): notify if there are no more open transactions
842+
if (!hasActiveTransaction()) {
843+
transactions.notifyAll();
844+
}
817845
}
818846
}
819847

848+
/**
849+
* Returns if {@link #transactions} has a single transaction that {@link Transaction#isActive() isActive()}.
850+
* <p>
851+
* Callers must synchronize on {@link #transactions}.
852+
*/
853+
private boolean hasActiveTransaction() {
854+
for (Transaction tx : transactions) {
855+
if (tx.isActive()) {
856+
return true;
857+
}
858+
}
859+
return false;
860+
}
861+
820862
void txCommitted(Transaction tx, @Nullable int[] entityTypeIdsAffected) {
821863
// Only one write TX at a time, but there is a chance two writers race after commit: thus synchronize
822864
synchronized (txCommitCountLock) {
@@ -1290,6 +1332,18 @@ public long getNativeStore() {
12901332
return handle;
12911333
}
12921334

1335+
/**
1336+
* For internal use only. This API might change or be removed with a future release.
1337+
* <p>
1338+
* Returns if the native Store was closed.
1339+
* <p>
1340+
* This is {@code true} shortly after {@link #close()} was called and {@link #isClosed()} returns {@code true}.
1341+
*/
1342+
@Internal
1343+
public boolean isNativeStoreClosed() {
1344+
return handle == 0;
1345+
}
1346+
12931347
/**
12941348
* Returns the {@link SyncClient} associated with this store. To create one see {@link io.objectbox.sync.Sync Sync}.
12951349
*/

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

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2017 ObjectBox Ltd. All rights reserved.
2+
* Copyright 2017-2024 ObjectBox Ltd. All rights reserved.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -124,7 +124,7 @@ public synchronized void close() {
124124

125125
// If store is already closed natively, destroying the tx would cause EXCEPTION_ACCESS_VIOLATION
126126
// TODO not destroying is probably only a small leak on rare occasions, but still could be fixed
127-
if (!store.isClosed()) {
127+
if (!store.isNativeStoreClosed()) {
128128
nativeDestroy(transaction);
129129
}
130130
}
@@ -193,8 +193,7 @@ public BoxStore getStore() {
193193
}
194194

195195
public boolean isActive() {
196-
checkOpen();
197-
return nativeIsActive(transaction);
196+
return !closed && nativeIsActive(transaction);
198197
}
199198

200199
public boolean isRecycled() {

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -299,6 +299,7 @@ public void testClose() {
299299
assertFalse(tx.isClosed());
300300
tx.close();
301301
assertTrue(tx.isClosed());
302+
assertFalse(tx.isActive());
302303

303304
// Double close should be fine
304305
tx.close();
@@ -312,7 +313,6 @@ public void testClose() {
312313
assertThrowsTxClosed(tx::renew);
313314
assertThrowsTxClosed(tx::createKeyValueCursor);
314315
assertThrowsTxClosed(() -> tx.createCursor(TestEntity.class));
315-
assertThrowsTxClosed(tx::isActive);
316316
assertThrowsTxClosed(tx::isRecycled);
317317
}
318318

0 commit comments

Comments
 (0)