Possible thread safety problem in CachingWrapperFilter

I'm trying to change the CachingWrapperFilter class to cache
a
SortedVIntList for LUCENE-584. That is progressing nicely,
but
I found this snippet at the beginning of the current
CachingWrapperFilter.bits() method:

    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:

synchronized (cache) { ... cache.get(...); } 
...
synchronized (cache) { cache.put(...); } 

Would this be safe to initialize the cache:

synchronized(this) {
    if (cache == null) {
      cache = new WeakHashMap();
    }
}

and should the cache accesses also use synchronized(this) ?

Regards,
Paul Elschot

------------------------------------------------------------
---------
To unsubscribe, e-mail: java-dev-unsubscribelucene.apache.org
For additional commands, e-mail: java-dev-helplucene.apache.org

:     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.

: and should the cache accesses also use synchronized(this)
?

I can't see a need for that ... then the same instance
couldn't be used on 
two distinct readers in parallel threads ... but that should
be fine.



-Hoss


------------------------------------------------------------
---------
To unsubscribe, e-mail: java-dev-unsubscribelucene.apache.org
For additional commands, e-mail: java-dev-helplucene.apache.org

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.

That would indeed fix the problem.
 
> : and should the cache accesses also use
synchronized(this) ?
> 
> I can't see a need for that ... then the same instance
couldn't be used on 
> two distinct readers in parallel threads ... but that
should be fine.

An alternative fix would be to have two synchronized(this)
blocks with
the first one containing the lazy initialisation of the
cache.

A patch for this will probably conflict with LUCENE-584 on 
CachingWrapperFilter, so I'd rather not provide a patch
myself this time.

Regards,
Paul Elschot

------------------------------------------------------------
---------
To unsubscribe, e-mail: java-dev-unsubscribelucene.apache.org
For additional commands, e-mail: java-dev-helplucene.apache.org

On 9/4/07, Chris Hostetter <hossman_lucenefucit.org> wrote:
> : and should the cache accesses also use
synchronized(this) ?
>
> I can't see a need for that ... then the same instance
couldn't be used on
> two distinct readers in parallel threads ... but that
should be fine.

synchronized (this) will provide the same level of
concurrency as
synchronized (cache)

A related point: watch out for subclass use of
"cache".

-Yonik

------------------------------------------------------------
---------
To unsubscribe, e-mail: java-dev-unsubscribelucene.apache.org
For additional commands, e-mail: java-dev-helplucene.apache.org

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-unsubscribelucene.apache.org
For additional commands, e-mail: java-dev-helplucene.apache.org

: 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.

Ahhh... that actally makes a lot of sense.  transient fields
won't be 
saved/restored in serialization, so initing only in the
constructor isn't 
good enough, after it's been serialized it will be null.

: 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

I can't even fathom what it would mean to be both final and
transient ... 
they seem diametricaly opposed to me (but it doesn't
generate a compiler 
warning, so it must mean something)



-Hoss


------------------------------------------------------------
---------
To unsubscribe, e-mail: java-dev-unsubscribelucene.apache.org
For additional commands, e-mail: java-dev-helplucene.apache.org