LUCENE-6745: IndexInput.clone is not thread-safe; fix BKD/RangeTree t…

…o respect that

git-svn-id: https://svn.apache.org/repos/asf/lucene/dev/trunk@1697010 13f79535-47bb-0310-9956-ffa450edef68

lucene/CHANGES.txt

@@ -79,8 +79,6 @@ Bug Fixes
   test data.  In addition, the performance of those filters was improved
   significantly.  (Uwe Schindler, Robert Muir)
 
-* LUCENE-6745: RAMInputStream.clone was not thread safe (Mike McCandless)
-
 * LUCENE-6748: UsageTrackingQueryCachingPolicy no longer caches trivial queries
   like MatchAllDocsQuery. (Adrien Grand)
 

lucene/core/src/java/org/apache/lucene/store/IndexInput.java

@@ -83,6 +83,10 @@ public String toString() {
    * 
    * <p>If you access the cloned IndexInput after closing the original object,
    * any <code>readXXX</code> methods will throw {@link AlreadyClosedException}.
+   *
+   * <p>This method is NOT thread safe, so if the current {@code IndexInput}
+   * is being used by one thread while {@code clone} is called by another,
+   * disaster could strike.
    */
   @Override
   public IndexInput clone() {

lucene/core/src/java/org/apache/lucene/store/RAMInputStream.java

@@ -157,19 +157,4 @@ public IndexInput slice(String sliceDescription, long ofs, long len) throws IOEx
       }
     };
   }
-
-  @Override
-  public RAMInputStream clone() {
-    RAMInputStream clone = (RAMInputStream) super.clone();
-    // If another thread was using our instance, this new clone could have a mismatched currentBuffer and currentBufferIndex, so we do
-    // a "fresh seek" here:
-    clone.currentBuffer = null;
-    try {
-      clone.seek(getFilePointer());
-    } catch (IOException ioe) {
-      // Should not happen!
-      throw new AssertionError(ioe);
-    }
-    return clone;
-  }
 }

lucene/sandbox/src/java/org/apache/lucene/bkdtree/BKDTreeDocValuesProducer.java

@@ -99,8 +99,13 @@ public synchronized SortedNumericDocValues getSortedNumeric(FieldInfo field) thr
       if (fp == null) {
         throw new IllegalArgumentException("this field was not indexed as a BKDPointField");
       }
-      datIn.seek(fp);
-      treeReader = new BKDTreeReader(datIn, maxDoc);
+
+      // LUCENE-6697: never do real IOPs with the original IndexInput because search
+      // threads can be concurrently cloning it:
+      IndexInput clone = datIn.clone();
+
+      clone.seek(fp);
+      treeReader = new BKDTreeReader(clone, maxDoc);
 
       // Only hang onto the reader when we are not merging:
       if (merging == false) {

lucene/sandbox/src/java/org/apache/lucene/rangetree/RangeTreeDocValuesProducer.java

@@ -99,8 +99,12 @@ public synchronized SortedNumericDocValues getSortedNumeric(FieldInfo field) thr
       // FieldInfos checks has already ensured we are a DV field of this type, and Codec ensures
       // this DVFormat was used at write time:
       assert fp != null;
-      datIn.seek(fp);
-      treeReader = new RangeTreeReader(datIn);
+
+      // LUCENE-6697: never do real IOPs with the original IndexInput because search
+      // threads can be concurrently cloning it:
+      IndexInput clone = datIn.clone();
+      clone.seek(fp);
+      treeReader = new RangeTreeReader(clone);
 
       // Only hang onto the reader when we are not merging:
       if (merging == false) {
@@ -148,9 +152,11 @@ public synchronized SortedSetDocValues getSortedSet(FieldInfo field) throws IOEx
       // this DVFormat was used at write time:
       assert fp != null;
 
-      datIn.seek(fp);
-      //System.out.println("load field=" + field.name);
-      treeReader = new RangeTreeReader(datIn);
+      // LUCENE-6697: never do real IOPs with the original IndexInput because search
+      // threads can be concurrently cloning it:
+      IndexInput clone = datIn.clone();
+      clone.seek(fp);
+      treeReader = new RangeTreeReader(clone);
 
       // Only hang onto the reader when we are not merging:
       if (merging == false) {

lucene/test-framework/src/java/org/apache/lucene/store/BaseDirectoryTestCase.java

@@ -1167,59 +1167,5 @@ public void testDoubleCloseInput() throws Throwable {
     in.close(); // close again
     dir.close();
   }
-
-  public void testCloneThreadSafety() throws Exception {
-    Directory dir = getDirectory(createTempDir());
-    IndexOutput out = dir.createOutput("randombytes", IOContext.DEFAULT);
-
-    // Write file with at least 20 K random bytes:
-    final int numBytes = atLeast(20*1024);
-    final byte[] bytes = new byte[numBytes];
-    random().nextBytes(bytes);
-    out.writeBytes(bytes, 0, bytes.length);
-    out.close();
-
-    // Then read the bytes back at random seek points from multiple threads:
-    final IndexInput in = dir.openInput("randombytes", IOContext.DEFAULT);
-
-    int numThreads = 4;
-    Thread[] threads = new Thread[numThreads];
-    for(int i=0;i<numThreads;i++) {
-      int finalI = i;
-      threads[i] = new Thread() {
-          @Override
-          public void run() {
-            int numIters = atLeast(1000);
-            byte[] scratch = new byte[numBytes];
-            for(int iter=0;iter<numIters;iter++) {
-              // First thread uses the original IndexInput, all other threads use clone:
-              IndexInput myIn;
-              if (finalI == 0) {
-                myIn = in;
-              } else {
-                myIn = in.clone();
-              }
-              int spot = random().nextInt(numBytes/2);
-              try {
-                myIn.seek(spot);
-                int length = numBytes-spot;
-                myIn.readBytes(scratch, 0, length);
-                for(int i=0;i<length;i++) {
-                  assertEquals(bytes[spot+i], scratch[i]);
-                }
-              } catch (IOException ioe) {
-                throw new RuntimeException(ioe);
-              }
-            }
-          }
-        };
-      threads[i].start();
-    }
-
-    for(Thread thread : threads) {
-      thread.join();
-    }
-    in.close();
-    dir.close();
-  }
 }
+