From 2ffc3bacd84df7e8867e0c5b77a60da97322f232 Mon Sep 17 00:00:00 2001
From: Uwe Schindler <uschindler@apache.org>
Date: Sun, 8 Nov 2009 21:46:20 +0000
Subject: [PATCH] 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                                   |  4 ++
 .../lucene/search/CachingSpanFilter.java      | 37 ++++++++++++-------
 .../lucene/search/CachingWrapperFilter.java   | 36 ++++++++++--------
 3 files changed, 47 insertions(+), 30 deletions(-)

diff --git a/CHANGES.txt b/CHANGES.txt
index 7681f89aa03..428a6ea1951 100644
--- a/CHANGES.txt
+++ b/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
diff --git a/src/java/org/apache/lucene/search/CachingSpanFilter.java b/src/java/org/apache/lucene/search/CachingSpanFilter.java
index 9dda4c6a3fc..d8284f9ce53 100644
--- a/src/java/org/apache/lucene/search/CachingSpanFilter.java
+++ b/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;
-    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);
+    lock.lock();
+    try {
+      if (cache == null) {
+        cache = new WeakHashMap<IndexReader,SpanFilterResult>();
       }
+      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;
   }
 
diff --git a/src/java/org/apache/lucene/search/CachingWrapperFilter.java b/src/java/org/apache/lucene/search/CachingWrapperFilter.java
index c341f33952e..9b2956162c4 100644
--- a/src/java/org/apache/lucene/search/CachingWrapperFilter.java
+++ b/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) {
-      cache = new WeakHashMap<IndexReader, DocIdSet>();
-    }
+    lock.lock();
+    try {
+      if (cache == null) {
+        cache = new WeakHashMap<IndexReader,DocIdSet>();
+      }
 
-    DocIdSet cached = null;
-    synchronized (cache) {  // check cache
-      cached = cache.get(reader);
-    }
-
-    if (cached != null) {
-      return cached;
+      final DocIdSet cached = cache.get(reader);
+      if (cached != null) return cached;
+    } finally {
+      lock.unlock();
     }
 
     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;
   }