Merge pull request #368 from fyrz/RocksJava-Hardening-RocksIterator

[RocksJava] - Hardening RocksIterator

java/Makefile

@@ -46,6 +46,7 @@ test: java
 	java -ea -Djava.library.path=.:../ -cp "$(ROCKSDB_JAR):.:./*" org.rocksdb.test.OptionsTest
 	java -ea -Djava.library.path=.:../ -cp "$(ROCKSDB_JAR):.:./*" org.rocksdb.test.ReadOnlyTest
 	java -ea -Djava.library.path=.:../ -cp "$(ROCKSDB_JAR):.:./*" org.rocksdb.test.ReadOptionsTest
+	java -ea -Djava.library.path=.:../ -cp "$(ROCKSDB_JAR):.:./*" org.rocksdb.test.RocksIteratorTest
 	java -ea -Djava.library.path=.:../ -cp "$(ROCKSDB_JAR):.:./*" org.rocksdb.test.SnapshotTest
 	java -ea -Djava.library.path=.:../ -cp "$(ROCKSDB_JAR):.:./*" org.rocksdb.test.StatisticsCollectorTest
 	java -ea -Djava.library.path=.:../ -cp "$(ROCKSDB_JAR):.:./*" org.rocksdb.test.ComparatorOptionsTest

java/org/rocksdb/RocksDB.java

@@ -888,7 +888,7 @@ public class RocksDB extends RocksObject {
    * @return instance of iterator object.
    */
   public RocksIterator newIterator() {
-    return new RocksIterator(iterator0(nativeHandle_));
+    return new RocksIterator(this, iterator0(nativeHandle_));
   }
 
 
@@ -936,7 +936,8 @@ public class RocksDB extends RocksObject {
    * @return instance of iterator object.
    */
   public RocksIterator newIterator(ColumnFamilyHandle columnFamilyHandle) {
-    return new RocksIterator(iterator0(nativeHandle_, columnFamilyHandle.nativeHandle_));
+    return new RocksIterator(this, iterator0(nativeHandle_,
+        columnFamilyHandle.nativeHandle_));
   }
 
   /**
@@ -958,7 +959,7 @@ public class RocksDB extends RocksObject {
 
     long[] iteratorRefs = iterators(nativeHandle_, columnFamilyHandleList);
     for (int i=0; i<columnFamilyHandleList.size(); i++){
-      iterators.add(new RocksIterator(iteratorRefs[i]));
+      iterators.add(new RocksIterator(this, iteratorRefs[i]));
     }
     return iterators;
   }

java/org/rocksdb/RocksIterator.java

@@ -19,9 +19,15 @@ package org.rocksdb;
  * @see org.rocksdb.RocksObject
  */
 public class RocksIterator extends RocksObject {
-  public RocksIterator(long nativeHandle) {
+  public RocksIterator(RocksDB rocksDB, long nativeHandle) {
     super();
     nativeHandle_ = nativeHandle;
+    // rocksDB must point to a valid RocksDB instance.
+    assert(rocksDB);
+    // RocksIterator must hold a reference to the related RocksDB instance
+    // to guarantee that while a GC cycle starts RocksDBIterator instances
+    // are freed prior to RocksDB instances.
+    rocksDB_ = rocksDB;
   }
 
   /**
@@ -125,12 +131,19 @@ public class RocksIterator extends RocksObject {
   }
 
   /**
-   * Deletes underlying C++ iterator pointer.
+   * <p>Deletes underlying C++ iterator pointer.</p>
+   *
+   * <p>Note: the underlying handle can only be safely deleted if the RocksDB
+   * instance related to a certain RocksIterator is still valid and initialized.
+   * Therefore {@code disposeInternal()} checks if the RocksDB is initialized
+   * before freeing the native handle.</p>
    */
   @Override protected void disposeInternal() {
     assert(isInitialized());
+    if (rocksDB_.isInitialized()) {
       disposeInternal(nativeHandle_);
     }
+  }
 
   private native boolean isValid0(long handle);
   private native void disposeInternal(long handle);
@@ -142,4 +155,6 @@ public class RocksIterator extends RocksObject {
   private native byte[] value0(long handle);
   private native void seek0(long handle, byte[] target, int targetLen);
   private native void status0(long handle);
+
+  RocksDB rocksDB_;
 }

java/org/rocksdb/test/RocksIteratorTest.java

@@ -0,0 +1,48 @@
+// Copyright (c) 2014, Facebook, Inc.  All rights reserved.
+// This source code is licensed under the BSD-style license found in the
+// LICENSE file in the root directory of this source tree. An additional grant
+// of patent rights can be found in the PATENTS file in the same directory.
+package org.rocksdb.test;
+
+import org.rocksdb.ColumnFamilyHandle;
+import org.rocksdb.Options;
+import org.rocksdb.RocksDB;
+import org.rocksdb.RocksDBException;
+import org.rocksdb.RocksIterator;
+
+import java.util.ArrayList;
+import java.util.List;
+
+public class RocksIteratorTest {
+  static final String DB_PATH = "/tmp/rocksdbjni_iterator_test";
+  static {
+    RocksDB.loadLibrary();
+  }
+
+  public static void main(String[] args){
+    RocksDB db;
+    Options options = new Options();
+    options.setCreateIfMissing(true)
+        .setCreateMissingColumnFamilies(true);
+    try {
+      db = RocksDB.open(options, DB_PATH);
+      db.put("key".getBytes(), "value".getBytes());
+      RocksIterator iter = db.newIterator();
+      RocksIterator iter2 = db.newIterator();
+      RocksIterator iter3 = db.newIterator();
+      iter = null;
+      db.close();
+      db = null;
+      iter2 = null;
+      System.gc();
+      System.runFinalization();
+      System.out.println("Passed RocksIterator Test");
+      iter3.dispose();
+      System.gc();
+      System.runFinalization();
+    }catch (RocksDBException e){
+      e.printStackTrace();
+      assert(false);
+    }
+  }
+}