Skip to content

Commit

Permalink
Experiment with RocksDB object owning iterators
Browse files Browse the repository at this point in the history
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 facebook#5234
  • Loading branch information
alanpaxton committed Dec 2, 2024
1 parent 6f9d826 commit 932fbfe
Show file tree
Hide file tree
Showing 5 changed files with 133 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

/**
Expand Down
18 changes: 17 additions & 1 deletion java/src/main/java/org/rocksdb/AbstractRocksIterator.java
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
package org.rocksdb;

import java.nio.ByteBuffer;
import java.util.function.Function;

/**
* Base class implementation for Rocks Iterators
Expand All @@ -24,16 +25,23 @@
public abstract class AbstractRocksIterator<P extends RocksObject>
extends RocksObject implements RocksIteratorInterface {
final P parent_;
final Function<AbstractRocksIterator<P>, Boolean> removeOnClosure_;

protected AbstractRocksIterator(final P parent,
final long nativeHandle) {
final long nativeHandle, final Function<AbstractRocksIterator<P>, Boolean> removeOnClosure) {
super(nativeHandle);
// parent must point to a valid RocksDB instance.
assert (parent != null);
// RocksIterator must hold a reference to the related parent instance
// 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
Expand Down Expand Up @@ -120,6 +128,14 @@ public void status() throws RocksDBException {
status0(nativeHandle_);
}

@Override
public void close() {
super.close();
if (removeOnClosure_ != null) {
removeOnClosure_.apply(this);
}
}

/**
* <p>Deletes underlying C++ iterator pointer.</p>
*
Expand Down
37 changes: 28 additions & 9 deletions java/src/main/java/org/rocksdb/RocksDB.java
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ private enum LibraryState {

final List<ColumnFamilyHandle> ownedColumnFamilyHandles = new ArrayList<>();

final List<RocksIterator> ownedIterators = new ArrayList<>();

/**
* Loads the necessary library files.
* Calling this method twice will have no effect.
Expand Down Expand Up @@ -674,6 +676,12 @@ public void closeE() throws RocksDBException {
@SuppressWarnings("PMD.EmptyCatchBlock")
@Override
public void close() {
final List<RocksIterator> 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();
Expand Down Expand Up @@ -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_));
}

Expand All @@ -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_));
}

/**
Expand All @@ -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_));
}
Expand All @@ -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_));
}

/**
Expand Down Expand Up @@ -3376,7 +3381,7 @@ public List<RocksIterator> newIterators(
final List<RocksIterator> iterators = new ArrayList<>(
columnFamilyHandleList.size());
for (int i=0; i<columnFamilyHandleList.size(); i++){
iterators.add(new RocksIterator(this, iteratorRefs[i]));
iterators.add(makeIterator(iteratorRefs[i]));
}
return iterators;
}
Expand Down Expand Up @@ -4746,6 +4751,20 @@ public void deleteFilesInRanges(final ColumnFamilyHandle columnFamily, final Lis
rangesArray, includeEnd);
}

/**
* Wrap a (newly created) native iterator in a RocksIterator,
* and record it in our list of owned iterators for consistent removal prior to db close.
*
* @param nativeIterator the native iterator to wrap
* @return the wrapped iterator
*/
private RocksIterator makeIterator(final long nativeIterator) {
final RocksIterator rocksIterator = new RocksIterator(this, nativeIterator, ownedIterators::remove);
ownedIterators.add(rocksIterator);

return rocksIterator;
}

/**
* Static method to destroy the contents of the specified database.
* Be very careful using this method.
Expand Down
5 changes: 5 additions & 0 deletions java/src/main/java/org/rocksdb/RocksIterator.java
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import static org.rocksdb.util.BufferUtil.CheckBounds;

import java.nio.ByteBuffer;
import java.util.function.Function;

/**
* <p>An iterator that yields a sequence of key/value pairs from a source.
Expand All @@ -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<AbstractRocksIterator<RocksDB>, Boolean> removeOnClosure) {
super(rocksDB, nativeHandle, removeOnClosure);
}

/**
* <p>Return the key for the current entry. The underlying storage for
* the returned slice is valid only until the next modification of
Expand Down
79 changes: 79 additions & 0 deletions java/src/test/java/org/rocksdb/IteratorClosedDBTest.java
Original file line number Diff line number Diff line change
@@ -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();
}
}
}

0 comments on commit 932fbfe

Please sign in to comment.