Index: CHANGES.txt =================================================================== --- CHANGES.txt (revision 783094) +++ CHANGES.txt (working copy) @@ -481,10 +481,12 @@ 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 - incorrectly closing the shared FSDirectory. This bug would only - happen if you use IndexReader.open with a File or String argument. - (Mark Miller via Mike McCandless) +6. LUCENE-1453: Ensure IndexReader.reopen()/clone() does not result in + incorrectly closing the shared FSDirectory. This bug would only + happen if you use IndexReader.open() with a File or String argument. + 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) Index: src/java/org/apache/lucene/index/DirectoryOwningReader.java =================================================================== --- src/java/org/apache/lucene/index/DirectoryOwningReader.java (revision 0) +++ src/java/org/apache/lucene/index/DirectoryOwningReader.java (revision 0) @@ -0,0 +1,116 @@ +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 { + + DirectoryOwningReader(final IndexReader in) { + this(in, new RefCounter()); + } + + private DirectoryOwningReader(final IndexReader in, final RefCounter refCount) { + super(in); + this.refCount = refCount; + refCount.incRef(); + } + + public IndexReader reopen() throws CorruptIndexException, IOException { + ensureOpen(); + final IndexReader r = in.reopen(); + if (r != in) + return new DirectoryOwningReader(r, refCount); + return this; + } + + public IndexReader reopen(boolean openReadOnly) throws CorruptIndexException, IOException { + ensureOpen(); + final IndexReader r = in.reopen(openReadOnly); + if (r != in) + return new DirectoryOwningReader(r, refCount); + 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, refCount); + return this; + } + + public Object clone() { + ensureOpen(); + return new DirectoryOwningReader((IndexReader) in.clone(), refCount); + } + + public IndexReader clone(boolean openReadOnly) throws CorruptIndexException, IOException { + ensureOpen(); + return new DirectoryOwningReader(in.clone(openReadOnly), refCount); + } + + 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 (refCount.decRef() == 0) { + try { + in.directory().close(); + } catch (IOException e) { + if (ioe == null) ioe = e; + } + } + // throw the first exception + if (ioe != null) throw ioe; + } + + private final RefCounter refCount; + + /** + * This class contains the ref counter, that is passed to each instance after cloning/reopening, + * and is global to all DirectoryOwningIndexReader derived from the original one. + * In Java 1.5, I would use a java.util.concurrent.atomic.AtomicInteger, which is equal to this one. + */ + private static final class RefCounter { + private int c = 0; + + public synchronized int incRef() { + return ++c; + } + + public synchronized int decRef() { + return --c; + } + + } + +} + Property changes on: src\java\org\apache\lucene\index\DirectoryOwningReader.java ___________________________________________________________________ Added: svn:eol-style + native Index: src/java/org/apache/lucene/index/DirectoryReader.java =================================================================== --- src/java/org/apache/lucene/index/DirectoryReader.java (revision 783094) +++ src/java/org/apache/lucene/index/DirectoryReader.java (working copy) @@ -36,7 +36,6 @@ 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 @@ class DirectoryReader extends IndexReader implements Cloneable { protected Directory directory; protected boolean readOnly; - protected boolean closeDirectory; IndexWriter writer; @@ -64,48 +62,23 @@ 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 { - SegmentInfos.FindSegmentsFile finder = new SegmentInfos.FindSegmentsFile(directory) { - + static IndexReader open(final Directory directory, final IndexDeletionPolicy deletionPolicy, final IndexCommit commit, final boolean readOnly) throws CorruptIndexException, IOException { + 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); } - }; - - 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; + }.run(commit); } /** 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 @@ 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 @@ } /** 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 @@ DirectoryReader newReader = doReopen((SegmentInfos) segmentInfos.clone(), true, openReadOnly); if (this != newReader) { - newReader.closeDirectory = closeDirectory; newReader.deletionPolicy = deletionPolicy; } newReader.writer = writer; @@ -445,54 +415,21 @@ } } - 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); } - }; - - 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; + }.run(commit); } 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 @@ } protected synchronized void doClose() throws IOException { - for (int i = 0; i < subReaders.length; i++) - subReaders[i].decRef(); - - if (closeDirectory) - directory.close(); + IOException ioe = null; + for (int i = 0; i < subReaders.length; i++) { + // try to close each reader, even if an exception is thrown + try { + 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) { Index: src/java/org/apache/lucene/index/IndexReader.java =================================================================== --- src/java/org/apache/lucene/index/IndexReader.java (revision 783094) +++ src/java/org/apache/lucene/index/IndexReader.java (working copy) @@ -199,7 +199,7 @@ 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 @@ * 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 @@ * 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 @@ * 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 @@ * 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 @@ * 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 @@ * @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 @@ * @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 @@ * @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 @@ * @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 @@ * @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 @@ * @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 @@ * @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 { - return DirectoryReader.open(directory, closeDirectory, deletionPolicy, commit, readOnly); + private static IndexReader open(final Directory directory, final IndexDeletionPolicy deletionPolicy, final IndexCommit commit, final boolean readOnly) throws CorruptIndexException, IOException { + return DirectoryReader.open(directory, deletionPolicy, commit, readOnly); } /** @@ -577,9 +577,11 @@ */ public static long getCurrentVersion(File directory) throws CorruptIndexException, IOException { Directory dir = FSDirectory.getDirectory(directory); - long version = getCurrentVersion(dir); - dir.close(); - return version; + try { + return getCurrentVersion(dir); + } finally { + dir.close(); + } } /** @@ -1196,9 +1198,11 @@ */ public static boolean isLocked(String directory) throws IOException { Directory dir = FSDirectory.getDirectory(directory); - boolean result = isLocked(dir); - dir.close(); - return result; + try { + return isLocked(dir); + } finally { + dir.close(); + } } /** Index: src/java/org/apache/lucene/index/ReadOnlyDirectoryReader.java =================================================================== --- src/java/org/apache/lucene/index/ReadOnlyDirectoryReader.java (revision 783094) +++ src/java/org/apache/lucene/index/ReadOnlyDirectoryReader.java (working copy) @@ -23,12 +23,12 @@ import java.util.Map; class ReadOnlyDirectoryReader extends DirectoryReader { - ReadOnlyDirectoryReader(Directory directory, SegmentInfos sis, IndexDeletionPolicy deletionPolicy, boolean closeDirectory) throws IOException { - super(directory, sis, deletionPolicy, closeDirectory, true); + ReadOnlyDirectoryReader(Directory directory, SegmentInfos sis, IndexDeletionPolicy deletionPolicy) throws IOException { + super(directory, sis, deletionPolicy, true); } - ReadOnlyDirectoryReader(Directory directory, SegmentInfos infos, boolean closeDirectory, SegmentReader[] oldReaders, int[] oldStarts, Map oldNormsCache, boolean doClone) throws IOException { - super(directory, infos, closeDirectory, oldReaders, oldStarts, oldNormsCache, true, doClone); + ReadOnlyDirectoryReader(Directory directory, SegmentInfos infos, SegmentReader[] oldReaders, int[] oldStarts, Map oldNormsCache, boolean doClone) throws IOException { + super(directory, infos, oldReaders, oldStarts, oldNormsCache, true, doClone); } ReadOnlyDirectoryReader(IndexWriter writer, SegmentInfos infos) throws IOException {