LUCENE-2030: Fix locks in CachingWrapperFilter and CachingSpanFilter (make members private, also synchronize on WeakHashMap build, use new Java5 ReentrantLock)

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

CHANGES.txt

@@ -12,6 +12,10 @@ Changes in backwards compatibility policy
 
 * oal.Lock.isLocked is now allowed to throw an IOException
 
+* LUCENE-2030: CachingWrapperFilter and CachingSpanFilter now hide
+  the internal cache implementation for thread safety, before it was
+  declared protected.  (Peter Lenahan, Uwe Schindler, Simon Willnauer)
+
 Changes in runtime behavior
 
 * LUCENE-1677: Remove the system property to set SegmentReader class

src/java/org/apache/lucene/search/CachingSpanFilter.java

@@ -19,21 +19,23 @@ package org.apache.lucene.search;
 import org.apache.lucene.index.IndexReader;
 
 import java.io.IOException;
-
 import java.util.Map;
 import java.util.WeakHashMap;
+import java.util.concurrent.locks.ReentrantLock;
 
 /**
  * Wraps another SpanFilter's result and caches it.  The purpose is to allow
  * filters to simply filter, and then wrap with this class to add caching.
  */
 public class CachingSpanFilter extends SpanFilter {
-  protected SpanFilter filter;
+  private SpanFilter filter;
 
   /**
-   * A transient Filter cache.
+   * A transient Filter cache (package private because of test)
    */
-  protected transient Map<IndexReader,SpanFilterResult> cache;
+  private transient Map<IndexReader,SpanFilterResult> cache;
+
+  private final ReentrantLock lock = new ReentrantLock();
 
   /**
    * @param filter Filter to cache results of
@@ -49,18 +51,25 @@ public class CachingSpanFilter extends SpanFilter {
   }
   
   private SpanFilterResult getCachedResult(IndexReader reader) throws IOException {
-    SpanFilterResult result = null;
+    lock.lock();
-    if (cache == null) {
+    try {
-      cache = new WeakHashMap<IndexReader,SpanFilterResult>();
+      if (cache == null) {
-    }
+        cache = new WeakHashMap<IndexReader,SpanFilterResult>();
-
-    synchronized (cache) {  // check cache
-      result = cache.get(reader);
-      if (result == null) {
-        result = filter.bitSpans(reader);
-        cache.put(reader, result);
       }
+      final SpanFilterResult cached = cache.get(reader);
+      if (cached != null) return cached;
+    } finally {
+      lock.unlock();
     }
+    
+    final SpanFilterResult result = filter.bitSpans(reader);
+    lock.lock();
+    try {
+      cache.put(reader, result);
+    } finally {
+      lock.unlock();
+    }
+    
     return result;
   }
 

src/java/org/apache/lucene/search/CachingWrapperFilter.java

@@ -20,6 +20,7 @@ package org.apache.lucene.search;
 import java.io.IOException;
 import java.util.Map;
 import java.util.WeakHashMap;
+import java.util.concurrent.locks.ReentrantLock;
 
 import org.apache.lucene.index.IndexReader;
 import org.apache.lucene.util.OpenBitSetDISI;
@@ -29,12 +30,14 @@ import org.apache.lucene.util.OpenBitSetDISI;
  * filters to simply filter, and then wrap with this class to add caching.
  */
 public class CachingWrapperFilter extends Filter {
-  protected Filter filter;
+  Filter filter;
 
   /**
-   * A transient Filter cache.
+   * A transient Filter cache (package private because of test)
    */
-  protected transient Map<IndexReader, DocIdSet> cache;
+  transient Map<IndexReader, DocIdSet> cache;
+  
+  private final ReentrantLock lock = new ReentrantLock();
 
   /**
    * @param filter Filter to cache results of
@@ -63,27 +66,28 @@ public class CachingWrapperFilter extends Filter {
   
   @Override
   public DocIdSet getDocIdSet(IndexReader reader) throws IOException {
-    if (cache == null) {
+    lock.lock();
-      cache = new WeakHashMap<IndexReader, DocIdSet>();
+    try {
-    }
+      if (cache == null) {
+        cache = new WeakHashMap<IndexReader,DocIdSet>();
+      }
 
-    DocIdSet cached = null;
+      final DocIdSet cached = cache.get(reader);
-    synchronized (cache) {  // check cache
+      if (cached != null) return cached;
-      cached = cache.get(reader);
+    } finally {
-    }
+      lock.unlock();
-
-    if (cached != null) {
-      return cached;
     }
 
     final DocIdSet docIdSet = docIdSetToCache(filter.getDocIdSet(reader), reader);
-
     if (docIdSet != null) {
-      synchronized (cache) {  // update cache
+      lock.lock();
+      try {
         cache.put(reader, docIdSet);
+      } finally {
+        lock.unlock();
       }
     }
-
+    
     return docIdSet;
   }