svn commit: r628921 - /lucene/java/trunk/contrib/xml-query-parser/src/java/org/apache/lucene/xmlparser/builders/CachedFilterBuilder.java

[mh...@apache.org]

Author: mharwood
Date: Mon Feb 18 15:39:12 2008
New Revision: 628921

URL: http://svn.apache.org/viewvc?rev=628921&view=rev
Log:
Additional thread safety around filter creation - old code could create duplicate CachingWrapperFilter if thread1 gets cache miss and thread 2 has a cache miss before thread1 populates cache with new CachingWrapperFilter.
Synchronization cost around whole method is OK here because Filter object construction should be a lightweight call. 
Note: CachingWrapperFilter currently has a similar bug in bits() method but adding "synchronized" around that whole method would not be a solution there because of the cost of evaluating filter.bits and the unnecessary blocking effect this would have on threads using different readers to the thread with the lock.

 

Modified:
    lucene/java/trunk/contrib/xml-query-parser/src/java/org/apache/lucene/xmlparser/builders/CachedFilterBuilder.java

Modified: lucene/java/trunk/contrib/xml-query-parser/src/java/org/apache/lucene/xmlparser/builders/CachedFilterBuilder.java
URL: http://svn.apache.org/viewvc/lucene/java/trunk/contrib/xml-query-parser/src/java/org/apache/lucene/xmlparser/builders/CachedFilterBuilder.java?rev=628921&r1=628920&r2=628921&view=diff
==============================================================================
--- lucene/java/trunk/contrib/xml-query-parser/src/java/org/apache/lucene/xmlparser/builders/CachedFilterBuilder.java (original)
+++ lucene/java/trunk/contrib/xml-query-parser/src/java/org/apache/lucene/xmlparser/builders/CachedFilterBuilder.java Mon Feb 18 15:39:12 2008
@@ -66,7 +66,7 @@
 		this.cacheSize=cacheSize;
 	}
 
-	public Filter getFilter(Element e) throws ParserException
+	public synchronized Filter getFilter(Element e) throws ParserException
 	{
 
 		Element childElement = DOMUtils.getFirstChildOrFail(e);
@@ -78,8 +78,7 @@
 
 		// Test to see if child Element is a query or filter that needs to be
 		// cached
-		QueryBuilder qb = queryFactory.getQueryBuilder(childElement
-				.getNodeName());
+		QueryBuilder qb = queryFactory.getQueryBuilder(childElement.getNodeName());
 		Object cacheKey = null;
 		Query q = null;
 		Filter f = null;
@@ -92,14 +91,10 @@
 			f = filterFactory.getFilter(childElement);
 			cacheKey = f;
 		}
-		Filter cachedFilter = null;
-		synchronized (filterCache)
-		{ // check cache
-			cachedFilter = (Filter) filterCache.get(cacheKey);
-			if (cachedFilter != null)
-			{
-				return cachedFilter; // cache hit
-			}
+		Filter cachedFilter = (Filter) filterCache.get(cacheKey);
+		if (cachedFilter != null)
+		{
+			return cachedFilter; // cache hit
 		}
 		
 		//cache miss
@@ -111,10 +106,7 @@
 			cachedFilter = new CachingWrapperFilter(f);
 		}
 
-		synchronized (filterCache)
-		{ // update cache
-			filterCache.put(cacheKey, cachedFilter);
-		}
+		filterCache.put(cacheKey, cachedFilter);
 		return cachedFilter;
 	}