IndexReader.close() contract

[Dawid Weiss <da...@gmail.com>]

I'm looking at IndexReader.close and keep wondering:

  /**
   * Closes files associated with this index.
   * Also saves any new deletions to disk.
   * No other methods should be called after this has been called.
   * @throws IOException if there is a low-level IO error
   */
  @Override
  public final synchronized void close() throws IOException {
    if (!closed) {
      decRef();
      closed = true;
    }
  }

If you have refCount > 1 this leads to an odd scenario in which the
decRef decreases the counter, but does not call doClose. It also sets 'closed'
to true, but the object remains functional (since ensureClosed checks
refCount only).

Wouldn't it be better for close() to actually check the refCount and
(if closed == false):

- always call doClose
- mark the object as closed (closed = true)
- if refCount != 1 => unpaired incRef/decRef somewhere, signal an exception?

Then the javadoc would stay true, even if somebody played with reference counts.

Dawid

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

Re: IndexReader.close() contract

[Dawid Weiss <da...@gmail.com>]

Also applicable to the general discussion above -- just noticed that
decRef is not synchronized and closed is a regular variable so
doClose() can be called from both close and decRef under a race
condition. I know ref counting is an "expert feature", but I think it
could better agree with close(), where calling close means the
definitive end of the reader's life cycle.

Dawid

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

Re: IndexReader.close() contract

[Dawid Weiss <da...@gmail.com>]

> Regarding your point about concurrent calls to close() and decref(), I don't
> see how doClose() may be called twice. The `closed` boolean is only used to
> make close() idempotent. Do I miss something?

No, sorry -- you're right, too much looking at this.

> It is just a practical thing for now until there is a correct close()
> in java itself for this kind of thing that won't crash the VM.

Ok, fine.

D.

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

Re: IndexReader.close() contract

[Robert Muir <rc...@gmail.com>]

On Wed, Mar 28, 2018 at 9:59 AM, Adrien Grand <jp...@gmail.com> wrote:
> I'd
> still be interested in other opinions regarding whether it would be ok to
> rely more on ByteBufferGuard to have exceptions rather than segfaults when
> reading closed index inputs given that comments say it is only a best
> effort.
>

I think that would be a mistake. this logic tries to reduce the window
as much as possible, without impacting performance but it is not
"correct" for multiple threads: it uses a regular 'boolean' variable.
It is just a practical thing for now until there is a correct close()
in java itself for this kind of thing that won't crash the VM.

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

Re: IndexReader.close() contract

[Adrien Grand <jp...@gmail.com>]

I agree with your points in general. I didn't have the code available to
check when I first replied but it looks like the error message from the
index inputs would be ok if you keep using the reader in another thread.
I'd still be interested in other opinions regarding whether it would be ok
to rely more on ByteBufferGuard to have exceptions rather than segfaults
when reading closed index inputs given that comments say it is only a best
effort.

Regarding your point about concurrent calls to close() and decref(), I
don't see how doClose() may be called twice. The `closed` boolean is only
used to make close() idempotent. Do I miss something?

Re: IndexReader.close() contract

[Dawid Weiss <da...@gmail.com>]

Hi Adrien,

> I'm afraid that always calling doClose would be problematic. If the reader
> is used in another thread then files might get closed under the hood while a
> search is being performed. Even if the guard that we added to MMapDirectory
> would now prevent segfaults, users would get potentially confusing
> exception.

Isn't it the right behavior though? Think of any closeable resource --
a file channel or even a stream. When you close it, other threads
using it should get an exception (that's the point of closing the
channel). The reader's contract is inconsistent to me here. If there
are multiple threads using it they should incRef/ decRef consistently
(there are assertions that verify sanity of this) and if it's a single
thread then calling close() should make the reader stop serving any
requests.

The situation you're describing above does sound like a real bug to me
where the reader is closed yet other threads still make happy use of
it -- I just can't imagine a real-life situation where this doesn't
signal a threading/ use case problem?

> I see pros and cons regarding checking "if (closed)" in ensureOpen. In any
> case I think we should keep a way to let ongoing searches finish
> successfully before effectively closing the reader, which I suspect users
> rely on.

This is the bit I find strange, actually. Converting to sequential
code, it'd be like this.

DirectoryReader reader = openReader();
IndexSearcher searcher = new IndexSearcher(reader);
reader.close();
searcher.search(new TermQuery(new Term("foo", "bar")), 10);

The above in a purely sequential case does not work (as expected). The
searcher doesn't incRef the reader's counter, so closing the reader
closes all its resources.

In my opinion ff there is code out there that relies on incRefs then
it shouldn't be calling close at all (and use decRef which closes the
reader when the ref count drops to zero). The close() on such a reader
should result in any subsequent searches being terminated (with the
AlreadyClosedException). There are efficient way to make sure all
search threads using a searcher in parallel terminate (join them,
await thread pool's termination)...

Conversely, you could argue that if you have parallel code that uses
ref counts and then wish to abort you won't be able to close such a
reader at all (because you don't know how many decRefs to call)?

Dawid

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

Re: IndexReader.close() contract

[Adrien Grand <jp...@gmail.com>]

I'm afraid that always calling doClose would be problematic. If the reader
is used in another thread then files might get closed under the hood while
a search is being performed. Even if the guard that we added to
MMapDirectory would now prevent segfaults, users would get potentially
confusing exception.

I see pros and cons regarding checking "if (closed)" in ensureOpen. In any
case I think we should keep a way to let ongoing searches finish
successfully before effectively closing the reader, which I suspect users
rely on.

Le mar. 27 mars 2018 à 15:19, Dawid Weiss <da...@gmail.com> a écrit :

> I'm looking at IndexReader.close and keep wondering:
>
>   /**
>    * Closes files associated with this index.
>    * Also saves any new deletions to disk.
>    * No other methods should be called after this has been called.
>    * @throws IOException if there is a low-level IO error
>    */
>   @Override
>   public final synchronized void close() throws IOException {
>     if (!closed) {
>       decRef();
>       closed = true;
>     }
>   }
>
> If you have refCount > 1 this leads to an odd scenario in which the
> decRef decreases the counter, but does not call doClose. It also sets
> 'closed'
> to true, but the object remains functional (since ensureClosed checks
> refCount only).
>
> Wouldn't it be better for close() to actually check the refCount and
> (if closed == false):
>
> - always call doClose
> - mark the object as closed (closed = true)
> - if refCount != 1 => unpaired incRef/decRef somewhere, signal an
> exception?
>
> Then the javadoc would stay true, even if somebody played with reference
> counts.
>
> Dawid
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
> For additional commands, e-mail: dev-help@lucene.apache.org
>
>