Lucene - Core
  1. Lucene - Core
  2. LUCENE-5262

StandardDirectoryReader should decRef readers on exception, not close them

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.5.1, 4.6, 6.0
    • Component/s: core/index
    • Labels:
      None
    • Lucene Fields:
      New, Patch Available

      Description

      I've hit this while debugging a test, and was able to reproduce with a simple testcase. StandardDirectoryReader.open (nrt) calls IOUtils.close() on hitting an exception from RLD.getReadOnlyClone. But this is wrong, since if two attempts are made to obtain an NRT reader, and both of them fail, the readers that were closed on the first time are no longer closed, since their "closed" member is true. It should instead decRef() them. I'll upload a testcase and fix shortly.

        Activity

        Hide
        Shai Erera added a comment -

        Patch adds testNRTOpenExceptions to TestIndexWriterReader which simulates the bug. I've fixed StandardDirectoryReader.open to decRef the readers on exceptions as well as simplified the method to not save the prior exception that was hit since since we now only decRef() and we're guaranteed that this decRef won't attempt to close the reader since it's obtained from ReaderAndLiveDocs.

        I think it's ready, but I'll wait for the policemen.

        Show
        Shai Erera added a comment - Patch adds testNRTOpenExceptions to TestIndexWriterReader which simulates the bug. I've fixed StandardDirectoryReader.open to decRef the readers on exceptions as well as simplified the method to not save the prior exception that was hit since since we now only decRef() and we're guaranteed that this decRef won't attempt to close the reader since it's obtained from ReaderAndLiveDocs. I think it's ready, but I'll wait for the policemen.
        Hide
        Michael McCandless added a comment -

        +1, tricky!

        Show
        Michael McCandless added a comment - +1, tricky!
        Hide
        Robert Muir added a comment -

        +1

        Show
        Robert Muir added a comment - +1
        Hide
        ASF subversion and git services added a comment -

        Commit 1530051 from Shai Erera in branch 'dev/trunk'
        [ https://svn.apache.org/r1530051 ]

        LUCENE-5262: StandardDirectoryReader should decRef readers on exception, not close them

        Show
        ASF subversion and git services added a comment - Commit 1530051 from Shai Erera in branch 'dev/trunk' [ https://svn.apache.org/r1530051 ] LUCENE-5262 : StandardDirectoryReader should decRef readers on exception, not close them
        Hide
        Uwe Schindler added a comment -

        Patch is fine. If an Exception happens on the decRef, it should call IOUtils.addSuppressed() (branch_4x) or Throwable.addSuppressed() (trunk).

        Show
        Uwe Schindler added a comment - Patch is fine. If an Exception happens on the decRef, it should call IOUtils.addSuppressed() (branch_4x) or Throwable.addSuppressed() (trunk).
        Hide
        Uwe Schindler added a comment -

        Oh its already committed! Please add the addSuppressed, because we should not swallow exceptions.

        Show
        Uwe Schindler added a comment - Oh its already committed! Please add the addSuppressed, because we should not swallow exceptions.
        Hide
        ASF subversion and git services added a comment -

        Commit 1530063 from Shai Erera in branch 'dev/branches/branch_4x'
        [ https://svn.apache.org/r1530063 ]

        LUCENE-5262: StandardDirectoryReader should decRef readers on exception, not close them

        Show
        ASF subversion and git services added a comment - Commit 1530063 from Shai Erera in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1530063 ] LUCENE-5262 : StandardDirectoryReader should decRef readers on exception, not close them
        Hide
        Uwe Schindler added a comment -

        Shai, it's fine to not fix this now! This supressed Exceptions success=true antipattern is another big problem. I was just feeling bad because you did the opposite Robert fixed for TokenStreams yesterday. But IndexWriter is still full of this antipattern, so we should take the time to fix it

        Show
        Uwe Schindler added a comment - Shai, it's fine to not fix this now! This supressed Exceptions success=true antipattern is another big problem. I was just feeling bad because you did the opposite Robert fixed for TokenStreams yesterday. But IndexWriter is still full of this antipattern, so we should take the time to fix it
        Hide
        Shai Erera added a comment -

        Thanks Uwe. Since it's very unlikely that we'll hit an exception from this call to decRef (since we've just incRef'd it above), I prefer that we leave the code as-is. Maybe one day we should move to try-with-resources in all these places...

        Show
        Shai Erera added a comment - Thanks Uwe. Since it's very unlikely that we'll hit an exception from this call to decRef (since we've just incRef'd it above), I prefer that we leave the code as-is. Maybe one day we should move to try-with-resources in all these places...
        Hide
        ASF subversion and git services added a comment -

        Commit 1530845 from Robert Muir in branch 'dev/branches/lucene_solr_4_5'
        [ https://svn.apache.org/r1530845 ]

        LUCENE-4998, LUCENE-5242, LUCENE-5254, LUCENE-5262, LUCENE-5263, LUCENE-5264: svn merge -c 1522723 -c 1525896 -c 1529136 -c 1529141 -c 1530063 -c 1530416 -c 1530657

        Show
        ASF subversion and git services added a comment - Commit 1530845 from Robert Muir in branch 'dev/branches/lucene_solr_4_5' [ https://svn.apache.org/r1530845 ] LUCENE-4998 , LUCENE-5242 , LUCENE-5254 , LUCENE-5262 , LUCENE-5263 , LUCENE-5264 : svn merge -c 1522723 -c 1525896 -c 1529136 -c 1529141 -c 1530063 -c 1530416 -c 1530657

          People

          • Assignee:
            Shai Erera
            Reporter:
            Shai Erera
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development