From 932fbfedc4ea17c12b1ba23c45bbdb264485ce1c Mon Sep 17 00:00:00 2001 From: Alan Paxton Date: Fri, 11 Oct 2024 15:48:30 +0100 Subject: [PATCH 1/7] 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(); + } + } +} From b1b75e3005416137b3153d8dff7c9b71c1ac9061 Mon Sep 17 00:00:00 2001 From: Alan Paxton Date: Mon, 11 Nov 2024 12:41:53 +0000 Subject: [PATCH 2/7] Clean up tests for iterator automatic closure --- java/CMakeLists.txt | 2 + java/Makefile | 1 + .../AbstractImmutableNativeReference.java | 5 +-- .../org/rocksdb/AbstractRocksIterator.java | 12 +++--- .../org/rocksdb/IteratorClosedDBTest.java | 41 +++++++------------ 5 files changed, 26 insertions(+), 35 deletions(-) diff --git a/java/CMakeLists.txt b/java/CMakeLists.txt index a60847ead37..7d530f37f17 100644 --- a/java/CMakeLists.txt +++ b/java/CMakeLists.txt @@ -348,6 +348,7 @@ set(JAVA_TEST_CLASSES src/test/java/org/rocksdb/ImportColumnFamilyTest.java src/test/java/org/rocksdb/InfoLogLevelTest.java src/test/java/org/rocksdb/IngestExternalFileOptionsTest.java + src/test/java/org/rocksdb/IteratorClosedDBTest.java src/test/java/org/rocksdb/KeyExistsTest.java src/test/java/org/rocksdb/KeyMayExistTest.java src/test/java/org/rocksdb/LRUCacheTest.java @@ -466,6 +467,7 @@ set(JAVA_TEST_RUNNING_CLASSES org.rocksdb.ImportColumnFamilyTest org.rocksdb.InfoLogLevelTest org.rocksdb.IngestExternalFileOptionsTest + org.rocksdb.IteratorClosedDBTest org.rocksdb.KeyExistsTest org.rocksdb.KeyMayExistTest org.rocksdb.LRUCacheTest diff --git a/java/Makefile b/java/Makefile index 5e00921c62b..78df9a3df10 100644 --- a/java/Makefile +++ b/java/Makefile @@ -138,6 +138,7 @@ JAVA_TESTS = \ org.rocksdb.EnvOptionsTest\ org.rocksdb.EventListenerTest\ org.rocksdb.IngestExternalFileOptionsTest\ + org.rocksdb.IteratorClosedDBTest\ org.rocksdb.util.IntComparatorTest\ org.rocksdb.util.JNIComparatorTest\ org.rocksdb.FilterTest\ diff --git a/java/src/main/java/org/rocksdb/AbstractImmutableNativeReference.java b/java/src/main/java/org/rocksdb/AbstractImmutableNativeReference.java index 66f3f2ec157..173d63e9011 100644 --- a/java/src/main/java/org/rocksdb/AbstractImmutableNativeReference.java +++ b/java/src/main/java/org/rocksdb/AbstractImmutableNativeReference.java @@ -28,10 +28,7 @@ protected AbstractImmutableNativeReference(final boolean owningHandle) { @Override public boolean isOwningHandle() { - if (!owningHandle_.get()) { - throw new RuntimeException("Object does not own its native handle."); - } - return true; + return owningHandle_.get(); } /** diff --git a/java/src/main/java/org/rocksdb/AbstractRocksIterator.java b/java/src/main/java/org/rocksdb/AbstractRocksIterator.java index fed91988832..b9076eddb4c 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.concurrent.atomic.AtomicReference; import java.util.function.Function; /** @@ -20,12 +21,12 @@ * @param

The type of the Parent Object from which the Rocks Iterator was * created. This is used by disposeInternal to avoid double-free * issues with the underlying C++ object. - * @see org.rocksdb.RocksObject + * @see RocksObject */ public abstract class AbstractRocksIterator

extends RocksObject implements RocksIteratorInterface { final P parent_; - final Function, Boolean> removeOnClosure_; + final AtomicReference, Boolean>> removeOnClosure_ = new AtomicReference<>(); protected AbstractRocksIterator(final P parent, final long nativeHandle, final Function, Boolean> removeOnClosure) { @@ -36,7 +37,7 @@ 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; + removeOnClosure_.set(removeOnClosure); } protected AbstractRocksIterator(final P parent, @@ -131,8 +132,9 @@ public void status() throws RocksDBException { @Override public void close() { super.close(); - if (removeOnClosure_ != null) { - removeOnClosure_.apply(this); + Function, Boolean> removeOnClosure = removeOnClosure_.getAndSet(null); + if (removeOnClosure != null) { + removeOnClosure.apply(this); } } diff --git a/java/src/test/java/org/rocksdb/IteratorClosedDBTest.java b/java/src/test/java/org/rocksdb/IteratorClosedDBTest.java index db3d3a6fa3f..4b1f6a9b32d 100644 --- a/java/src/test/java/org/rocksdb/IteratorClosedDBTest.java +++ b/java/src/test/java/org/rocksdb/IteratorClosedDBTest.java @@ -5,8 +5,6 @@ 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 { @@ -18,25 +16,6 @@ public class IteratorClosedDBTest { @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())) { @@ -52,11 +31,11 @@ public void ownedIterators() throws RocksDBException { it.next(); assertThat(it.isValid()).isFalse(); - } //iterator is still open when we close the DB, C++ assertion in DEBUG_LEVEL=1 + } //if iterator were still open when we close the DB, we would see a C++ assertion in DEBUG_LEVEL=1 } @Test - public void shouldCrashJavaRocks() throws RocksDBException { + public void shouldNotCrashJavaRocks() throws RocksDBException { try (Options options = new Options().setCreateIfMissing(true)) { RocksDB db = RocksDB.open(options, dbFolder.getRoot().getAbsolutePath()); byte[] key = {0x1}; @@ -68,11 +47,21 @@ public void shouldCrashJavaRocks() throws RocksDBException { 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 + // Close should work because iterator references are now cleaned up + // Previously would have thrown an exception here (assertion failure in C++) - + // when built with DEBUG_LEVEL=1 + // Because the outstanding iterator has a reference to the column family which is being closed db.close(); - assertThat(it.isValid()).isFalse(); + // should assert + try { + boolean isValidShouldAssert = it.isValid(); + throw new RuntimeException("it.isValid() should cause an assertion"); + } catch (AssertionError ignored) { + } + + // Multiple close() should be fine/no-op + it.close(); it.close(); } } From 3f2aeacc923f82750e3e42dbd98e6482d480d056 Mon Sep 17 00:00:00 2001 From: Alan Paxton Date: Mon, 11 Nov 2024 13:28:19 +0000 Subject: [PATCH 3/7] fix format --- .../org/rocksdb/AbstractRocksIterator.java | 10 +- java/src/main/java/org/rocksdb/RocksDB.java | 15 +-- .../main/java/org/rocksdb/RocksIterator.java | 3 +- .../org/rocksdb/IteratorClosedDBTest.java | 97 +++++++++---------- 4 files changed, 63 insertions(+), 62 deletions(-) diff --git a/java/src/main/java/org/rocksdb/AbstractRocksIterator.java b/java/src/main/java/org/rocksdb/AbstractRocksIterator.java index b9076eddb4c..f7b3916034f 100644 --- a/java/src/main/java/org/rocksdb/AbstractRocksIterator.java +++ b/java/src/main/java/org/rocksdb/AbstractRocksIterator.java @@ -26,10 +26,11 @@ public abstract class AbstractRocksIterator

extends RocksObject implements RocksIteratorInterface { final P parent_; - final AtomicReference, Boolean>> removeOnClosure_ = new AtomicReference<>(); + final AtomicReference, Boolean>> removeOnClosure_ = + new AtomicReference<>(); - protected AbstractRocksIterator(final P parent, - final long nativeHandle, final Function, Boolean> removeOnClosure) { + protected AbstractRocksIterator(final P parent, final long nativeHandle, + final Function, Boolean> removeOnClosure) { super(nativeHandle); // parent must point to a valid RocksDB instance. assert (parent != null); @@ -40,8 +41,7 @@ protected AbstractRocksIterator(final P parent, removeOnClosure_.set(removeOnClosure); } - protected AbstractRocksIterator(final P parent, - final long nativeHandle) { + protected AbstractRocksIterator(final P parent, final long nativeHandle) { this(parent, nativeHandle, null); } diff --git a/java/src/main/java/org/rocksdb/RocksDB.java b/java/src/main/java/org/rocksdb/RocksDB.java index 530be1d5501..a8b8919c3a6 100644 --- a/java/src/main/java/org/rocksdb/RocksDB.java +++ b/java/src/main/java/org/rocksdb/RocksDB.java @@ -678,7 +678,7 @@ public void closeE() throws RocksDBException { 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 + // The lambda supplied to rocksIterator on construction will remove it from ownedIterators rocksIterator.close(); } @@ -3271,7 +3271,7 @@ public KeyMayExist keyMayExist(final ColumnFamilyHandle columnFamilyHandle, */ public RocksIterator newIterator() { return makeIterator(iterator(nativeHandle_, defaultColumnFamilyHandle_.nativeHandle_, - defaultReadOptions_.nativeHandle_)); + defaultReadOptions_.nativeHandle_)); } /** @@ -3308,9 +3308,8 @@ public RocksIterator newIterator(final ReadOptions readOptions) { */ public RocksIterator newIterator( final ColumnFamilyHandle columnFamilyHandle) { - return makeIterator( - iterator( - nativeHandle_, columnFamilyHandle.nativeHandle_, defaultReadOptions_.nativeHandle_)); + return makeIterator(iterator( + nativeHandle_, columnFamilyHandle.nativeHandle_, defaultReadOptions_.nativeHandle_)); } /** @@ -3330,7 +3329,8 @@ public RocksIterator newIterator( */ public RocksIterator newIterator(final ColumnFamilyHandle columnFamilyHandle, final ReadOptions readOptions) { - return makeIterator(iterator(nativeHandle_, columnFamilyHandle.nativeHandle_, readOptions.nativeHandle_)); + return makeIterator( + iterator(nativeHandle_, columnFamilyHandle.nativeHandle_, readOptions.nativeHandle_)); } /** @@ -4759,7 +4759,8 @@ public void deleteFilesInRanges(final ColumnFamilyHandle columnFamily, final Lis * @return the wrapped iterator */ private RocksIterator makeIterator(final long nativeIterator) { - final RocksIterator rocksIterator = new RocksIterator(this, nativeIterator, ownedIterators::remove); + final RocksIterator rocksIterator = + new RocksIterator(this, nativeIterator, ownedIterators::remove); ownedIterators.add(rocksIterator); return rocksIterator; diff --git a/java/src/main/java/org/rocksdb/RocksIterator.java b/java/src/main/java/org/rocksdb/RocksIterator.java index c93f85ebd60..227791c175c 100644 --- a/java/src/main/java/org/rocksdb/RocksIterator.java +++ b/java/src/main/java/org/rocksdb/RocksIterator.java @@ -28,7 +28,8 @@ protected RocksIterator(final RocksDB rocksDB, final long nativeHandle) { super(rocksDB, nativeHandle); } - protected RocksIterator(final RocksDB rocksDB, final long nativeHandle, final Function, Boolean> removeOnClosure) { + protected RocksIterator(final RocksDB rocksDB, final long nativeHandle, + final Function, Boolean> removeOnClosure) { super(rocksDB, nativeHandle, removeOnClosure); } diff --git a/java/src/test/java/org/rocksdb/IteratorClosedDBTest.java b/java/src/test/java/org/rocksdb/IteratorClosedDBTest.java index 4b1f6a9b32d..cd4fa8fc615 100644 --- a/java/src/test/java/org/rocksdb/IteratorClosedDBTest.java +++ b/java/src/test/java/org/rocksdb/IteratorClosedDBTest.java @@ -1,68 +1,67 @@ package org.rocksdb; +import static org.assertj.core.api.Assertions.assertThat; + import org.junit.ClassRule; import org.junit.Rule; import org.junit.Test; import org.junit.rules.TemporaryFolder; -import static org.assertj.core.api.Assertions.assertThat; - public class IteratorClosedDBTest { + @ClassRule + public static final RocksNativeLibraryResource ROCKS_NATIVE_LIBRARY_RESOURCE = + new RocksNativeLibraryResource(); - @ClassRule - public static final RocksNativeLibraryResource ROCKS_NATIVE_LIBRARY_RESOURCE = - new RocksNativeLibraryResource(); + @Rule public TemporaryFolder dbFolder = new TemporaryFolder(); - @Rule - public TemporaryFolder dbFolder = new TemporaryFolder(); + @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); - @Test - public void ownedIterators() throws RocksDBException { - try (Options options = new Options().setCreateIfMissing(true); RocksDB db = RocksDB.open(options, dbFolder.getRoot().getAbsolutePath())) { + RocksIterator it = db.newIterator(); + it.seekToFirst(); + assertThat(it.key()).isEqualTo(key); + assertThat(it.value()).isEqualTo(value); - 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(); - } //if iterator were still open when we close the DB, we would see a C++ assertion in DEBUG_LEVEL=1 - } + it.next(); + assertThat(it.isValid()).isFalse(); + } // if iterator were still open when we close the DB, we would see a C++ assertion in + // DEBUG_LEVEL=1 + } - @Test - public void shouldNotCrashJavaRocks() 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); + @Test + public void shouldNotCrashJavaRocks() 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(); + RocksIterator it = db.newIterator(); + assertThat(it.isValid()).isFalse(); + it.seekToFirst(); + assertThat(it.isValid()).isTrue(); - // Close should work because iterator references are now cleaned up - // Previously would have thrown an exception here (assertion failure in C++) - - // when built with DEBUG_LEVEL=1 - // Because the outstanding iterator has a reference to the column family which is being closed - db.close(); + // Close should work because iterator references are now cleaned up + // Previously would have thrown an exception here (assertion failure in C++) - + // when built with DEBUG_LEVEL=1 + // Because the outstanding iterator has a reference to the column family which is being closed + db.close(); - // should assert - try { - boolean isValidShouldAssert = it.isValid(); - throw new RuntimeException("it.isValid() should cause an assertion"); - } catch (AssertionError ignored) { - } + // should assert + try { + boolean isValidShouldAssert = it.isValid(); + throw new RuntimeException("it.isValid() should cause an assertion"); + } catch (AssertionError ignored) { + } - // Multiple close() should be fine/no-op - it.close(); - it.close(); - } + // Multiple close() should be fine/no-op + it.close(); + it.close(); } + } } From 06608ad4cb683dea37b6499f0eb2207676086d57 Mon Sep 17 00:00:00 2001 From: Alan Paxton Date: Mon, 11 Nov 2024 14:21:30 +0000 Subject: [PATCH 4/7] Fix pmd report upload ? --- .github/workflows/pr-jobs.yml | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/.github/workflows/pr-jobs.yml b/.github/workflows/pr-jobs.yml index 9d89a111e5a..d4362cbe581 100644 --- a/.github/workflows/pr-jobs.yml +++ b/.github/workflows/pr-jobs.yml @@ -613,13 +613,15 @@ jobs: - name: PMD RocksDBJava run: make V=1 J=8 -j8 jpmd - uses: actions/upload-artifact@v4.0.0 + if: failure() with: name: pmd-report - path: "${{ github.workspace }}/java/target/pmd.xml" + path: java/target/pmd.xml - uses: actions/upload-artifact@v4.0.0 + if: failure() with: name: maven-site - path: "${{ github.workspace }}/java/target/site" + path: java/target/site build-linux-arm: if: ${{ github.repository_owner == 'facebook' }} runs-on: From 81d5409c5d51895660b76fb42fc6d683e3c9cf60 Mon Sep 17 00:00:00 2001 From: Alan Paxton Date: Mon, 11 Nov 2024 17:39:45 +0000 Subject: [PATCH 5/7] Suppress misleading PMD --- java/src/main/java/org/rocksdb/RocksDB.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/src/main/java/org/rocksdb/RocksDB.java b/java/src/main/java/org/rocksdb/RocksDB.java index a8b8919c3a6..2b9aa1c1730 100644 --- a/java/src/main/java/org/rocksdb/RocksDB.java +++ b/java/src/main/java/org/rocksdb/RocksDB.java @@ -673,7 +673,7 @@ public void closeE() throws RocksDBException { *

* See also {@link #close()}. */ - @SuppressWarnings("PMD.EmptyCatchBlock") + @SuppressWarnings("PMD.EmptyCatchBlock","PMD.CloseResource") @Override public void close() { final List removeIterators = new ArrayList<>(ownedIterators); From 2c068ff373e6a25230e275419d4c4acd977c2729 Mon Sep 17 00:00:00 2001 From: Alan Paxton Date: Tue, 19 Nov 2024 16:42:09 +0000 Subject: [PATCH 6/7] Correct declaration of multiple PMD suppressions --- java/src/main/java/org/rocksdb/RocksDB.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/src/main/java/org/rocksdb/RocksDB.java b/java/src/main/java/org/rocksdb/RocksDB.java index 2b9aa1c1730..e7e59d0bcaf 100644 --- a/java/src/main/java/org/rocksdb/RocksDB.java +++ b/java/src/main/java/org/rocksdb/RocksDB.java @@ -673,7 +673,7 @@ public void closeE() throws RocksDBException { *

* See also {@link #close()}. */ - @SuppressWarnings("PMD.EmptyCatchBlock","PMD.CloseResource") + @SuppressWarnings({"PMD.EmptyCatchBlock", "PMD.CloseResource"}) @Override public void close() { final List removeIterators = new ArrayList<>(ownedIterators); From 7aa0770e11e41487b1a6dd36d88150998225f30a Mon Sep 17 00:00:00 2001 From: Alan Paxton Date: Wed, 20 Nov 2024 10:11:30 +0000 Subject: [PATCH 7/7] A bit more test of Java API on closed iterators --- java/src/test/java/org/rocksdb/IteratorClosedDBTest.java | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/java/src/test/java/org/rocksdb/IteratorClosedDBTest.java b/java/src/test/java/org/rocksdb/IteratorClosedDBTest.java index cd4fa8fc615..db6ad635755 100644 --- a/java/src/test/java/org/rocksdb/IteratorClosedDBTest.java +++ b/java/src/test/java/org/rocksdb/IteratorClosedDBTest.java @@ -46,12 +46,21 @@ public void shouldNotCrashJavaRocks() throws RocksDBException { it.seekToFirst(); assertThat(it.isValid()).isTrue(); + byte[] valueOK = it.value(); + assertThat(valueOK).isEqualTo(value); + // Close should work because iterator references are now cleaned up // Previously would have thrown an exception here (assertion failure in C++) - // when built with DEBUG_LEVEL=1 // Because the outstanding iterator has a reference to the column family which is being closed db.close(); + try { + byte[] valueShouldAssert = it.value(); + throw new RuntimeException("it.value() should cause an assertion"); + } catch (AssertionError ignored) { + } + // should assert try { boolean isValidShouldAssert = it.isValid();