diff --git a/lucene/core/src/java/org/apache/lucene/index/BaseMultiReader.java b/lucene/core/src/java/org/apache/lucene/index/BaseMultiReader.java index 3031ceb60be..2cbb4824ffc 100644 --- a/lucene/core/src/java/org/apache/lucene/index/BaseMultiReader.java +++ b/lucene/core/src/java/org/apache/lucene/index/BaseMultiReader.java @@ -36,11 +36,13 @@ abstract class BaseMultiReader extends CompositeReader { boolean hasDeletions = false; for (int i = 0; i < subReaders.length; i++) { starts[i] = maxDoc; - maxDoc += subReaders[i].maxDoc(); // compute maxDocs - numDocs += subReaders[i].numDocs(); // compute numDocs - if (subReaders[i].hasDeletions()) { + final IndexReader r = subReaders[i]; + maxDoc += r.maxDoc(); // compute maxDocs + numDocs += r.numDocs(); // compute numDocs + if (r.hasDeletions()) { hasDeletions = true; } + r.registerParentReader(this); } starts[subReaders.length] = maxDoc; this.maxDoc = maxDoc; diff --git a/lucene/core/src/java/org/apache/lucene/index/FilterAtomicReader.java b/lucene/core/src/java/org/apache/lucene/index/FilterAtomicReader.java index 80b1e8cc160..6a1da23a358 100644 --- a/lucene/core/src/java/org/apache/lucene/index/FilterAtomicReader.java +++ b/lucene/core/src/java/org/apache/lucene/index/FilterAtomicReader.java @@ -282,6 +282,7 @@ public class FilterAtomicReader extends AtomicReader { public FilterAtomicReader(AtomicReader in) { super(); this.in = in; + in.registerParentReader(this); } @Override diff --git a/lucene/core/src/java/org/apache/lucene/index/IndexReader.java b/lucene/core/src/java/org/apache/lucene/index/IndexReader.java index 8f6a3139f22..5bf88caf150 100644 --- a/lucene/core/src/java/org/apache/lucene/index/IndexReader.java +++ b/lucene/core/src/java/org/apache/lucene/index/IndexReader.java @@ -21,6 +21,7 @@ import java.io.Closeable; import java.io.IOException; import java.util.Collections; import java.util.LinkedHashSet; +import java.util.WeakHashMap; import java.util.Set; import java.util.concurrent.atomic.AtomicInteger; @@ -72,10 +73,13 @@ import org.apache.lucene.util.ReaderUtil; // for javadocs */ public abstract class IndexReader implements Closeable { + private boolean closed = false; + private boolean closedByChild = false; + private final AtomicInteger refCount = new AtomicInteger(1); + IndexReader() { if (!(this instanceof CompositeReader || this instanceof AtomicReader)) - throw new Error("This class should never be directly extended, subclass AtomicReader or CompositeReader instead!"); - refCount.set(1); + throw new Error("IndexReader should never be directly extended, subclass AtomicReader or CompositeReader instead."); } /** @@ -91,6 +95,9 @@ public abstract class IndexReader implements Closeable { private final Set readerClosedListeners = Collections.synchronizedSet(new LinkedHashSet()); + private final Set parentReaders = + Collections.synchronizedSet(Collections.newSetFromMap(new WeakHashMap())); + /** Expert: adds a {@link ReaderClosedListener}. The * provided listener will be invoked when this reader is closed. * @@ -107,8 +114,19 @@ public abstract class IndexReader implements Closeable { ensureOpen(); readerClosedListeners.remove(listener); } + + /** Expert: This method is called by {@code IndexReader}s which wrap other readers + * (e.g. {@link CompositeReader} or {@link FilterAtomicReader}) to register the parent + * at the child (this reader) on construction of the parent. When this reader is closed, + * it will mark all registered parents as closed, too. The references to parent readers + * are weak only, so they can be GCed once they are no longer in use. + * @lucene.experimental */ + public final void registerParentReader(IndexReader reader) { + ensureOpen(); + parentReaders.add(reader); + } - private final void notifyReaderClosedListeners() { + private void notifyReaderClosedListeners() { synchronized(readerClosedListeners) { for(ReaderClosedListener listener : readerClosedListeners) { listener.onClose(this); @@ -116,9 +134,17 @@ public abstract class IndexReader implements Closeable { } } - private boolean closed = false; - - private final AtomicInteger refCount = new AtomicInteger(); + private void reportCloseToParentReaders() { + synchronized(parentReaders) { + for(IndexReader parent : parentReaders) { + parent.closedByChild = true; + // cross memory barrier by a fake write: + parent.refCount.addAndGet(0); + // recurse: + parent.reportCloseToParentReaders(); + } + } + } /** Expert: returns the current refCount for this reader */ public final int getRefCount() { @@ -191,7 +217,12 @@ public abstract class IndexReader implements Closeable { * @see #incRef */ public final void decRef() throws IOException { - ensureOpen(); + // only check refcount here (don't call ensureOpen()), so we can + // still close the reader if it was made invalid by a child: + if (refCount.get() <= 0) { + throw new AlreadyClosedException("this IndexReader is closed"); + } + final int rc = refCount.decrementAndGet(); if (rc == 0) { boolean success = false; @@ -204,6 +235,7 @@ public abstract class IndexReader implements Closeable { refCount.incrementAndGet(); } } + reportCloseToParentReaders(); notifyReaderClosedListeners(); } else if (rc < 0) { throw new IllegalStateException("too many decRef calls: refCount is " + rc + " after decrement"); @@ -217,6 +249,33 @@ public abstract class IndexReader implements Closeable { if (refCount.get() <= 0) { throw new AlreadyClosedException("this IndexReader is closed"); } + // the happens before rule on reading the refCount, which must be after the fake write, + // ensures that we see the value: + if (closedByChild) { + throw new AlreadyClosedException("this IndexReader cannot be used anymore as one of its child readers was closed"); + } + } + + /** {@inheritDoc} + *

For caching purposes, {@code IndexReader} subclasses are not allowed + * to implement equals/hashCode, so methods are declared final. + * To lookup instances from caches use {@link #getCoreCacheKey} and + * {@link #getCombinedCoreAndDeletesKey}. + */ + @Override + public final boolean equals(Object obj) { + return (this == obj); + } + + /** {@inheritDoc} + *

For caching purposes, {@code IndexReader} subclasses are not allowed + * to implement equals/hashCode, so methods are declared final. + * To lookup instances from caches use {@link #getCoreCacheKey} and + * {@link #getCombinedCoreAndDeletesKey}. + */ + @Override + public final int hashCode() { + return System.identityHashCode(this); } /** Returns a IndexReader reading the index in the given diff --git a/lucene/core/src/java/org/apache/lucene/index/ParallelAtomicReader.java b/lucene/core/src/java/org/apache/lucene/index/ParallelAtomicReader.java index 4a8980c6bb1..460c7fa60e2 100644 --- a/lucene/core/src/java/org/apache/lucene/index/ParallelAtomicReader.java +++ b/lucene/core/src/java/org/apache/lucene/index/ParallelAtomicReader.java @@ -131,10 +131,11 @@ public final class ParallelAtomicReader extends AtomicReader { } // do this finally so any Exceptions occurred before don't affect refcounts: - if (!closeSubReaders) { - for (AtomicReader reader : completeReaderSet) { + for (AtomicReader reader : completeReaderSet) { + if (!closeSubReaders) { reader.incRef(); } + reader.registerParentReader(this); } } @@ -216,11 +217,6 @@ public final class ParallelAtomicReader extends AtomicReader { @Override public Fields fields() { ensureOpen(); - // we cache the inner field instances, so we must check - // that the delegate readers are really still open: - for (final AtomicReader reader : parallelReaders) { - reader.ensureOpen(); - } return fields; } diff --git a/lucene/core/src/java/org/apache/lucene/index/SlowCompositeReaderWrapper.java b/lucene/core/src/java/org/apache/lucene/index/SlowCompositeReaderWrapper.java index 11486ccc118..bb6dacce947 100644 --- a/lucene/core/src/java/org/apache/lucene/index/SlowCompositeReaderWrapper.java +++ b/lucene/core/src/java/org/apache/lucene/index/SlowCompositeReaderWrapper.java @@ -68,6 +68,7 @@ public final class SlowCompositeReaderWrapper extends AtomicReader { in = reader; fields = MultiFields.getFields(in); liveDocs = MultiFields.getLiveDocs(in); + in.registerParentReader(this); } @Override @@ -78,7 +79,6 @@ public final class SlowCompositeReaderWrapper extends AtomicReader { @Override public Fields fields() throws IOException { ensureOpen(); - in.ensureOpen(); // as we cached the fields, we better check the original reader return fields; } @@ -127,7 +127,6 @@ public final class SlowCompositeReaderWrapper extends AtomicReader { @Override public Bits getLiveDocs() { ensureOpen(); - in.ensureOpen(); // as we cached the liveDocs, we better check the original reader return liveDocs; } diff --git a/lucene/core/src/test/org/apache/lucene/index/TestReaderClosed.java b/lucene/core/src/test/org/apache/lucene/index/TestReaderClosed.java index 7c06a466feb..bcae25784e6 100644 --- a/lucene/core/src/test/org/apache/lucene/index/TestReaderClosed.java +++ b/lucene/core/src/test/org/apache/lucene/index/TestReaderClosed.java @@ -30,7 +30,6 @@ import org.apache.lucene.util.LuceneTestCase; import org.apache.lucene.util._TestUtil; public class TestReaderClosed extends LuceneTestCase { - private IndexSearcher searcher; private IndexReader reader; private Directory dir; @@ -54,12 +53,12 @@ public class TestReaderClosed extends LuceneTestCase { writer.addDocument(doc); } reader = writer.getReader(); - searcher = newSearcher(reader, /* TODO: change that back to true and add better test, - so wrapped readers are explicitely checked, see LUCENE-3800: */ false); writer.close(); } public void test() throws Exception { + assertTrue(reader.getRefCount() > 0); + IndexSearcher searcher = newSearcher(reader); TermRangeQuery query = TermRangeQuery.newStringRange("field", "a", "z", true, true); searcher.search(query, 5); reader.close(); @@ -70,6 +69,25 @@ public class TestReaderClosed extends LuceneTestCase { } } + // LUCENE-3800 + public void testReaderChaining() throws Exception { + assertTrue(reader.getRefCount() > 0); + IndexReader wrappedReader = SlowCompositeReaderWrapper.wrap(reader); + wrappedReader = new ParallelAtomicReader((AtomicReader) wrappedReader); + IndexSearcher searcher = newSearcher(wrappedReader); + TermRangeQuery query = TermRangeQuery.newStringRange("field", "a", "z", true, true); + searcher.search(query, 5); + reader.close(); // close original child reader + try { + searcher.search(query, 5); + } catch (AlreadyClosedException ace) { + assertEquals( + "this IndexReader cannot be used anymore as one of its child readers was closed", + ace.getMessage() + ); + } + } + public void tearDown() throws Exception { dir.close(); super.tearDown(); diff --git a/solr/core/src/test/org/apache/solr/search/TestDocSet.java b/solr/core/src/test/org/apache/solr/search/TestDocSet.java index 6449c09f932..1e8f295aa55 100644 --- a/solr/core/src/test/org/apache/solr/search/TestDocSet.java +++ b/solr/core/src/test/org/apache/solr/search/TestDocSet.java @@ -22,7 +22,10 @@ import java.util.Arrays; import java.util.Random; import org.apache.lucene.index.FieldInfos; -import org.apache.lucene.index.FilterAtomicReader; +import org.apache.lucene.index.DocValues; +import org.apache.lucene.index.StoredFieldVisitor; +import org.apache.lucene.index.Fields; +import org.apache.lucene.index.AtomicReader; import org.apache.lucene.index.IndexReader; import org.apache.lucene.index.AtomicReaderContext; import org.apache.lucene.index.MultiReader; @@ -31,6 +34,7 @@ import org.apache.lucene.search.DocIdSet; import org.apache.lucene.search.DocIdSetIterator; import org.apache.lucene.search.Filter; import org.apache.lucene.util.LuceneTestCase; +import org.apache.lucene.util.Bits; import org.apache.lucene.util.OpenBitSet; import org.apache.lucene.util.OpenBitSetIterator; @@ -336,9 +340,8 @@ public class TestDocSet extends LuceneTestCase { } ***/ - public IndexReader dummyIndexReader(final int maxDoc) { - // TODO FIXME: THIS IS HEAVY BROKEN AND ILLEGAL TO DO (null delegate): - IndexReader r = new FilterAtomicReader(null) { + public AtomicReader dummyIndexReader(final int maxDoc) { + return new AtomicReader() { @Override public int maxDoc() { return maxDoc; @@ -358,8 +361,40 @@ public class TestDocSet extends LuceneTestCase { public FieldInfos getFieldInfos() { return new FieldInfos(); } + + @Override + public Bits getLiveDocs() { + return null; + } + + @Override + public Fields fields() { + return null; + } + + @Override + public Fields getTermVectors(int doc) { + return null; + } + + @Override + public DocValues normValues(String field) { + return null; + } + + @Override + public DocValues docValues(String field) { + return null; + } + + @Override + protected void doClose() { + } + + @Override + public void document(int doc, StoredFieldVisitor visitor) { + } }; - return r; } public IndexReader dummyMultiReader(int nSeg, int maxDoc) throws IOException {