LUCENE-1453: Remove the complicated closeDir handling from DirectoryReader which no longer "owns" the FSDirectory. A FilterIndexReader, that is wrapped around the DirectoryReader by IndexReader.open(), will handle the closing of the dir. This wrapper and all File/String IndexReader.open()'s will be removed in 3.0.

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

CHANGES.txt

@@ -481,10 +481,12 @@ Bug fixes
 5. LUCENE-1442: Multiple-valued NOT_ANALYZED fields can double-count
    token offsets.  (Mike McCandless)
 
-6. LUCENE-1453: Ensure IndexReader.reopen() does not result in
+6. LUCENE-1453: Ensure IndexReader.reopen()/clone() does not result in
-   incorrectly closing the shared FSDirectory.  This bug would only
+   incorrectly closing the shared FSDirectory. This bug would only
-   happen if you use IndexReader.open with a File or String argument.
+   happen if you use IndexReader.open() with a File or String argument.
-   (Mark Miller via Mike McCandless)
+   The returned readers are wrapped by a FilterIndexReader that
+   correctly handles closing of directory after reopen()/clone(). 
+   (Mark Miller, Uwe Schindler, Mike McCandless)
 
 7. LUCENE-1457: Fix possible overflow bugs during binary
    searches. (Mark Miller via Mike McCandless)

src/java/org/apache/lucene/index/DirectoryOwningReader.java

@@ -0,0 +1,105 @@
+package org.apache.lucene.index;
+
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import java.io.IOException;
+
+/** 
+ * This class keeps track of closing the underlying directory. It is used to wrap
+ * DirectoryReaders, that are created using a String/File parameter
+ * in IndexReader.open() with FSDirectory.getDirectory().
+ * @deprecated This helper class is removed with all String/File
+ * IndexReader.open() methods in Lucene 3.0
+ */
+final class DirectoryOwningReader extends FilterIndexReader implements Cloneable {
+
+  DirectoryOwningReader(final IndexReader in) {
+    super(in);
+    this.ref = new SegmentReader.Ref();
+    assert this.ref.refCount() == 1;
+  }
+
+  private DirectoryOwningReader(final IndexReader in, final SegmentReader.Ref ref) {
+    super(in);
+    this.ref = ref;
+    ref.incRef();
+  }
+
+  public IndexReader reopen() throws CorruptIndexException, IOException {
+    ensureOpen();
+    final IndexReader r = in.reopen();
+    if (r != in)
+      return new DirectoryOwningReader(r, ref);
+    return this;
+  }
+
+  public IndexReader reopen(boolean openReadOnly) throws CorruptIndexException, IOException {
+    ensureOpen();
+    final IndexReader r = in.reopen(openReadOnly);
+    if (r != in)
+      return new DirectoryOwningReader(r, ref);
+    return this;
+  }
+
+  public IndexReader reopen(final IndexCommit commit) throws CorruptIndexException, IOException {
+    ensureOpen();
+    final IndexReader r = in.reopen(commit);
+    if (r != in)
+      return new DirectoryOwningReader(r, ref);
+    return this;
+  }
+
+  public Object clone() {
+    ensureOpen();
+    return new DirectoryOwningReader((IndexReader) in.clone(), ref);
+  }
+
+  public IndexReader clone(boolean openReadOnly) throws CorruptIndexException, IOException {
+    ensureOpen();
+    return new DirectoryOwningReader(in.clone(openReadOnly), ref);
+  }
+
+  protected void doClose() throws IOException {
+    IOException ioe = null;
+    // close the reader, record exception
+    try {
+      super.doClose();
+    } catch (IOException e) {
+      ioe = e;
+    }
+    // close the directory, record exception
+    if (ref.decRef() == 0) {
+      try {
+        in.directory().close();
+      } catch (IOException e) {
+        if (ioe == null) ioe = e;
+      }
+    }
+    // throw the first exception
+    if (ioe != null) throw ioe;
+  }
+
+  /**
+   * This member contains the ref counter, that is passed to each instance after cloning/reopening,
+   * and is global to all DirectoryOwningReader derived from the original one.
+   * This reuses the class {@link SegmentReader.Ref}
+   */
+  private final SegmentReader.Ref ref;
+
+}
+

src/java/org/apache/lucene/index/DirectoryReader.java

@@ -36,7 +36,6 @@ import org.apache.lucene.store.Directory;
 import org.apache.lucene.store.Lock;
 import org.apache.lucene.store.LockObtainFailedException;
 import org.apache.lucene.store.AlreadyClosedException;
-import org.apache.lucene.store.FSDirectory;
 
 /** 
  * An IndexReader which reads indexes with multiple segments.
@@ -44,7 +43,6 @@ import org.apache.lucene.store.FSDirectory;
 class DirectoryReader extends IndexReader implements Cloneable {
   protected Directory directory;
   protected boolean readOnly;
-  protected boolean closeDirectory;
 
   IndexWriter writer;
 
@@ -64,48 +62,23 @@ class DirectoryReader extends IndexReader implements Cloneable {
   private int numDocs = -1;
   private boolean hasDeletions = false;
 
-  static IndexReader open(final Directory directory, final boolean closeDirectory, final IndexDeletionPolicy deletionPolicy, final IndexCommit commit, final boolean readOnly) throws CorruptIndexException, IOException {
+  static IndexReader open(final Directory directory, final IndexDeletionPolicy deletionPolicy, final IndexCommit commit, final boolean readOnly) throws CorruptIndexException, IOException {
-    SegmentInfos.FindSegmentsFile finder = new SegmentInfos.FindSegmentsFile(directory) {
+    return (IndexReader) new SegmentInfos.FindSegmentsFile(directory) {
-
       protected Object doBody(String segmentFileName) throws CorruptIndexException, IOException {
-
         SegmentInfos infos = new SegmentInfos();
         infos.read(directory, segmentFileName);
-
         if (readOnly)
-          return new ReadOnlyDirectoryReader(directory, infos, deletionPolicy, closeDirectory);
+          return new ReadOnlyDirectoryReader(directory, infos, deletionPolicy);
         else
-          return new DirectoryReader(directory, infos, deletionPolicy, closeDirectory, false);
+          return new DirectoryReader(directory, infos, deletionPolicy, false);
       }
-    };
+    }.run(commit);
-
-    IndexReader reader = null;
-    try {
-      reader = (IndexReader) finder.run(commit);
-    } finally {
-      // We passed false above for closeDirectory so that
-      // the directory would not be closed before we were
-      // done retrying, so at this point if we truly failed
-      // to open a reader, which means an exception is being
-      // thrown, then close the directory now:
-      if (reader == null && closeDirectory) {
-        try {
-          directory.close();
-        } catch (IOException ioe) {
-          // suppress, so we keep throwing original failure
-          // from opening the reader
-        }
-      }
-    }
-
-    return reader;
   }
 
   /** Construct reading the named set of readers. */
-  DirectoryReader(Directory directory, SegmentInfos sis, IndexDeletionPolicy deletionPolicy, boolean closeDirectory, boolean readOnly) throws IOException {
+  DirectoryReader(Directory directory, SegmentInfos sis, IndexDeletionPolicy deletionPolicy, boolean readOnly) throws IOException {
     this.directory = directory;
     this.readOnly = readOnly;
-    this.closeDirectory = closeDirectory;
     this.segmentInfos = sis;
     this.deletionPolicy = deletionPolicy;
 
@@ -147,7 +120,6 @@ class DirectoryReader extends IndexReader implements Cloneable {
   DirectoryReader(IndexWriter writer, SegmentInfos infos) throws IOException {
     this.directory = writer.getDirectory();
     this.readOnly = true;
-    this.closeDirectory = false;
     this.segmentInfos = infos;
     if (!readOnly) {
       // We assume that this segments_N was previously
@@ -198,11 +170,10 @@ class DirectoryReader extends IndexReader implements Cloneable {
   }
 
   /** This contructor is only used for {@link #reopen()} */
-  DirectoryReader(Directory directory, SegmentInfos infos, boolean closeDirectory, SegmentReader[] oldReaders, int[] oldStarts,
+  DirectoryReader(Directory directory, SegmentInfos infos, SegmentReader[] oldReaders, int[] oldStarts,
                      Map oldNormsCache, boolean readOnly, boolean doClone) throws IOException {
     this.directory = directory;
     this.readOnly = readOnly;
-    this.closeDirectory = closeDirectory;
     this.segmentInfos = infos;
     if (!readOnly) {
       // We assume that this segments_N was previously
@@ -347,7 +318,6 @@ class DirectoryReader extends IndexReader implements Cloneable {
     DirectoryReader newReader = doReopen((SegmentInfos) segmentInfos.clone(), true, openReadOnly);
 
     if (this != newReader) {
-      newReader.closeDirectory = closeDirectory;
       newReader.deletionPolicy = deletionPolicy;
     }
     newReader.writer = writer;
@@ -445,54 +415,21 @@ class DirectoryReader extends IndexReader implements Cloneable {
       }
     }
 
-    final SegmentInfos.FindSegmentsFile finder = new SegmentInfos.FindSegmentsFile(directory) {
+    return (IndexReader) new SegmentInfos.FindSegmentsFile(directory) {
-
       protected Object doBody(String segmentFileName) throws CorruptIndexException, IOException {
         SegmentInfos infos = new SegmentInfos();
         infos.read(directory, segmentFileName);
         return doReopen(infos, false, openReadOnly);
       }
-    };
+    }.run(commit);
-
-    DirectoryReader reader = null;
-
-    /* TODO: Remove this in 3.0 - the directory is then
-     * no longer owned by the IndexReader and must not be
-     * closed.
-     * While trying to reopen, we temporarily mark our
-     * closeDirectory as false.  This way any exceptions hit
-     * partway while opening the reader, which is expected
-     * eg if writer is committing, won't close our
-     * directory.  We restore this value below:
-     */
-    final boolean myCloseDirectory = closeDirectory; // @deprectated
-    closeDirectory = false;
-
-    try {
-      reader = (DirectoryReader) finder.run(commit);
-    } finally {
-      if (myCloseDirectory) {
-        assert directory instanceof FSDirectory;
-        // Restore my closeDirectory
-        closeDirectory = true;
-        if (reader != null && reader != this) {
-          // Success, and a new reader was actually opened
-          reader.closeDirectory = true;
-          // Clone the directory
-          reader.directory = FSDirectory.getDirectory(((FSDirectory) directory).getFile());
-        }
-      }
-    }
-
-    return reader;
   }
 
   private synchronized DirectoryReader doReopen(SegmentInfos infos, boolean doClone, boolean openReadOnly) throws CorruptIndexException, IOException {
     DirectoryReader reader;
 	  if (openReadOnly) {
-      reader = new ReadOnlyDirectoryReader(directory, infos, closeDirectory, subReaders, starts, normsCache, doClone);
+      reader = new ReadOnlyDirectoryReader(directory, infos, subReaders, starts, normsCache, doClone);
     } else {
-      reader = new DirectoryReader(directory, infos, closeDirectory, subReaders, starts, normsCache, false, doClone);
+      reader = new DirectoryReader(directory, infos, subReaders, starts, normsCache, false, doClone);
     }
     reader.setDisableFakeNorms(getDisableFakeNorms());
     return reader;
@@ -868,11 +805,17 @@ class DirectoryReader extends IndexReader implements Cloneable {
   }
 
   protected synchronized void doClose() throws IOException {
-    for (int i = 0; i < subReaders.length; i++)
+    IOException ioe = null;
-      subReaders[i].decRef();
+    for (int i = 0; i < subReaders.length; i++) {
-
+      // try to close each reader, even if an exception is thrown
-    if (closeDirectory)
+      try {
-      directory.close();
+        subReaders[i].decRef();
+      } catch (IOException e) {
+        if (ioe == null) ioe = e;
+      }
+    }
+    // throw the first exception
+    if (ioe != null) throw ioe;
   }
 
   public Collection getFieldNames (IndexReader.FieldOption fieldNames) {

src/java/org/apache/lucene/index/IndexReader.java

@@ -199,7 +199,7 @@ public abstract class IndexReader implements Cloneable {
       throw new AlreadyClosedException("this IndexReader is closed");
     }
   }
-
+  
   /** Returns a read/write IndexReader reading the index in an FSDirectory in the named
    *  path.
    * @throws CorruptIndexException if the index is corrupt
@@ -208,7 +208,7 @@ public abstract class IndexReader implements Cloneable {
    * Use {@link #open(Directory, boolean)} instead
    * @param path the path to the index directory */
   public static IndexReader open(String path) throws CorruptIndexException, IOException {
-    return open(FSDirectory.getDirectory(path), true, null, null, false);
+    return new DirectoryOwningReader(open(FSDirectory.getDirectory(path), null, null, false));
   }
 
   /** Returns an IndexReader reading the index in an
@@ -225,7 +225,7 @@ public abstract class IndexReader implements Cloneable {
    * Use {@link #open(Directory, boolean)} instead
    */
   public static IndexReader open(String path, boolean readOnly) throws CorruptIndexException, IOException {
-    return open(FSDirectory.getDirectory(path), true, null, null, readOnly);
+    return new DirectoryOwningReader(open(FSDirectory.getDirectory(path), null, null, readOnly));
   }
 
   /** Returns a read/write IndexReader reading the index in an FSDirectory in the named
@@ -237,7 +237,7 @@ public abstract class IndexReader implements Cloneable {
    * Use {@link #open(Directory, boolean)} instead
    */
   public static IndexReader open(File path) throws CorruptIndexException, IOException {
-    return open(FSDirectory.getDirectory(path), true, null, null, false);
+    return new DirectoryOwningReader(open(FSDirectory.getDirectory(path), null, null, false));
   }
 
   /** Returns an IndexReader reading the index in an
@@ -254,7 +254,7 @@ public abstract class IndexReader implements Cloneable {
    * Use {@link #open(Directory, boolean)} instead
    */
   public static IndexReader open(File path, boolean readOnly) throws CorruptIndexException, IOException {
-    return open(FSDirectory.getDirectory(path), true, null, null, readOnly);
+    return new DirectoryOwningReader(open(FSDirectory.getDirectory(path), null, null, readOnly));
   }
 
   /** Returns a read/write IndexReader reading the index in
@@ -266,7 +266,7 @@ public abstract class IndexReader implements Cloneable {
    * Use {@link #open(Directory, boolean)} instead
    */
   public static IndexReader open(final Directory directory) throws CorruptIndexException, IOException {
-    return open(directory, false, null, null, false);
+    return open(directory, null, null, false);
   }
 
   /** Returns an IndexReader reading the index in the given
@@ -280,7 +280,7 @@ public abstract class IndexReader implements Cloneable {
    * @throws IOException if there is a low-level IO error
    */
   public static IndexReader open(final Directory directory, boolean readOnly) throws CorruptIndexException, IOException {
-    return open(directory, false, null, null, readOnly);
+    return open(directory, null, null, readOnly);
   }
 
   /** Expert: returns a read/write IndexReader reading the index in the given
@@ -292,7 +292,7 @@ public abstract class IndexReader implements Cloneable {
    * @throws IOException if there is a low-level IO error
    */
   public static IndexReader open(final IndexCommit commit) throws CorruptIndexException, IOException {
-    return open(commit.getDirectory(), false, null, commit, false);
+    return open(commit.getDirectory(), null, commit, false);
   }
 
   /** Expert: returns an IndexReader reading the index in the given
@@ -306,7 +306,7 @@ public abstract class IndexReader implements Cloneable {
    * @throws IOException if there is a low-level IO error
    */
   public static IndexReader open(final IndexCommit commit, boolean readOnly) throws CorruptIndexException, IOException {
-    return open(commit.getDirectory(), false, null, commit, readOnly);
+    return open(commit.getDirectory(), null, commit, readOnly);
   }
 
   /** Expert: returns a read/write IndexReader reading the index in the given
@@ -321,7 +321,7 @@ public abstract class IndexReader implements Cloneable {
    * @throws IOException if there is a low-level IO error
    */
   public static IndexReader open(final Directory directory, IndexDeletionPolicy deletionPolicy) throws CorruptIndexException, IOException {
-    return open(directory, false, deletionPolicy, null, false);
+    return open(directory, deletionPolicy, null, false);
   }
 
   /** Expert: returns an IndexReader reading the index in
@@ -339,7 +339,7 @@ public abstract class IndexReader implements Cloneable {
    * @throws IOException if there is a low-level IO error
    */
   public static IndexReader open(final Directory directory, IndexDeletionPolicy deletionPolicy, boolean readOnly) throws CorruptIndexException, IOException {
-    return open(directory, false, deletionPolicy, null, readOnly);
+    return open(directory, deletionPolicy, null, readOnly);
   }
 
   /** Expert: returns a read/write IndexReader reading the index in the given
@@ -357,7 +357,7 @@ public abstract class IndexReader implements Cloneable {
    * @throws IOException if there is a low-level IO error
    */
   public static IndexReader open(final IndexCommit commit, IndexDeletionPolicy deletionPolicy) throws CorruptIndexException, IOException {
-    return open(commit.getDirectory(), false, deletionPolicy, commit, false);
+    return open(commit.getDirectory(), deletionPolicy, commit, false);
   }
 
   /** Expert: returns an IndexReader reading the index in
@@ -377,11 +377,11 @@ public abstract class IndexReader implements Cloneable {
    * @throws IOException if there is a low-level IO error
    */
   public static IndexReader open(final IndexCommit commit, IndexDeletionPolicy deletionPolicy, boolean readOnly) throws CorruptIndexException, IOException {
-    return open(commit.getDirectory(), false, deletionPolicy, commit, readOnly);
+    return open(commit.getDirectory(), deletionPolicy, commit, readOnly);
   }
 
-  private static IndexReader open(final Directory directory, final boolean closeDirectory, final IndexDeletionPolicy deletionPolicy, final IndexCommit commit, final boolean readOnly) throws CorruptIndexException, IOException {
+  private static IndexReader open(final Directory directory, final IndexDeletionPolicy deletionPolicy, final IndexCommit commit, final boolean readOnly) throws CorruptIndexException, IOException {
-    return DirectoryReader.open(directory, closeDirectory, deletionPolicy, commit, readOnly);
+    return DirectoryReader.open(directory, deletionPolicy, commit, readOnly);
   }
 
   /**
@@ -577,9 +577,11 @@ public abstract class IndexReader implements Cloneable {
    */
   public static long getCurrentVersion(File directory) throws CorruptIndexException, IOException {
     Directory dir = FSDirectory.getDirectory(directory);
-    long version = getCurrentVersion(dir);
+    try {
-    dir.close();
+      return getCurrentVersion(dir);
-    return version;
+    } finally {
+      dir.close();
+    }
   }
 
   /**
@@ -1196,9 +1198,11 @@ public abstract class IndexReader implements Cloneable {
    */
   public static boolean isLocked(String directory) throws IOException {
     Directory dir = FSDirectory.getDirectory(directory);
-    boolean result = isLocked(dir);
+    try {
-    dir.close();
+      return isLocked(dir);
-    return result;
+    } finally {
+      dir.close();
+    }
   }
 
   /**

src/java/org/apache/lucene/index/ReadOnlyDirectoryReader.java

@@ -23,12 +23,12 @@ import java.io.IOException;
 import java.util.Map;
 
 class ReadOnlyDirectoryReader extends DirectoryReader {
-  ReadOnlyDirectoryReader(Directory directory, SegmentInfos sis, IndexDeletionPolicy deletionPolicy, boolean closeDirectory) throws IOException {
+  ReadOnlyDirectoryReader(Directory directory, SegmentInfos sis, IndexDeletionPolicy deletionPolicy) throws IOException {
-    super(directory, sis, deletionPolicy, closeDirectory, true);
+    super(directory, sis, deletionPolicy, true);
   }
 
-  ReadOnlyDirectoryReader(Directory directory, SegmentInfos infos, boolean closeDirectory, SegmentReader[] oldReaders, int[] oldStarts, Map oldNormsCache, boolean doClone) throws IOException {
+  ReadOnlyDirectoryReader(Directory directory, SegmentInfos infos, SegmentReader[] oldReaders, int[] oldStarts, Map oldNormsCache, boolean doClone) throws IOException {
-    super(directory, infos, closeDirectory, oldReaders, oldStarts, oldNormsCache, true, doClone);
+    super(directory, infos, oldReaders, oldStarts, oldNormsCache, true, doClone);
   }
   
   ReadOnlyDirectoryReader(IndexWriter writer, SegmentInfos infos) throws IOException {

src/test/org/apache/lucene/index/TestIndexReaderClone.java

@@ -17,6 +17,9 @@ package org.apache.lucene.index;
  * limitations under the License.
  */
 
+import java.io.File;
+import java.io.IOException;
+
 import org.apache.lucene.index.SegmentReader.Norm;
 import org.apache.lucene.search.Similarity;
 import org.apache.lucene.analysis.SimpleAnalyzer;
@@ -25,7 +28,9 @@ import org.apache.lucene.document.Field;
 import org.apache.lucene.store.Directory;
 import org.apache.lucene.store.LockObtainFailedException;
 import org.apache.lucene.store.MockRAMDirectory;
+import org.apache.lucene.store.FSDirectory;
 import org.apache.lucene.util.LuceneTestCase;
+import org.apache.lucene.store.AlreadyClosedException;
 
 /**
  * Tests cloning multiple types of readers, modifying the deletedDocs and norms
@@ -51,6 +56,34 @@ public class TestIndexReaderClone extends LuceneTestCase {
     dir1.close();
   }
 
+  // LUCENE-1453
+  public void testFSDirectoryClone() throws Exception {
+
+    String tempDir = System.getProperty("java.io.tmpdir");
+    if (tempDir == null)
+      throw new IOException("java.io.tmpdir undefined, cannot run test");
+    File indexDir2 = new File(tempDir, "FSDirIndexReaderClone");
+
+    Directory dir1 = FSDirectory.getDirectory(indexDir2);
+    TestIndexReaderReopen.createIndex(dir1, false);
+
+    IndexReader reader = IndexReader.open(indexDir2);
+    IndexReader readOnlyReader = (IndexReader) reader.clone();
+    reader.close();
+    readOnlyReader.close();
+
+    // Make sure we didn't pick up too many incRef's along
+    // the way -- this close should be the final close:
+    dir1.close();
+
+    try {
+      dir1.listAll();
+      fail("did not hit AlreadyClosedException");
+    } catch (AlreadyClosedException ace) {
+      // expected
+    }
+  }
+
   // open non-readOnly reader1, clone to non-readOnly
   // reader2, make sure we can change reader2
   public void testCloneNoChangesStillReadOnly() throws Exception {