Lucene - Core
  1. Lucene - Core
  2. LUCENE-1453

When reopen returns a new IndexReader, both IndexReaders may now control the lifecycle of the underlying Directory which is managed by reference counting

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.4.1, 2.9
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      Rough summary. Basically, FSDirectory tracks references to FSDirectory and when IndexReader.reopen shares a Directory with a created IndexReader and closeDirectory is true, FSDirectory's ref management will see two decrements for one increment. You can end up getting an AlreadyClosed exception on the Directory when the IndexReader is open.

      I have a test I'll put up. A solution seems fairly straightforward (at least in what needs to be accomplished).

      1. LUCENE-1453-fix-TestIndexReader.patch
        3 kB
        Uwe Schindler
      2. LUCENE-1453.patch
        22 kB
        Uwe Schindler
      3. LUCENE-1453.patch
        21 kB
        Uwe Schindler
      4. LUCENE-1453-with-FSDir-open.patch
        23 kB
        Uwe Schindler
      5. LUCENE-1453.patch
        17 kB
        Uwe Schindler
      6. Failing-testcase-LUCENE-1453.patch
        9 kB
        Uwe Schindler
      7. LUCENE-1453.patch
        6 kB
        Michael McCandless
      8. LUCENE-1453.patch
        5 kB
        Michael McCandless
      9. LUCENE-1453.patch
        4 kB
        Mark Miller

        Issue Links

          Activity

          Hide
          Michael McCandless added a comment -

          OK thanks Uwe!

          Show
          Michael McCandless added a comment - OK thanks Uwe!
          Hide
          Uwe Schindler added a comment -

          Committed revision 783314. Thanks Mike! Next time I will also test-tag, sorry.

          I will go through all other tests using FSDir.open() in trunk now and check, if there are more cases, that rely on refcounting. They can be found easily, because they catch AlreadyClosedException.

          Show
          Uwe Schindler added a comment - Committed revision 783314. Thanks Mike! Next time I will also test-tag, sorry. I will go through all other tests using FSDir.open() in trunk now and check, if there are more cases, that rely on refcounting. They can be found easily, because they catch AlreadyClosedException.
          Hide
          Uwe Schindler added a comment -

          This fixes this special case and the test on trunk to also hit this, I commit soon!

          Show
          Uwe Schindler added a comment - This fixes this special case and the test on trunk to also hit this, I commit soon!
          Hide
          Uwe Schindler added a comment - - edited

          Thanks Mike,
          it is from this fix. The test should normally also fail with trunk, but it doesn't because we are using FSDir.open() in the trunk test. This is another test that relies on the refcounting of FSDir.getDirectory.

          The problem:
          If you do IndexReader.open() on a invalid index and IndexReader.open fails with an Exception, the Directory keeps open (because the wrapper has no chance to close it). I'll fix this and also enable FSDir.getDirectory for this test in trunk.

          Show
          Uwe Schindler added a comment - - edited Thanks Mike, it is from this fix. The test should normally also fail with trunk, but it doesn't because we are using FSDir.open() in the trunk test. This is another test that relies on the refcounting of FSDir.getDirectory. The problem: If you do IndexReader.open() on a invalid index and IndexReader.open fails with an Exception, the Directory keeps open (because the wrapper has no chance to close it). I'll fix this and also enable FSDir.getDirectory for this test in trunk.
          Hide
          Michael McCandless added a comment -

          I'm seeing a failure in back-compat tests ("and test-tag -Dtestcase=TestIndexReader"):

              [junit] Testcase: testFalseDirectoryAlreadyClosed(org.apache.lucene.index.TestIndexReader):	FAILED
              [junit] did not hit expected exception
              [junit] junit.framework.AssertionFailedError: did not hit expected exception
              [junit] 	at org.apache.lucene.index.TestIndexReader.testFalseDirectoryAlreadyClosed(TestIndexReader.java:1514)
          

          (I assume, but I'm not certain, it's from this fix...).

          Show
          Michael McCandless added a comment - I'm seeing a failure in back-compat tests ("and test-tag -Dtestcase=TestIndexReader"): [junit] Testcase: testFalseDirectoryAlreadyClosed(org.apache.lucene.index.TestIndexReader): FAILED [junit] did not hit expected exception [junit] junit.framework.AssertionFailedError: did not hit expected exception [junit] at org.apache.lucene.index.TestIndexReader.testFalseDirectoryAlreadyClosed(TestIndexReader.java:1514) (I assume, but I'm not certain, it's from this fix...).
          Hide
          Uwe Schindler added a comment -

          Committed revision 783280.

          2.4 branch is untouched, if backporting is needed (because somebody has problems with reopen/clone), reopen this issue!

          Show
          Uwe Schindler added a comment - Committed revision 783280. 2.4 branch is untouched, if backporting is needed (because somebody has problems with reopen/clone), reopen this issue!
          Hide
          Uwe Schindler added a comment -

          Mike: OK, I commit the latest patch soon!

          Earwin:

          Closeable is a Java 1.5 interface only, so this refactoring must wait until 3.0, but the idea is good!

          We can introduce our own Closeable, and replace it with java native in 3.0, thank gods the interface is simple

          I think you should open an issue about that. But the own closeable should be declared as deprecated from the beginning with the note "will be replaced by java.io.Closeable" in 3.0.

          Show
          Uwe Schindler added a comment - Mike: OK, I commit the latest patch soon! Earwin: Closeable is a Java 1.5 interface only, so this refactoring must wait until 3.0, but the idea is good! We can introduce our own Closeable, and replace it with java native in 3.0, thank gods the interface is simple I think you should open an issue about that. But the own closeable should be declared as deprecated from the beginning with the note "will be replaced by java.io.Closeable" in 3.0.
          Hide
          Earwin Burrfoot added a comment -

          As the Filter is just a deprecated wrapper, that is removed in 3.0, I think reusing SegmentReader.Ref for that is ok.

          Ok. Maybe you are right.

          Closeable is a Java 1.5 interface only, so this refactoring must wait until 3.0, but the idea is good!

          We can introduce our own Closeable, and replace it with java native in 3.0, thank gods the interface is simple

          Show
          Earwin Burrfoot added a comment - As the Filter is just a deprecated wrapper, that is removed in 3.0, I think reusing SegmentReader.Ref for that is ok. Ok. Maybe you are right. Closeable is a Java 1.5 interface only, so this refactoring must wait until 3.0, but the idea is good! We can introduce our own Closeable, and replace it with java native in 3.0, thank gods the interface is simple
          Hide
          Michael McCandless added a comment -

          Mike will you do this, or should I assign myself to this issue?

          Go ahead & assign to yourself!

          Show
          Michael McCandless added a comment - Mike will you do this, or should I assign myself to this issue? Go ahead & assign to yourself!
          Hide
          Uwe Schindler added a comment - - edited

          Hi Earwin,
          attached is a patch, that simply reuses SegmentReader.Ref. Factoring it out to o.a.l.util would be harder to do at the moment (some test-cases rely on this class). And SegmentReader seems to be the only class, that uses such a Ref construct. Other classes have their refCounter as Field.
          As the Filter is just a deprecated wrapper, that is removed in 3.0, I think reusing SegmentReader.Ref for that is ok.

          This patch also contains a new test for clone(), that does the same like the reopen test (checking the refcounts).

          Closeable is a Java 1.5 interface only, so this refactoring must wait until 3.0, but the idea is good!

          Show
          Uwe Schindler added a comment - - edited Hi Earwin, attached is a patch, that simply reuses SegmentReader.Ref. Factoring it out to o.a.l.util would be harder to do at the moment (some test-cases rely on this class). And SegmentReader seems to be the only class, that uses such a Ref construct. Other classes have their refCounter as Field. As the Filter is just a deprecated wrapper, that is removed in 3.0, I think reusing SegmentReader.Ref for that is ok. This patch also contains a new test for clone(), that does the same like the reopen test (checking the refcounts). Closeable is a Java 1.5 interface only, so this refactoring must wait until 3.0, but the idea is good!
          Hide
          Earwin Burrfoot added a comment -

          Two suggestions:

          Factor out RefCount class and use it everywhere through Lucene. I see at least one identical to yours in SegmentReader. Would be easier to replace all these uses with AtomicInteger later.

          Looking at the new unsightly loop in doClose(), what if we change all Lucene closeable classes to implement java.io.Closeable and create a static utility method(-s) that receives a bunch of Closeables (an array, iterable, vararg in 1.5) and tries to close them all?
          The method should be nullsafe (so you can skip != null checks) and will handle/rethrow exceptions. The most proper way to handle exceptions is probably this - rethrow original exception if it is the only one (be it Runtime or IO), if there's more - gather all exceptions and wrap them into a special IOException subclass that concatenates their messages and keeps them around, so they are inspectable at debug-time or if you implement special treatment for that exception in your code.
          This method can be reused in a heap of places later, SR.doClose() comes first to mind.

          I can do the latter one in a separate patch to close this issue faster.

          Show
          Earwin Burrfoot added a comment - Two suggestions: Factor out RefCount class and use it everywhere through Lucene. I see at least one identical to yours in SegmentReader. Would be easier to replace all these uses with AtomicInteger later. Looking at the new unsightly loop in doClose(), what if we change all Lucene closeable classes to implement java.io.Closeable and create a static utility method(-s) that receives a bunch of Closeables (an array, iterable, vararg in 1.5) and tries to close them all? The method should be nullsafe (so you can skip != null checks) and will handle/rethrow exceptions. The most proper way to handle exceptions is probably this - rethrow original exception if it is the only one (be it Runtime or IO), if there's more - gather all exceptions and wrap them into a special IOException subclass that concatenates their messages and keeps them around, so they are inspectable at debug-time or if you implement special treatment for that exception in your code. This method can be reused in a heap of places later, SR.doClose() comes first to mind. I can do the latter one in a separate patch to close this issue faster.
          Hide
          Uwe Schindler added a comment - - edited

          Do we need to backport to 2.4.2? It's not so easy, because DirectoryReader does not yet exist there (so you cannot simply apply the patch)...
          Without backporting, clone() would not work correctly with shared FSDir IndexReaders.

          And: Backporting would hurt performance, because 2.4 does not use per-segment search, so each call from the searcher is passed through the FilterIndexReader...

          Show
          Uwe Schindler added a comment - - edited Do we need to backport to 2.4.2? It's not so easy, because DirectoryReader does not yet exist there (so you cannot simply apply the patch)... Without backporting, clone() would not work correctly with shared FSDir IndexReaders. And: Backporting would hurt performance, because 2.4 does not use per-segment search, so each call from the searcher is passed through the FilterIndexReader...
          Hide
          Uwe Schindler added a comment -

          Here is an updated patch:

          • Factored out the FilterIndexReader
          • Rewrite doClose() in both readers to record the first exception, but still try to close cleanup everything.
          • Some more code cleanup, CHANGES.txt

          It is ready to commit. Mike will you do this, or should I assign myself to this issue?

          Show
          Uwe Schindler added a comment - Here is an updated patch: Factored out the FilterIndexReader Rewrite doClose() in both readers to record the first exception, but still try to close cleanup everything. Some more code cleanup, CHANGES.txt It is ready to commit. Mike will you do this, or should I assign myself to this issue?
          Hide
          Michael McCandless added a comment -

          Good question Exceptions in close are tricky...

          In some places we try hard to keep closing stuff, and then throw the
          first exception we hit. Probably that should be our preferred
          approach, in general, for readers? (My guess is most apps simply
          catch & log the IOException, so, we should try hard to close all that
          we can on the first go.)

          In other places (SR, DR) we don't.

          In general you can retry a close – calling close more than once on
          Lucene classes is allowed (or, we aim for that, but likely not all
          classes achieve it).

          For writers (IndexWriter, and DR with pending changes) we try hard not
          to lose state on hitting an exception during close. Meaning, on
          hitting an exception in close, either 1) your state already made it
          into the index, or 2) if you fix the root cause (free up disk space,
          fix permisisions, whatever, which of course hardly any app is going to
          be doing) and retry the close, it would succeed, and your index would
          be fine and your changes are committed.

          Show
          Michael McCandless added a comment - Good question Exceptions in close are tricky... In some places we try hard to keep closing stuff, and then throw the first exception we hit. Probably that should be our preferred approach, in general, for readers? (My guess is most apps simply catch & log the IOException, so, we should try hard to close all that we can on the first go.) In other places (SR, DR) we don't. In general you can retry a close – calling close more than once on Lucene classes is allowed (or, we aim for that, but likely not all classes achieve it). For writers (IndexWriter, and DR with pending changes) we try hard not to lose state on hitting an exception during close. Meaning, on hitting an exception in close, either 1) your state already made it into the index, or 2) if you fix the root cause (free up disk space, fix permisisions, whatever, which of course hardly any app is going to be doing) and retry the close, it would succeed, and your index would be fine and your changes are committed.
          Hide
          Earwin Burrfoot added a comment -

          I think it should (be closed in a finally clause).

          Then there's the next question of the same sort, but probably belonging in a separate issue. If we close a DR and one of SR throws an exception - should we close the others (currently we don't)? What is the right way, in general, of handling IOExceptions on IR close? Can we retry the close? What does this exception mean?

          Show
          Earwin Burrfoot added a comment - I think it should (be closed in a finally clause). Then there's the next question of the same sort, but probably belonging in a separate issue. If we close a DR and one of SR throws an exception - should we close the others (currently we don't)? What is the right way, in general, of handling IOExceptions on IR close? Can we retry the close? What does this exception mean?
          Hide
          Michael McCandless added a comment -

          If DirectoryReader.close() throws an Exception, should the directory still be closed for cleanup? This is implemented exactly that way using a try...finally block in DirectoryOwningIndexReader.doClose().

          I think it should (be closed in a finally clause).

          Should I factor out DirectoryOwningIndexReader (any better name?) into a separate java file to not overload the already very big IndexReader.java?

          That sounds good.

          Show
          Michael McCandless added a comment - If DirectoryReader.close() throws an Exception, should the directory still be closed for cleanup? This is implemented exactly that way using a try...finally block in DirectoryOwningIndexReader.doClose(). I think it should (be closed in a finally clause). Should I factor out DirectoryOwningIndexReader (any better name?) into a separate java file to not overload the already very big IndexReader.java? That sounds good.
          Hide
          Uwe Schindler added a comment -

          Only two discussion points (the first one came up during an IRC chat with Earwin):

          • If DirectoryReader.close() throws an Exception, should the directory still be closed for cleanup? This is implemented exactly that way using a try...finally block in DirectoryOwningIndexReader.doClose(). The old DirectoryReader was different, with all its bugs.
          • Should I factor out DirectoryOwningIndexReader (any better name?) into a separate java file to not overload the already very big IndexReader.java? With 3.0, we could simply remove this class file (because already deprecated) together with String/File open() .
          Show
          Uwe Schindler added a comment - Only two discussion points (the first one came up during an IRC chat with Earwin): If DirectoryReader.close() throws an Exception, should the directory still be closed for cleanup? This is implemented exactly that way using a try...finally block in DirectoryOwningIndexReader.doClose(). The old DirectoryReader was different, with all its bugs. Should I factor out DirectoryOwningIndexReader (any better name?) into a separate java file to not overload the already very big IndexReader.java? With 3.0, we could simply remove this class file (because already deprecated) together with String/File open() .
          Hide
          Michael McCandless added a comment -

          OK patch looks good Uwe! This is much cleaner, and it's great to be able to get the scary closeDirectory logic entirely out of DirectoryReader.

          Show
          Michael McCandless added a comment - OK patch looks good Uwe! This is much cleaner, and it's great to be able to get the scary closeDirectory logic entirely out of DirectoryReader.
          Hide
          Uwe Schindler added a comment -

          Both patches are the same, the second one is just using FSDir.open() instead of FSDir.getDirectory. For committing, I would use getDirectory(), the first patch. FSDir.open() would change behaviour for uses relying on the System property to specify the directory impl.

          Show
          Uwe Schindler added a comment - Both patches are the same, the second one is just using FSDir.open() instead of FSDir.getDirectory. For committing, I would use getDirectory(), the first patch. FSDir.open() would change behaviour for uses relying on the System property to specify the directory impl.
          Hide
          Earwin Burrfoot added a comment -

          Patch looks fine. I read the last one, LUCENE-1453-with-FSDir-open.patch.

          Show
          Earwin Burrfoot added a comment - Patch looks fine. I read the last one, LUCENE-1453 -with-FSDir-open.patch.
          Hide
          Uwe Schindler added a comment -

          I forgot to mention:

          Oh, you mean there is an intermittent failure on the current trunk? (Ie, when using FSDir.getDirectory under the hood).

          Yes, for example, if you clone() and index that owns a directory. In this case the directory was not cloned using the ugly hack currently in trunk (see patches before, which is also removed again by my patch) and was then not correctly refcounted. During reopen(), sometimes DirectoryReader uses clone() to create a new instance, when the readonly flag changed, but the index was not modified. The testcase leads sometimes to exactly this behaviour and fails then.

          My patch now also handles clone correctly, so it fixes all these problems by just factoring out the deprecated Directory management to a FilterIndexReader. DirectoryReader just sees the directory and will never try to close it.

          Show
          Uwe Schindler added a comment - I forgot to mention: Oh, you mean there is an intermittent failure on the current trunk? (Ie, when using FSDir.getDirectory under the hood). Yes, for example, if you clone() and index that owns a directory. In this case the directory was not cloned using the ugly hack currently in trunk (see patches before, which is also removed again by my patch) and was then not correctly refcounted. During reopen(), sometimes DirectoryReader uses clone() to create a new instance, when the readonly flag changed, but the index was not modified. The testcase leads sometimes to exactly this behaviour and fails then. My patch now also handles clone correctly, so it fixes all these problems by just factoring out the deprecated Directory management to a FilterIndexReader. DirectoryReader just sees the directory and will never try to close it.
          Hide
          Uwe Schindler added a comment -

          This is a variant for testing the same with FSDir.open(). As you see, the reopening now also works correctly here and the underlying directory is not closed too often.

          This patch is for demonstration only.

          Show
          Uwe Schindler added a comment - This is a variant for testing the same with FSDir.open(). As you see, the reopening now also works correctly here and the underlying directory is not closed too often. This patch is for demonstration only.
          Hide
          Uwe Schindler added a comment -

          This is the solution using the FilterIndexReader, all tests now pass (with refcounting deprectated dirs as well as FSDir.open-dirs, see next Patch).
          The solution consists of two parts:

          • All closeDirectory stuff is removed from DirectoryIndexReader (even the ugly FSDir cloning) and from ReadOnlyDirectoryIndexReader; the code is now simplier to understand. It is now on the status for 3.0, no deprecated helper stuff anymore in these internal classes. So they can be used in 3.0 without modifications.
          • As the DirectoryIndexReader is not closing the directory anymore, the deprectated IndexReader.open methods taking String/File would not work anymore correctly (because they miss to close the dir on closing). To fix this easily, a deprectated private class extends FIlterIndexReader was added, that wraps around the DirectoryIndexReader, when File/String opens are used. This class keeps a refcounter that is incremented on reopen/clone and decremented on doClose(). The last doClose, closes the directory. In 3.0 this class can be removed easily with all File/String open() methods. I could remove this class from IndexReader.java and put in a separate package-private file, if you like.

          I would like to have this in 2.9, to get rid of these ugly closeDirectory hacks! All tests pass (I retried TestIndexReaderReopen about hundred times and no variant fails anymore). It also works, when replacing the refcounting FSDir.getDirectory by FSDir.open() calls (see next patch).

          Show
          Uwe Schindler added a comment - This is the solution using the FilterIndexReader, all tests now pass (with refcounting deprectated dirs as well as FSDir.open-dirs, see next Patch). The solution consists of two parts: All closeDirectory stuff is removed from DirectoryIndexReader (even the ugly FSDir cloning) and from ReadOnlyDirectoryIndexReader; the code is now simplier to understand. It is now on the status for 3.0, no deprecated helper stuff anymore in these internal classes. So they can be used in 3.0 without modifications. As the DirectoryIndexReader is not closing the directory anymore, the deprectated IndexReader.open methods taking String/File would not work anymore correctly (because they miss to close the dir on closing). To fix this easily, a deprectated private class extends FIlterIndexReader was added, that wraps around the DirectoryIndexReader, when File/String opens are used. This class keeps a refcounter that is incremented on reopen/clone and decremented on doClose(). The last doClose, closes the directory. In 3.0 this class can be removed easily with all File/String open() methods. I could remove this class from IndexReader.java and put in a separate package-private file, if you like. I would like to have this in 2.9, to get rid of these ugly closeDirectory hacks! All tests pass (I retried TestIndexReaderReopen about hundred times and no variant fails anymore). It also works, when replacing the refcounting FSDir.getDirectory by FSDir.open() calls (see next patch).
          Hide
          Michael McCandless added a comment -

          But the refcounting is also deprecated? And, IndexReader will no longer track closeDir in 3.0, since that's only set to true in the deprecated methods?

          Because the error sometimes also occurs with the refcounting directories,

          Oh, you mean there is an intermittent failure on the current trunk? (Ie, when using FSDir.getDirectory under the hood).

          Show
          Michael McCandless added a comment - But the refcounting is also deprecated? And, IndexReader will no longer track closeDir in 3.0, since that's only set to true in the deprecated methods? Because the error sometimes also occurs with the refcounting directories, Oh, you mean there is an intermittent failure on the current trunk? (Ie, when using FSDir.getDirectory under the hood).
          Hide
          Uwe Schindler added a comment -

          Because the error sometimes also occurs with the refcounting directories, but more seldom (because of the refcounting helps to keep the directory open, even when it is closed one time too much).
          And our problem: we want to really remove this ugly closeDir stuff from IndexReaders, the code is sometimes unreadable and its hard to find out whats going on.

          Show
          Uwe Schindler added a comment - Because the error sometimes also occurs with the refcounting directories, but more seldom (because of the refcounting helps to keep the directory open, even when it is closed one time too much). And our problem: we want to really remove this ugly closeDir stuff from IndexReaders, the code is sometimes unreadable and its hard to find out whats going on.
          Hide
          Michael McCandless added a comment -

          Since we've deprecated all methods that are using FSDirectory.getDirectory under-the-hood, why do we even need to fix this? Ie why replace all these with the new FSDir.open, now, when we're just going to remove them in 3.0 anyway?

          Show
          Michael McCandless added a comment - Since we've deprecated all methods that are using FSDirectory.getDirectory under-the-hood, why do we even need to fix this? Ie why replace all these with the new FSDir.open, now, when we're just going to remove them in 3.0 anyway?
          Hide
          Earwin Burrfoot added a comment -

          There are two possibilities to fix this:

          Vote for "leave them open". Yes, it breaches the contract, but the breach is controlled (and thus harmless) and we get rid of some weird code (=possible point of failure) without introducing new.
          There is a way to notice change in DirectoryReader behaviour, but it is too unrealistic:

          IndexReader r = IndexReader.open("/path/to/index");
          .....
          Directory d = r.directory(); // you have to get directory reference as you're not the one who created it
          .....
          r.close();
          .....
          d.doSomething(); // and EXPECT this call to fail with exception
          
          Show
          Earwin Burrfoot added a comment - There are two possibilities to fix this: Vote for "leave them open". Yes, it breaches the contract, but the breach is controlled (and thus harmless) and we get rid of some weird code (=possible point of failure) without introducing new. There is a way to notice change in DirectoryReader behaviour, but it is too unrealistic: IndexReader r = IndexReader.open( "/path/to/index" ); ..... Directory d = r.directory(); // you have to get directory reference as you're not the one who created it ..... r.close(); ..... d.doSomething(); // and EXPECT this call to fail with exception
          Hide
          Uwe Schindler added a comment -

          Attached is the patch, that replaces all occurences of FSDir.openDirectory() by open(). The test "TestIndexReaderReopen" fails almost every time with an AlreadyClosedException.

          Show
          Uwe Schindler added a comment - Attached is the patch, that replaces all occurences of FSDir.openDirectory() by open(). The test "TestIndexReaderReopen" fails almost every time with an AlreadyClosedException.
          Hide
          Uwe Schindler added a comment -

          After some testing, I found out, that this issue is not fixed. The test "TestIndexReaderReopen" fails very often, if all occurences of FSDirectory.getDirectory() are replaced by FSDirectory.open() in IndexReader/IndexWriter and DirectoryReader.
          open() returns non refcounting single-use directorys that can be closed only one time. If readers on top of this (using the now-deprecated File/String IndexReader.open()) are reopened the directories are sometimes closed false.
          I was hoping, LUCENE-1651 fixes this, but this is not so.

          There are two possibilities to fix this:
          For both, the first step is to remove the whole closeDir stuff from all IndexReaders and do not close it from within indexReader.
          Then there are two solutions:

          • As FSDir.open() only returns subclasses of FSDir that have a no-op close() method, no closing is required. So we can just leave them open
          • Another possibility is to wrap all readers opened by IR.open() with File/String param by an (deprecated, package private) FilterIndexReader that handles closing the directory in close() and reopen() in a proper way. This is much simplier than doing it inside the DirectoryReader reopen stuff. The small overhead in passing through FilterIndexReader only affects people using the deprecated File/String open() methods. Users, directly using FSDir & Co. have no slowdown.
          Show
          Uwe Schindler added a comment - After some testing, I found out, that this issue is not fixed. The test "TestIndexReaderReopen" fails very often, if all occurences of FSDirectory.getDirectory() are replaced by FSDirectory.open() in IndexReader/IndexWriter and DirectoryReader. open() returns non refcounting single-use directorys that can be closed only one time. If readers on top of this (using the now-deprecated File/String IndexReader.open()) are reopened the directories are sometimes closed false. I was hoping, LUCENE-1651 fixes this, but this is not so. There are two possibilities to fix this: For both, the first step is to remove the whole closeDir stuff from all IndexReaders and do not close it from within indexReader. Then there are two solutions: As FSDir.open() only returns subclasses of FSDir that have a no-op close() method, no closing is required. So we can just leave them open Another possibility is to wrap all readers opened by IR.open() with File/String param by an (deprecated, package private) FilterIndexReader that handles closing the directory in close() and reopen() in a proper way. This is much simplier than doing it inside the DirectoryReader reopen stuff. The small overhead in passing through FilterIndexReader only affects people using the deprecated File/String open() methods. Users, directly using FSDir & Co. have no slowdown.
          Hide
          Uwe Schindler added a comment - - edited

          It was too late in the night yesterday: I think this is not really related to LUCENE-1658, but this case showed possibly a problem in my code.
          As this issue is only interesting for IndexReaders opened without "Directory" but witch FS path, it was a problem with incomplete removal of the reference counting of FSDirs in an early test.
          If the problem occurs again, I will reopen this issue.

          Show
          Uwe Schindler added a comment - - edited It was too late in the night yesterday: I think this is not really related to LUCENE-1658 , but this case showed possibly a problem in my code. As this issue is only interesting for IndexReaders opened without "Directory" but witch FS path, it was a problem with incomplete removal of the reference counting of FSDirs in an early test. If the problem occurs again, I will reopen this issue.
          Hide
          Uwe Schindler added a comment -

          Is this patch obsolete, if LUCENE-1658 is correctly resolved and the whole caching from FSDir is removed?

          Show
          Uwe Schindler added a comment - Is this patch obsolete, if LUCENE-1658 is correctly resolved and the whole caching from FSDir is removed?
          Hide
          Michael McCandless added a comment -

          Committed revision 745797 on 2.4 branch.

          Show
          Michael McCandless added a comment - Committed revision 745797 on 2.4 branch.
          Hide
          Michael McCandless added a comment -

          Reopening for backport to 2.4.1.

          Show
          Michael McCandless added a comment - Reopening for backport to 2.4.1.
          Hide
          Michael McCandless added a comment -

          I agree – I just committed an entry to CHANGES.txt.

          Show
          Michael McCandless added a comment - I agree – I just committed an entry to CHANGES.txt.
          Hide
          Mark Miller added a comment - - edited

          I think this bug should be called out in changes. Disagree? I've seen 2 to 3 people that have seen it, and it seems we should document that its fixed in this version. I'll add it before the next release if nobody disagrees.

          Show
          Mark Miller added a comment - - edited I think this bug should be called out in changes. Disagree? I've seen 2 to 3 people that have seen it, and it seems we should document that its fixed in this version. I'll add it before the next release if nobody disagrees.
          Hide
          Michael McCandless added a comment -

          Committed revision 718540. Thanks Mark!

          Show
          Michael McCandless added a comment - Committed revision 718540. Thanks Mark!
          Hide
          Michael McCandless added a comment -

          OK thanks for the review guys! I'll commit soon.

          Show
          Michael McCandless added a comment - OK thanks for the review guys! I'll commit soon.
          Hide
          Michael Busch added a comment -

          Patch looks good... I guess this was my bug, so thanks for finding and fixing it Mark and Mike!

          Show
          Michael Busch added a comment - Patch looks good... I guess this was my bug, so thanks for finding and fixing it Mark and Mike!
          Hide
          Mark Miller added a comment -

          Okay, I've banged on it and I can't break it. Thats a brilliant way to do it by the way - not nearly as ugly (and is much shorter and actually works, so thats nice).

          Show
          Mark Miller added a comment - Okay, I've banged on it and I can't break it. Thats a brilliant way to do it by the way - not nearly as ugly (and is much shorter and actually works, so thats nice).
          Hide
          Mark Miller added a comment - - edited

          Hmm...I've still got a test that fails. I'll post it tonight or in the morning after a triple check.

          ...trying to check my shoot from the hip modus operandi for a moment <g>...

          edit

          Ah, was optimizing without changing so getting back the same indexreader. Bad test. Patch holding up good so far...

          Show
          Mark Miller added a comment - - edited Hmm...I've still got a test that fails. I'll post it tonight or in the morning after a triple check. ...trying to check my shoot from the hip modus operandi for a moment <g>... edit Ah, was optimizing without changing so getting back the same indexreader. Bad test. Patch holding up good so far...
          Hide
          Michael McCandless added a comment -

          Attached another iteration...

          I moved the logic into DirectoryIndexReader's reopen method, and fixed the case where an exception his hit during reopen (eg if a writer is committing, that's expected) to not incorrectly close the directory.

          I also strengthened the test cases to verify we are not over-incRef'ing the FSDir.

          Show
          Michael McCandless added a comment - Attached another iteration... I moved the logic into DirectoryIndexReader's reopen method, and fixed the case where an exception his hit during reopen (eg if a writer is committing, that's expected) to not incorrectly close the directory. I also strengthened the test cases to verify we are not over-incRef'ing the FSDir.
          Hide
          Mark Miller added a comment -

          Goes deeper than I would have guessed (not that I know reopen at all). Still have a test failing...

          Show
          Mark Miller added a comment - Goes deeper than I would have guessed (not that I know reopen at all). Still have a test failing...
          Hide
          Mark Miller added a comment -

          Ahh, I see. The newReader variable name should have tipped me off <g> I had it in my mind that there could be more but obviously didn't retain the idea (that test actually only breaks on one of the two originals).

          Show
          Mark Miller added a comment - Ahh, I see. The newReader variable name should have tipped me off <g> I had it in my mind that there could be more but obviously didn't retain the idea (that test actually only breaks on one of the two originals).
          Hide
          Michael McCandless added a comment -

          Hmm actually it's not so simple, because we also need the directory to be cloned if you reopen the segment or get a whole new SegmentReader. Also, the case that LUCENE-1430 fixed (in IndexReader.open) also must be guarded against in reopen. I'll work out a new patch...

          Show
          Michael McCandless added a comment - Hmm actually it's not so simple, because we also need the directory to be cloned if you reopen the segment or get a whole new SegmentReader. Also, the case that LUCENE-1430 fixed (in IndexReader.open) also must be guarded against in reopen. I'll work out a new patch...
          Hide
          Mark Miller added a comment -

          Looks great to me. The only nitpick thing I'd change (its a left over from my first patch) is dropping mabyeCopy in SegmentReader into the else clause. Don't think its necessary, but I want to OCD it down.

          Show
          Mark Miller added a comment - Looks great to me. The only nitpick thing I'd change (its a left over from my first patch) is dropping mabyeCopy in SegmentReader into the else clause. Don't think its necessary, but I want to OCD it down.
          Hide
          Michael McCandless added a comment -

          New patch attached. If this looks OK to you Mark, I'll commit. Thanks!

          Show
          Michael McCandless added a comment - New patch attached. If this looks OK to you Mark, I'll commit. Thanks!
          Hide
          Michael McCandless added a comment -

          Hmm good catch Mark! With LUCENE-1451 we are moving away from sharing instances of FSDir, which will fix this problem in 3.0. But in the meantime I think this approach is good. Though, maybe conditionalize it on whether closeDirectory is true? We should add a comment explaining why are re-getting the FSDir in that case, also explaining that this logic can be removed in 3.0.

          I'll make those changes & commit.

          Show
          Michael McCandless added a comment - Hmm good catch Mark! With LUCENE-1451 we are moving away from sharing instances of FSDir, which will fix this problem in 3.0. But in the meantime I think this approach is good. Though, maybe conditionalize it on whether closeDirectory is true? We should add a comment explaining why are re-getting the FSDir in that case, also explaining that this logic can be removed in 3.0. I'll make those changes & commit.
          Hide
          Mark Miller added a comment -

          So this is a bit of an ugly way to do this. Whats the best way? Some kind of clone support on Directory?

          Show
          Mark Miller added a comment - So this is a bit of an ugly way to do this. Whats the best way? Some kind of clone support on Directory?
          Hide
          Mark Miller added a comment -

          Test exposing the problem and one possible fix for it.

          Show
          Mark Miller added a comment - Test exposing the problem and one possible fix for it.

            People

            • Assignee:
              Uwe Schindler
              Reporter:
              Mark Miller
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development