You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@lucene.apache.org by si...@apache.org on 2014/03/25 17:39:10 UTC

svn commit: r1581408 - in /lucene/dev/branches/lucene_solr_4_7: ./ lucene/ lucene/core/ lucene/core/src/java/org/apache/lucene/index/ lucene/core/src/java/org/apache/lucene/util/ lucene/core/src/test/org/apache/lucene/index/

Author: simonw
Date: Tue Mar 25 16:39:09 2014
New Revision: 1581408

URL: http://svn.apache.org/r1581408
Log:
LUCENE-5553: IndexReader#ReaderClosedListener is not always called on IndexReader#close()

Added:
    lucene/dev/branches/lucene_solr_4_7/lucene/core/src/test/org/apache/lucene/index/TestIndexReaderClose.java
      - copied, changed from r1581404, lucene/dev/branches/branch_4x/lucene/core/src/test/org/apache/lucene/index/TestIndexReaderClose.java
Modified:
    lucene/dev/branches/lucene_solr_4_7/   (props changed)
    lucene/dev/branches/lucene_solr_4_7/lucene/   (props changed)
    lucene/dev/branches/lucene_solr_4_7/lucene/CHANGES.txt   (contents, props changed)
    lucene/dev/branches/lucene_solr_4_7/lucene/core/   (props changed)
    lucene/dev/branches/lucene_solr_4_7/lucene/core/src/java/org/apache/lucene/index/IndexReader.java
    lucene/dev/branches/lucene_solr_4_7/lucene/core/src/java/org/apache/lucene/index/SegmentCoreReaders.java
    lucene/dev/branches/lucene_solr_4_7/lucene/core/src/java/org/apache/lucene/index/SegmentReader.java
    lucene/dev/branches/lucene_solr_4_7/lucene/core/src/java/org/apache/lucene/util/IOUtils.java

Modified: lucene/dev/branches/lucene_solr_4_7/lucene/CHANGES.txt
URL: http://svn.apache.org/viewvc/lucene/dev/branches/lucene_solr_4_7/lucene/CHANGES.txt?rev=1581408&r1=1581407&r2=1581408&view=diff
==============================================================================
--- lucene/dev/branches/lucene_solr_4_7/lucene/CHANGES.txt (original)
+++ lucene/dev/branches/lucene_solr_4_7/lucene/CHANGES.txt Tue Mar 25 16:39:09 2014
@@ -56,6 +56,12 @@ Bug Fixes
   indexed shapes within 1/2 maxDistErr from the edge of the query shape.  This meant
   searching for a point by the same point as a query rarely worked.  (David Smiley)
 
+* LUCENE-5553: IndexReader#ReaderClosedListener is not always invoked when 
+  IndexReader#close() is called or if refCount is 0. If an exception is 
+  thrown during interal close or on any of the close listerns some or all
+  listerners might be missed. This can cause memory leaks if the core listeners
+  are used to clear caches. (Simon Willnauer)
+
 Build
 
 * LUCENE-5511: "ant precommit" / "ant check-svn-working-copy" now work again

Modified: lucene/dev/branches/lucene_solr_4_7/lucene/core/src/java/org/apache/lucene/index/IndexReader.java
URL: http://svn.apache.org/viewvc/lucene/dev/branches/lucene_solr_4_7/lucene/core/src/java/org/apache/lucene/index/IndexReader.java?rev=1581408&r1=1581407&r2=1581408&view=diff
==============================================================================
--- lucene/dev/branches/lucene_solr_4_7/lucene/core/src/java/org/apache/lucene/index/IndexReader.java (original)
+++ lucene/dev/branches/lucene_solr_4_7/lucene/core/src/java/org/apache/lucene/index/IndexReader.java Tue Mar 25 16:39:09 2014
@@ -17,6 +17,13 @@ package org.apache.lucene.index;
  * limitations under the License.
  */
 
+import org.apache.lucene.document.Document;
+import org.apache.lucene.document.DocumentStoredFieldVisitor;
+import org.apache.lucene.store.AlreadyClosedException;
+import org.apache.lucene.store.Directory;
+import org.apache.lucene.util.Bits;
+import org.apache.lucene.util.IOUtils;
+
 import java.io.Closeable;
 import java.io.IOException;
 import java.util.Collections;
@@ -25,12 +32,6 @@ import java.util.List;
 import java.util.Set;
 import java.util.WeakHashMap;
 import java.util.concurrent.atomic.AtomicInteger;
-
-import org.apache.lucene.document.Document;
-import org.apache.lucene.document.DocumentStoredFieldVisitor;
-import org.apache.lucene.store.AlreadyClosedException;
-import org.apache.lucene.store.Directory;
-import org.apache.lucene.util.Bits;
 // javadocs
 
 /** IndexReader is an abstract class, providing an interface for accessing an
@@ -127,11 +128,18 @@ public abstract class IndexReader implem
     parentReaders.add(reader);
   }
 
-  private void notifyReaderClosedListeners() {
+  private void notifyReaderClosedListeners(Throwable th) {
     synchronized(readerClosedListeners) {
       for(ReaderClosedListener listener : readerClosedListeners) {
-        listener.onClose(this);
+        try {
+          listener.onClose(this);
+        } catch (Throwable t) {
+          if (th == null) {
+            th = t;
+          }
+        }
       }
+      IOUtils.reThrowUnchecked(th);
     }
   }
 
@@ -227,18 +235,19 @@ public abstract class IndexReader implem
     
     final int rc = refCount.decrementAndGet();
     if (rc == 0) {
-      boolean success = false;
+      closed = true;
+      Throwable throwable = null;
       try {
         doClose();
-        success = true;
+      } catch (Throwable th) {
+        throwable = th;
       } finally {
-        if (!success) {
-          // Put reference back on failure
-          refCount.incrementAndGet();
+        try {
+          reportCloseToParentReaders();
+        } finally {
+          notifyReaderClosedListeners(throwable);
         }
       }
-      reportCloseToParentReaders();
-      notifyReaderClosedListeners();
     } else if (rc < 0) {
       throw new IllegalStateException("too many decRef calls: refCount is " + rc + " after decrement");
     }

Modified: lucene/dev/branches/lucene_solr_4_7/lucene/core/src/java/org/apache/lucene/index/SegmentCoreReaders.java
URL: http://svn.apache.org/viewvc/lucene/dev/branches/lucene_solr_4_7/lucene/core/src/java/org/apache/lucene/index/SegmentCoreReaders.java?rev=1581408&r1=1581407&r2=1581408&view=diff
==============================================================================
--- lucene/dev/branches/lucene_solr_4_7/lucene/core/src/java/org/apache/lucene/index/SegmentCoreReaders.java (original)
+++ lucene/dev/branches/lucene_solr_4_7/lucene/core/src/java/org/apache/lucene/index/SegmentCoreReaders.java Tue Mar 25 16:39:09 2014
@@ -17,14 +17,6 @@ package org.apache.lucene.index;
  * limitations under the License.
  */
 
-import java.io.IOException;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.LinkedHashSet;
-import java.util.Map;
-import java.util.Set;
-import java.util.concurrent.atomic.AtomicInteger;
-
 import org.apache.lucene.codecs.Codec;
 import org.apache.lucene.codecs.DocValuesProducer;
 import org.apache.lucene.codecs.FieldsProducer;
@@ -39,6 +31,14 @@ import org.apache.lucene.store.IOContext
 import org.apache.lucene.util.CloseableThreadLocal;
 import org.apache.lucene.util.IOUtils;
 
+import java.io.IOException;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.LinkedHashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicInteger;
+
 /** Holds core readers that are shared (unchanged) when
  * SegmentReader is cloned or reopened */
 final class SegmentCoreReaders {
@@ -173,19 +173,32 @@ final class SegmentCoreReaders {
   void decRef() throws IOException {
     if (ref.decrementAndGet() == 0) {
 //      System.err.println("--- closing core readers");
-      IOUtils.close(termVectorsLocal, fieldsReaderLocal, normsLocal, fields, termVectorsReaderOrig, fieldsReaderOrig, 
-          cfsReader, normsProducer);
-      notifyCoreClosedListeners();
+      Throwable th = null;
+      try {
+        IOUtils.close(termVectorsLocal, fieldsReaderLocal, normsLocal, fields, termVectorsReaderOrig, fieldsReaderOrig,
+            cfsReader, normsProducer);
+      } catch (Throwable throwable) {
+        th = throwable;
+      } finally {
+        notifyCoreClosedListeners(th);
+      }
     }
   }
   
-  private void notifyCoreClosedListeners() {
+  private void notifyCoreClosedListeners(Throwable th) {
     synchronized(coreClosedListeners) {
       for (CoreClosedListener listener : coreClosedListeners) {
         // SegmentReader uses our instance as its
         // coreCacheKey:
-        listener.onClose(this);
+        try {
+          listener.onClose(this);
+        } catch (Throwable t) {
+          if (th == null) {
+            th = t;
+          }
+        }
       }
+      IOUtils.reThrowUnchecked(th);
     }
   }
 

Modified: lucene/dev/branches/lucene_solr_4_7/lucene/core/src/java/org/apache/lucene/index/SegmentReader.java
URL: http://svn.apache.org/viewvc/lucene/dev/branches/lucene_solr_4_7/lucene/core/src/java/org/apache/lucene/index/SegmentReader.java?rev=1581408&r1=1581407&r2=1581408&view=diff
==============================================================================
--- lucene/dev/branches/lucene_solr_4_7/lucene/core/src/java/org/apache/lucene/index/SegmentReader.java (original)
+++ lucene/dev/branches/lucene_solr_4_7/lucene/core/src/java/org/apache/lucene/index/SegmentReader.java Tue Mar 25 16:39:09 2014
@@ -17,13 +17,6 @@ package org.apache.lucene.index;
  * limitations under the License.
  */
 
-import java.io.IOException;
-import java.util.ArrayList;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
-import java.util.Map.Entry;
-
 import org.apache.lucene.codecs.Codec;
 import org.apache.lucene.codecs.DocValuesFormat;
 import org.apache.lucene.codecs.DocValuesProducer;
@@ -36,6 +29,14 @@ import org.apache.lucene.store.Directory
 import org.apache.lucene.store.IOContext;
 import org.apache.lucene.util.Bits;
 import org.apache.lucene.util.CloseableThreadLocal;
+import org.apache.lucene.util.IOUtils;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Map.Entry;
 
 /**
  * IndexReader implementation over a single segment. 
@@ -250,9 +251,11 @@ public final class SegmentReader extends
       core.decRef();
     } finally {
       dvProducers.clear();
-      docValuesLocal.close();
-      docsWithFieldLocal.close();
-      segDocValues.decRef(dvGens);
+      try {
+        IOUtils.close(docValuesLocal, docsWithFieldLocal);
+      } finally {
+        segDocValues.decRef(dvGens);
+      }
     }
   }
 

Modified: lucene/dev/branches/lucene_solr_4_7/lucene/core/src/java/org/apache/lucene/util/IOUtils.java
URL: http://svn.apache.org/viewvc/lucene/dev/branches/lucene_solr_4_7/lucene/core/src/java/org/apache/lucene/util/IOUtils.java?rev=1581408&r1=1581407&r2=1581408&view=diff
==============================================================================
--- lucene/dev/branches/lucene_solr_4_7/lucene/core/src/java/org/apache/lucene/util/IOUtils.java (original)
+++ lucene/dev/branches/lucene_solr_4_7/lucene/core/src/java/org/apache/lucene/util/IOUtils.java Tue Mar 25 16:39:09 2014
@@ -17,6 +17,8 @@ package org.apache.lucene.util;
  * limitations under the License.
  */
 
+import org.apache.lucene.store.Directory;
+
 import java.io.BufferedReader;
 import java.io.Closeable;
 import java.io.File;
@@ -31,8 +33,6 @@ import java.nio.charset.Charset;
 import java.nio.charset.CharsetDecoder;
 import java.nio.charset.CodingErrorAction;
 
-import org.apache.lucene.store.Directory;
-
 /** This class emulates the new Java 7 "Try-With-Resources" statement.
  * Remove once Lucene is on Java 7.
  * @lucene.internal */
@@ -357,6 +357,17 @@ public final class IOUtils {
       if (th instanceof IOException) {
         throw (IOException) th;
       }
+      reThrowUnchecked(th);
+    }
+  }
+
+  /**
+   * Simple utilty method that takes a previously caught
+   * {@code Throwable} and rethrows it as an unchecked exception.
+   * If the argument is null then this method does nothing.
+   */
+  public static void reThrowUnchecked(Throwable th) {
+    if (th != null) {
       if (th instanceof RuntimeException) {
         throw (RuntimeException) th;
       }

Copied: lucene/dev/branches/lucene_solr_4_7/lucene/core/src/test/org/apache/lucene/index/TestIndexReaderClose.java (from r1581404, lucene/dev/branches/branch_4x/lucene/core/src/test/org/apache/lucene/index/TestIndexReaderClose.java)
URL: http://svn.apache.org/viewvc/lucene/dev/branches/lucene_solr_4_7/lucene/core/src/test/org/apache/lucene/index/TestIndexReaderClose.java?p2=lucene/dev/branches/lucene_solr_4_7/lucene/core/src/test/org/apache/lucene/index/TestIndexReaderClose.java&p1=lucene/dev/branches/branch_4x/lucene/core/src/test/org/apache/lucene/index/TestIndexReaderClose.java&r1=1581404&r2=1581408&rev=1581408&view=diff
==============================================================================
--- lucene/dev/branches/branch_4x/lucene/core/src/test/org/apache/lucene/index/TestIndexReaderClose.java (original)
+++ lucene/dev/branches/lucene_solr_4_7/lucene/core/src/test/org/apache/lucene/index/TestIndexReaderClose.java Tue Mar 25 16:39:09 2014
@@ -50,7 +50,7 @@ public class TestIndexReaderClose extend
           }
         }
       };
-      List<IndexReader.ReaderClosedListener> listeners = new ArrayList<>();
+      List<IndexReader.ReaderClosedListener> listeners = new ArrayList<IndexReader.ReaderClosedListener>();
       int listenerCount = random().nextInt(20);
       AtomicInteger count = new AtomicInteger();
       boolean faultySet = false;