lucene-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Paul Elschot <paul.elsc...@xs4all.nl>
Subject Re: Possible thread safety problem in CachingWrapperFilter
Date Wed, 05 Sep 2007 20:52:25 GMT
On Tuesday 04 September 2007 21:03, Chris Hostetter wrote:
> 
> :     if (cache == null) {
> :       cache = new WeakHashMap();
> :     }
> : 
> : I think the initial snippet is not thread safe and might result
> : in two threads initializing this cache to different objects,
> : possibly conflicting with the cache accesses after that:
> 
> i believe you are write ... if Thread A evaluates the "if (cache == null)" 
> op and then Thread B is given priority and executes the entire method, 
> when Thread A resumes it will blow away the old cache instance (and all of 
> B's hard work)
> 
> I suspect there wasn't a synchro block arround that bit of code because 
> synchronizing on an expression that evaluates to null returns an NPE 
> 
> : Would this be safe to initialize the cache:
> : 
> : synchronized(this) {
> :     if (cache == null) {
> :       cache = new WeakHashMap();
> :     }
> : }
> 
> for the life of me i can't imaging why "cache = new WeakHashMap();" isn't 
> just in the constructor.  then it's garunteed to only execute once.
> 

I tried initializing the cache in the constructor, but then the test case 
failed with a null pointer exception on the first synchronized(cache) .
Very strange, this seems to be a bug in the implementation of java 
serialisation. The cache variable is transient, so it should not be affected
at all by serialisation, but it appears to be affected nonetheless.

I had a quick look at the java serialisation spec, and iirc it was
also mentioned that one might use a final variable initialized in
the constructor. I did not try that (final transient cache), i preferred
to put the lazy initialization of cache in the first synchronized(this)
block.

I'll post a new patch at LUCENE-584 soon, and this will have two
synchronized(this) blocks in CachingWrapperFilter with a lazy
initialisation of cache in the first block.
In the patch, the caching has moved from the bits() method to
the getMatcher() method of CachingWrapperFilter.

Regards,
Paul Elschot

---------------------------------------------------------------------
To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-dev-help@lucene.apache.org


Mime
View raw message