From 56ef2caaa5ebfe855a4b8d76eec4d33cf04e731d Mon Sep 17 00:00:00 2001
From: fyrz <fyrgoss@gmail.com>
Date: Mon, 27 Oct 2014 20:55:57 +0100
Subject: [PATCH] [RocksJava] - Hardening RocksIterator

RocksIterator will sometimes Sigsegv on dispose. Mainly thats related
to dispose order. If the related RocksDB instance is freed beforehand
RocksIterator.dispose() will fail.

Within this commit there is a major change to RocksIterator. RocksIterator
will hold a private reference to the RocksDB instance which created the
RocksIterator. So even if RocksDB is freed in the same GC cycle the
RocksIterator instances will be freed prior to related RocksDB instances.

Another aspect targets the dispose logic if the RocksDB is freed previously
and already gc`ed. On dispose of a RocksIterator the dispose logic will check
if the RocksDB instance points to an initialized DB. If not the dispose logic
will not perform any further action.

The crash can be reproduced by using the related test provided within this
commit.

Related information: This relates to @adamretter`s facebook rocksdb-dev group
post about SigSegv on RocksIterator.dispose().
---
 java/Makefile                                |  1 +
 java/org/rocksdb/RocksDB.java                |  7 +--
 java/org/rocksdb/RocksIterator.java          |  9 +++-
 java/org/rocksdb/test/RocksIteratorTest.java | 48 ++++++++++++++++++++
 4 files changed, 60 insertions(+), 5 deletions(-)
 create mode 100644 java/org/rocksdb/test/RocksIteratorTest.java

diff --git a/java/Makefile b/java/Makefile
index d490da4e5..8c147f9b4 100644
--- a/java/Makefile
+++ b/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
diff --git a/java/org/rocksdb/RocksDB.java b/java/org/rocksdb/RocksDB.java
index 676f636d4..ca26a596d 100644
--- a/java/org/rocksdb/RocksDB.java
+++ b/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;
   }
diff --git a/java/org/rocksdb/RocksIterator.java b/java/org/rocksdb/RocksIterator.java
index 2adff26cc..ddaddbf95 100644
--- a/java/org/rocksdb/RocksIterator.java
+++ b/java/org/rocksdb/RocksIterator.java
@@ -19,9 +19,10 @@ 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_ = rocksDB;
   }
 
   /**
@@ -129,7 +130,9 @@ public class RocksIterator extends RocksObject {
    */
   @Override protected void disposeInternal() {
     assert(isInitialized());
-    disposeInternal(nativeHandle_);
+    if (rocksDB_.isInitialized()) {
+      disposeInternal(nativeHandle_);
+    }
   }
 
   private native boolean isValid0(long handle);
@@ -142,4 +145,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_;
 }
diff --git a/java/org/rocksdb/test/RocksIteratorTest.java b/java/org/rocksdb/test/RocksIteratorTest.java
new file mode 100644
index 000000000..1e2fa8c6d
--- /dev/null
+++ b/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);
+    }
+  }
+}