diff --git a/java/src/main/java/org/rocksdb/AbstractImmutableNativeReference.java b/java/src/main/java/org/rocksdb/AbstractImmutableNativeReference.java index 173d63e9011..66f3f2ec157 100644 --- a/java/src/main/java/org/rocksdb/AbstractImmutableNativeReference.java +++ b/java/src/main/java/org/rocksdb/AbstractImmutableNativeReference.java @@ -28,7 +28,10 @@ protected AbstractImmutableNativeReference(final boolean owningHandle) { @Override public boolean isOwningHandle() { - return owningHandle_.get(); + if (!owningHandle_.get()) { + throw new RuntimeException("Object does not own its native handle."); + } + return true; } /** diff --git a/java/src/main/java/org/rocksdb/AbstractRocksIterator.java b/java/src/main/java/org/rocksdb/AbstractRocksIterator.java index b7af848f0c5..fed91988832 100644 --- a/java/src/main/java/org/rocksdb/AbstractRocksIterator.java +++ b/java/src/main/java/org/rocksdb/AbstractRocksIterator.java @@ -6,6 +6,7 @@ package org.rocksdb; import java.nio.ByteBuffer; +import java.util.function.Function; /** * Base class implementation for Rocks Iterators @@ -24,9 +25,10 @@ public abstract class AbstractRocksIterator

extends RocksObject implements RocksIteratorInterface { final P parent_; + final Function, Boolean> removeOnClosure_; protected AbstractRocksIterator(final P parent, - final long nativeHandle) { + final long nativeHandle, final Function, Boolean> removeOnClosure) { super(nativeHandle); // parent must point to a valid RocksDB instance. assert (parent != null); @@ -34,6 +36,12 @@ protected AbstractRocksIterator(final P parent, // to guarantee that while a GC cycle starts RocksIterator instances // are freed prior to parent instances. parent_ = parent; + removeOnClosure_ = removeOnClosure; + } + + protected AbstractRocksIterator(final P parent, + final long nativeHandle) { + this(parent, nativeHandle, null); } @Override @@ -120,6 +128,14 @@ public void status() throws RocksDBException { status0(nativeHandle_); } + @Override + public void close() { + super.close(); + if (removeOnClosure_ != null) { + removeOnClosure_.apply(this); + } + } + /** *

Deletes underlying C++ iterator pointer.

* diff --git a/java/src/main/java/org/rocksdb/RocksDB.java b/java/src/main/java/org/rocksdb/RocksDB.java index 2467d249b05..530be1d5501 100644 --- a/java/src/main/java/org/rocksdb/RocksDB.java +++ b/java/src/main/java/org/rocksdb/RocksDB.java @@ -43,6 +43,8 @@ private enum LibraryState { final List ownedColumnFamilyHandles = new ArrayList<>(); + final List ownedIterators = new ArrayList<>(); + /** * Loads the necessary library files. * Calling this method twice will have no effect. @@ -674,6 +676,12 @@ public void closeE() throws RocksDBException { @SuppressWarnings("PMD.EmptyCatchBlock") @Override public void close() { + final List removeIterators = new ArrayList<>(ownedIterators); + for (final RocksIterator rocksIterator : removeIterators) { + //The lambda supplied to rocksIterator on construction will remove it from ownedIterators + rocksIterator.close(); + } + for (final ColumnFamilyHandle columnFamilyHandle : // NOPMD - CloseResource ownedColumnFamilyHandles) { columnFamilyHandle.close(); @@ -3262,8 +3270,7 @@ public KeyMayExist keyMayExist(final ColumnFamilyHandle columnFamilyHandle, * @return instance of iterator object. */ public RocksIterator newIterator() { - return new RocksIterator(this, - iterator(nativeHandle_, defaultColumnFamilyHandle_.nativeHandle_, + return makeIterator(iterator(nativeHandle_, defaultColumnFamilyHandle_.nativeHandle_, defaultReadOptions_.nativeHandle_)); } @@ -3281,9 +3288,8 @@ public RocksIterator newIterator() { * @return instance of iterator object. */ public RocksIterator newIterator(final ReadOptions readOptions) { - return new RocksIterator(this, - iterator( - nativeHandle_, defaultColumnFamilyHandle_.nativeHandle_, readOptions.nativeHandle_)); + return makeIterator(iterator( + nativeHandle_, defaultColumnFamilyHandle_.nativeHandle_, readOptions.nativeHandle_)); } /** @@ -3302,7 +3308,7 @@ public RocksIterator newIterator(final ReadOptions readOptions) { */ public RocksIterator newIterator( final ColumnFamilyHandle columnFamilyHandle) { - return new RocksIterator(this, + return makeIterator( iterator( nativeHandle_, columnFamilyHandle.nativeHandle_, defaultReadOptions_.nativeHandle_)); } @@ -3324,8 +3330,7 @@ public RocksIterator newIterator( */ public RocksIterator newIterator(final ColumnFamilyHandle columnFamilyHandle, final ReadOptions readOptions) { - return new RocksIterator( - this, iterator(nativeHandle_, columnFamilyHandle.nativeHandle_, readOptions.nativeHandle_)); + return makeIterator(iterator(nativeHandle_, columnFamilyHandle.nativeHandle_, readOptions.nativeHandle_)); } /** @@ -3376,7 +3381,7 @@ public List newIterators( final List iterators = new ArrayList<>( columnFamilyHandleList.size()); for (int i=0; iAn iterator that yields a sequence of key/value pairs from a source. @@ -27,6 +28,10 @@ protected RocksIterator(final RocksDB rocksDB, final long nativeHandle) { super(rocksDB, nativeHandle); } + protected RocksIterator(final RocksDB rocksDB, final long nativeHandle, final Function, Boolean> removeOnClosure) { + super(rocksDB, nativeHandle, removeOnClosure); + } + /** *

Return the key for the current entry. The underlying storage for * the returned slice is valid only until the next modification of diff --git a/java/src/test/java/org/rocksdb/IteratorClosedDBTest.java b/java/src/test/java/org/rocksdb/IteratorClosedDBTest.java new file mode 100644 index 00000000000..db3d3a6fa3f --- /dev/null +++ b/java/src/test/java/org/rocksdb/IteratorClosedDBTest.java @@ -0,0 +1,79 @@ +package org.rocksdb; + +import org.junit.ClassRule; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; + +import java.nio.file.Paths; + +import static org.assertj.core.api.Assertions.assertThat; + +public class IteratorClosedDBTest { + + @ClassRule + public static final RocksNativeLibraryResource ROCKS_NATIVE_LIBRARY_RESOURCE = + new RocksNativeLibraryResource(); + + @Rule + public TemporaryFolder dbFolder = new TemporaryFolder(); + + @Test + public void resourceIterators() throws RocksDBException { + try (Options options = new Options().setCreateIfMissing(true); RocksDB db = RocksDB.open(options, dbFolder.getRoot().getAbsolutePath())) { + + byte[] key = {0x1}; + byte[] value = {0x2}; + db.put(key, value); + + try (RocksIterator it = db.newIterator()) { + it.seekToFirst(); + assertThat(it.key()).isEqualTo(key); + assertThat(it.value()).isEqualTo(value); + + it.next(); + assertThat(it.isValid()).isFalse(); + } + } + } + + @Test + public void ownedIterators() throws RocksDBException { + try (Options options = new Options().setCreateIfMissing(true); RocksDB db = RocksDB.open(options, dbFolder.getRoot().getAbsolutePath())) { + + byte[] key = {0x1}; + byte[] value = {0x2}; + db.put(key, value); + + RocksIterator it = db.newIterator(); + it.seekToFirst(); + assertThat(it.key()).isEqualTo(key); + assertThat(it.value()).isEqualTo(value); + + it.next(); + assertThat(it.isValid()).isFalse(); + } //iterator is still open when we close the DB, C++ assertion in DEBUG_LEVEL=1 + } + + @Test + public void shouldCrashJavaRocks() throws RocksDBException { + try (Options options = new Options().setCreateIfMissing(true)) { + RocksDB db = RocksDB.open(options, dbFolder.getRoot().getAbsolutePath()); + byte[] key = {0x1}; + byte[] value = {0x2}; + db.put(key, value); + + RocksIterator it = db.newIterator(); + assertThat(it.isValid()).isFalse(); + it.seekToFirst(); + assertThat(it.isValid()).isTrue(); + + // Exception here (assertion failure in C++) - when built with DEBUG_LEVEL=1 + // Outstanding iterator has a reference to the column family which is being closed + db.close(); + + assertThat(it.isValid()).isFalse(); + it.close(); + } + } +}