From 932fbfedc4ea17c12b1ba23c45bbdb264485ce1c Mon Sep 17 00:00:00 2001 From: Alan Paxton Date: Fri, 11 Oct 2024 15:48:30 +0100 Subject: [PATCH] Experiment with RocksDB object owning iterators MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In an analogous way to owning column family handles, and closing them when the DB itself is closed, we make iterators be “child” objects of the DB. Iterator lifecycle is generally shorter than DB, so we give them a lambda to remove themselves from the DB on iterator close(). Add a few tests to check that iterators which remain open when the DB is requested to close, are now closed when the DB is closed. TODO - other DB subclasses need to add iterators to the list TODO - there are cases where isOwningHandle() should not throw an exception; we need a less sledgehammer way of implementing this TODO - fix the current Java tests TODO - the atomic improvement in https://github.com/facebook/rocksdb/issues/5234 --- .../AbstractImmutableNativeReference.java | 5 +- .../org/rocksdb/AbstractRocksIterator.java | 18 ++++- java/src/main/java/org/rocksdb/RocksDB.java | 37 ++++++--- .../main/java/org/rocksdb/RocksIterator.java | 5 ++ .../org/rocksdb/IteratorClosedDBTest.java | 79 +++++++++++++++++++ 5 files changed, 133 insertions(+), 11 deletions(-) create mode 100644 java/src/test/java/org/rocksdb/IteratorClosedDBTest.java 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(); + } + } +}