LUCENE-3800: "Soft-close" IndexReader (make it unuseable) once a child-reader (subreader, filter delegate, or parallel reader) is closed.

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

lucene/core/src/java/org/apache/lucene/index/BaseMultiReader.java

@@ -36,11 +36,13 @@ abstract class BaseMultiReader<R extends IndexReader> 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;

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

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<ReaderClosedListener> readerClosedListeners = 
       Collections.synchronizedSet(new LinkedHashSet<ReaderClosedListener>());
 
+  private final Set<IndexReader> parentReaders = 
+      Collections.synchronizedSet(Collections.newSetFromMap(new WeakHashMap<IndexReader,Boolean>()));
+
   /** Expert: adds a {@link ReaderClosedListener}.  The
    * provided listener will be invoked when this reader is closed.
    *
@@ -108,7 +115,18 @@ public abstract class IndexReader implements Closeable {
     readerClosedListeners.remove(listener);
   }
   
-  private final void notifyReaderClosedListeners() {
+  /** 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 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}
+   * <p>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}
+   * <p>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

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) {
+      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;
   }
   

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;
   }
 

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();

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 {