You are viewing a plain text version of this content. The canonical link for it is here.
Posted to java-user@lucene.apache.org by Alexey Lef <al...@sciquest.com> on 2011/04/26 14:46:21 UTC
IndexReader.close() behavior
This is the code in IndexReader.close():
public final synchronized void close() throws IOException {
if (!closed) {
decRef();
closed = true;
}
}
What strikes me as odd is that “closed” variable is set to true regardless of whether the index was actually closed using doClose(). It is causing a bit of a problem in the following setup.
We have many indexes. Some of these indexes are kept open and IndexReader is reused for performance reasons, some are open just for a single search. Some of them are combined together at the search time depending on the user performing the search. For every search, if there are multiple relevant IndexReader, we create a new MultiReader instance and close it once the search is done. Since reference counting is already built into the IndexReader, I figured I can just call incRef() when using a cached IndexReader and then call close() or let MultiReader call close() on it once it is done. The problem is only the first close() calls decRef(),subsequent calls do nothing, and if there were multiple incRef() calls, the IndexReader ends up being left open.
I thought of using decRef() instead of close() but MultiReader doesn’t have an option of calling decRef() on subreaders, only close().
The following change to IndexReader.close() fixes the problem:
public final synchronized void close() throws IOException {
if (refCount.get() > 0) {
decRef();
}
}
It seems more logical. Also it is now consistent with ensureOpen() method.
Am I missing something here? Is there a good reason to prevent close() from doing anything after the first call even if the IndexReader was not actually closed? Does it even need to be synchronized if decRef() does all the work?
Thanks,
Alexey
Re: IndexReader.close() behavior
Posted by Mindaugas Žakšauskas <mi...@gmail.com>.
Hi,
In my quest to fight a similar problem, I ended up writing a wrapper
finalizer (sic!) that simply closes the underlying reader/searcher. It
might be against all advices (e.g. "Effective Java 2ed" Item 7) but I
simply couldn't find any better solution to this problem.
I wish I wouldn't need doing close() myself. Hopefully this will be
improved with Java 7's AutoCloseable one day.
Regards,
Mindaugas
On Fri, May 13, 2011 at 2:08 PM, Alexey Lef <al...@sciquest.com> wrote:
> Apologies for a long silence - have been fighting a nasty bug of a non-computer variety.
>
> We are using 3.0.3. I was trying to upgrade to 3.1 and clear all deprecation warnings. What I described was an attempt to switch from MultiSearcher to MultiReader. We had a simple delegating IndexSearcher wrapper for cached/reused IndexSearchers that was doing reference counting. <..>
---------------------------------------------------------------------
To unsubscribe, e-mail: java-user-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-user-help@lucene.apache.org
RE: IndexReader.close() behavior
Posted by Alexey Lef <al...@sciquest.com>.
Apologies for a long silence - have been fighting a nasty bug of a non-computer variety.
We are using 3.0.3. I was trying to upgrade to 3.1 and clear all deprecation warnings. What I described was an attempt to switch from MultiSearcher to MultiReader. We had a simple delegating IndexSearcher wrapper for cached/reused IndexSearchers that was doing reference counting. Its close() method was overridden to perform a real close() only if ref count was 0. I could have any combination of single-use IndexSearchers and shared/cached IndexSearchers in a MultiSearcher and everything worked beautifully on close().
So, my first choice was to use the incRef()/close() in IndexReader, but IndexReader.close() behavior prevents that.
The second thought was creating a delegating wrapper for IndexReader similarly to what I had for IndexSearcher but there are so many methods, and half of them are final including close(). So, that doesn't work either.
I don't think I can let MultiReader call both incRef() and decRef() on sub readers for two reasons: single-use (non-shared) readers need to just be closed; and for shared readers - there is a race condition between getting a shared reader from cache and calling incRef(). There is a potential that during that time the reader may get decRef()'ed and closed due to, e.g. a new version of an index or cache expiration. So, a shared reader has to be incRef()'ed at the time I get it from cache.
Finally, I ended up creating a simple container interface to house the IndexReader and pass it around:
public interface IndexReaderContainer {
IndexReader getIndexReader();
void close() throws IOException;
}
I have 3 implementations of this interface:
single-use - close() simply closes IndexReader,
shared - does reference counting and closes IndexReader only when ref count = 0, and
multi-reader - keeps track of sub-IndexReaderContainer's and closes each one on close(). MultiReader is created with closeSubReaders=false.
The application code always calls IndexReaderContainer.close(), never IndexReader.close() directly.
Not very pretty but this way I have my combination of single-use and shared indexes, and I don't have to change Lucene code.
Personally, I have no sympathy for people who double-close their IndexReaders - let them get an exception, but I understand that it is not easy to change something that's been working in a particular way for years.
Thanks,
Alexey
-----Original Message-----
From: Michael McCandless [mailto:lucene@mikemccandless.com]
Sent: Tuesday, April 26, 2011 11:20 PM
To: java-user@lucene.apache.org
Subject: Re: IndexReader.close() behavior
The code is tricky, but it's intentional.
We always set closed to true to guard against double close, ie, it's
fine to double-close an IndexReader, ie doing so will not "steal"
references from other places that have incRef'd the reader.
Can you pass closeSubReaders=false when you create your MultiReader?
This way, the MultiReader incRefs on init and decRefs on close.
But: what Lucene version are you using...?
Mike
http://blog.mikemccandless.com
On Tue, Apr 26, 2011 at 8:46 AM, Alexey Lef <al...@sciquest.com> wrote:
> This is the code in IndexReader.close():
>
> public final synchronized void close() throws IOException {
> if (!closed) {
> decRef();
> closed = true;
> }
> }
>
> What strikes me as odd is that “closed” variable is set to true regardless of whether the index was actually closed using doClose(). It is causing a bit of a problem in the following setup.
>
> We have many indexes. Some of these indexes are kept open and IndexReader is reused for performance reasons, some are open just for a single search. Some of them are combined together at the search time depending on the user performing the search. For every search, if there are multiple relevant IndexReader, we create a new MultiReader instance and close it once the search is done. Since reference counting is already built into the IndexReader, I figured I can just call incRef() when using a cached IndexReader and then call close() or let MultiReader call close() on it once it is done. The problem is only the first close() calls decRef(),subsequent calls do nothing, and if there were multiple incRef() calls, the IndexReader ends up being left open.
>
> I thought of using decRef() instead of close() but MultiReader doesn’t have an option of calling decRef() on subreaders, only close().
>
> The following change to IndexReader.close() fixes the problem:
>
> public final synchronized void close() throws IOException {
> if (refCount.get() > 0) {
> decRef();
> }
> }
>
> It seems more logical. Also it is now consistent with ensureOpen() method.
>
> Am I missing something here? Is there a good reason to prevent close() from doing anything after the first call even if the IndexReader was not actually closed? Does it even need to be synchronized if decRef() does all the work?
>
> Thanks,
>
> Alexey
>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: java-user-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-user-help@lucene.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: java-user-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-user-help@lucene.apache.org
Re: IndexReader.close() behavior
Posted by Michael McCandless <lu...@mikemccandless.com>.
The code is tricky, but it's intentional.
We always set closed to true to guard against double close, ie, it's
fine to double-close an IndexReader, ie doing so will not "steal"
references from other places that have incRef'd the reader.
Can you pass closeSubReaders=false when you create your MultiReader?
This way, the MultiReader incRefs on init and decRefs on close.
But: what Lucene version are you using...?
Mike
http://blog.mikemccandless.com
On Tue, Apr 26, 2011 at 8:46 AM, Alexey Lef <al...@sciquest.com> wrote:
> This is the code in IndexReader.close():
>
> public final synchronized void close() throws IOException {
> if (!closed) {
> decRef();
> closed = true;
> }
> }
>
> What strikes me as odd is that “closed” variable is set to true regardless of whether the index was actually closed using doClose(). It is causing a bit of a problem in the following setup.
>
> We have many indexes. Some of these indexes are kept open and IndexReader is reused for performance reasons, some are open just for a single search. Some of them are combined together at the search time depending on the user performing the search. For every search, if there are multiple relevant IndexReader, we create a new MultiReader instance and close it once the search is done. Since reference counting is already built into the IndexReader, I figured I can just call incRef() when using a cached IndexReader and then call close() or let MultiReader call close() on it once it is done. The problem is only the first close() calls decRef(),subsequent calls do nothing, and if there were multiple incRef() calls, the IndexReader ends up being left open.
>
> I thought of using decRef() instead of close() but MultiReader doesn’t have an option of calling decRef() on subreaders, only close().
>
> The following change to IndexReader.close() fixes the problem:
>
> public final synchronized void close() throws IOException {
> if (refCount.get() > 0) {
> decRef();
> }
> }
>
> It seems more logical. Also it is now consistent with ensureOpen() method.
>
> Am I missing something here? Is there a good reason to prevent close() from doing anything after the first call even if the IndexReader was not actually closed? Does it even need to be synchronized if decRef() does all the work?
>
> Thanks,
>
> Alexey
>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: java-user-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-user-help@lucene.apache.org