Lucene - Core
  1. Lucene - Core
  2. LUCENE-5553

IndexReader#ReaderClosedListener is not always called on IndexReader#close()

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 4.7, 6.0
    • Fix Version/s: 4.7.1, 4.8, 6.0
    • Component/s: core/index
    • Labels:
      None
    • Lucene Fields:
      New, Patch Available

      Description

      Today IndexReader#ReaderClosedListener might not be called if the last IndexReader#decRef() call runs into an exception on IndexReader#doClose(). Today we just reset the refCount and never go and call the listeners. There seem to be a bunch of problems here along the same lines but IMO if we close a reader it should close all resources no matter what exception it runs into. What this should do is call the close listeners in a finally block and then rethrow the exception. The real problem here for apps relying on the listener to release resources is that you might leak memory or file handles or whatnot which I think is a bug how we handle closing the IR. As a side-note I think we should never reset the reference here to be honest.

      Along the same lines I think we need to fix the loop in IndexReader#notifyReaderClosedListeners() to make sure we call all of them in the case any of them throws an exception. It also seems that SegmentCoreReaders#decRef() has a similar problem where for instance a fieldsReader can throw an exception on close and we never call the core listeners.

      IMO we need to fix this for 4.7.1

      1. LUCENE-5553.patch
        13 kB
        Simon Willnauer
      2. LUCENE-5553.patch
        12 kB
        Simon Willnauer
      3. LUCENE-5553.patch
        11 kB
        Simon Willnauer

        Activity

        Hide
        Robert Muir added a comment -

        This stuff essentially needs the pattern of IOUtils.close, close everything, deliver the first exception.

        Show
        Robert Muir added a comment - This stuff essentially needs the pattern of IOUtils.close, close everything, deliver the first exception.
        Hide
        Shai Erera added a comment -

        if the last IndexReader#decRef() call runs into an exception on IndexReader#doClose(). Today we just reset the refCount and never go and call the listeners.

        As I read the code, if doClose() hits an exception, we put the reference back, then the exception is thrown further and therefore closed isn't set to true. So the reader is in fact not closed, and therefore you can attempt to r.close() is again? And also that's probably why we don't notify the listeners?

        I think we need to fix the loop in IndexReader#notifyReaderClosedListeners() to make sure we call all of them

        +1!

        It also seems that SegmentCoreReaders#decRef() has a similar problem where for instance a fieldsReader can throw an exception on close and we never call the core listeners.

        Hmm ... this is trickier. Here, unlike IndexReader.close(), we don't put the reference back. So if that's a bug, we should put back the reference if any of the IOUtils.close() failed. But if that's ok, then +1 to notify the listeners irregardless if close hit an exception.

        Show
        Shai Erera added a comment - if the last IndexReader#decRef() call runs into an exception on IndexReader#doClose(). Today we just reset the refCount and never go and call the listeners. As I read the code, if doClose() hits an exception, we put the reference back, then the exception is thrown further and therefore closed isn't set to true. So the reader is in fact not closed, and therefore you can attempt to r.close() is again? And also that's probably why we don't notify the listeners? I think we need to fix the loop in IndexReader#notifyReaderClosedListeners() to make sure we call all of them +1! It also seems that SegmentCoreReaders#decRef() has a similar problem where for instance a fieldsReader can throw an exception on close and we never call the core listeners. Hmm ... this is trickier. Here, unlike IndexReader.close(), we don't put the reference back. So if that's a bug, we should put back the reference if any of the IOUtils.close() failed. But if that's ok, then +1 to notify the listeners irregardless if close hit an exception.
        Hide
        Shai Erera added a comment -

        As a side-note I think we should never reset the reference here to be honest.

        Didn't see that – if you say that close() should render the IR closed, no matter what exception it hit, then you're right – we should fix the code to notify the listeners but also mark closed=true. And I think that's not a bad idea!

        E.g., I looked at SegmentReader.doClose() and I think it too isn't exception-safe. If any of the CloseableThreadLocals throw an exception (very unlikely), we fail to decRef SegDocValues.

        But what's worse is the whole chain – IR.close() -> SR.doClose() – if an exception is thrown from SegDocValues.decRef(), it happens after core.decRef() succeeded. IR.close() hits the exception and doesn't mark itself as closed. So it doesn't notify the listeners cause it thinks it isn't closed, but in fact I think if you try to use it you will hit an exception .. it's in a weird state?

        Show
        Shai Erera added a comment - As a side-note I think we should never reset the reference here to be honest. Didn't see that – if you say that close() should render the IR closed, no matter what exception it hit, then you're right – we should fix the code to notify the listeners but also mark closed=true . And I think that's not a bad idea! E.g., I looked at SegmentReader.doClose() and I think it too isn't exception-safe. If any of the CloseableThreadLocals throw an exception (very unlikely), we fail to decRef SegDocValues. But what's worse is the whole chain – IR.close() -> SR.doClose() – if an exception is thrown from SegDocValues.decRef(), it happens after core.decRef() succeeded. IR.close() hits the exception and doesn't mark itself as closed. So it doesn't notify the listeners cause it thinks it isn't closed, but in fact I think if you try to use it you will hit an exception .. it's in a weird state?
        Hide
        Simon Willnauer added a comment -

        here is a patch that adds a tests for the index reader including faulty listeners etc. I tried to fix the entire chain down to SCR and I dropped putting the reference back since this should really just close everything and hand the exception back. Shai is right you can call it again but this might not help since in some cases the exception is thrown again and that means it leaves you with a half broken reader. It also feels completly odd to call decRef more than once and a close call should close and shouldn't leave a broken reader behind.

        Show
        Simon Willnauer added a comment - here is a patch that adds a tests for the index reader including faulty listeners etc. I tried to fix the entire chain down to SCR and I dropped putting the reference back since this should really just close everything and hand the exception back. Shai is right you can call it again but this might not help since in some cases the exception is thrown again and that means it leaves you with a half broken reader. It also feels completly odd to call decRef more than once and a close call should close and shouldn't leave a broken reader behind.
        Hide
        Simon Willnauer added a comment -

        next iteration. I cleaned up things a bit and got rid of the static helpers in IR. I think it's ready

        Show
        Simon Willnauer added a comment - next iteration. I cleaned up things a bit and got rid of the static helpers in IR. I think it's ready
        Hide
        Shai Erera added a comment -

        Looks good. I'd maybe rename reThrowSilent to reThrowUnchecked cause it's not silent - it sure will hit someone very loudly! Otherwise, +1!

        Show
        Shai Erera added a comment - Looks good. I'd maybe rename reThrowSilent to reThrowUnchecked cause it's not silent - it sure will hit someone very loudly! Otherwise, +1!
        Hide
        Simon Willnauer added a comment - - edited

        next iter addressing shais comments and adds a changes entry.

        Show
        Simon Willnauer added a comment - - edited next iter addressing shais comments and adds a changes entry.
        Hide
        Michael McCandless added a comment -

        +1 for the patch and to change to "close really closes and throws first exc" behavior.

        Show
        Michael McCandless added a comment - +1 for the patch and to change to "close really closes and throws first exc" behavior.
        Hide
        Robert Muir added a comment -

        +1

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

        Commit 1581400 from Simon Willnauer in branch 'dev/trunk'
        [ https://svn.apache.org/r1581400 ]

        LUCENE-5553: IndexReader#ReaderClosedListener is not always called on IndexReader#close()

        Show
        ASF subversion and git services added a comment - Commit 1581400 from Simon Willnauer in branch 'dev/trunk' [ https://svn.apache.org/r1581400 ] LUCENE-5553 : IndexReader#ReaderClosedListener is not always called on IndexReader#close()
        Hide
        ASF subversion and git services added a comment -

        Commit 1581404 from Simon Willnauer in branch 'dev/branches/branch_4x'
        [ https://svn.apache.org/r1581404 ]

        LUCENE-5553: IndexReader#ReaderClosedListener is not always called on IndexReader#close()

        Show
        ASF subversion and git services added a comment - Commit 1581404 from Simon Willnauer in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1581404 ] LUCENE-5553 : IndexReader#ReaderClosedListener is not always called on IndexReader#close()
        Hide
        ASF subversion and git services added a comment -

        Commit 1581408 from Simon Willnauer in branch 'dev/branches/lucene_solr_4_7'
        [ https://svn.apache.org/r1581408 ]

        LUCENE-5553: IndexReader#ReaderClosedListener is not always called on IndexReader#close()

        Show
        ASF subversion and git services added a comment - Commit 1581408 from Simon Willnauer in branch 'dev/branches/lucene_solr_4_7' [ https://svn.apache.org/r1581408 ] LUCENE-5553 : IndexReader#ReaderClosedListener is not always called on IndexReader#close()
        Hide
        Uwe Schindler added a comment -

        Thanks, cool.
        I knew about this problem, but I was not aware that it is so serious.
        Thanks for adding suppressed Exceptions using Throwable#addSupressed().

        Show
        Uwe Schindler added a comment - Thanks, cool. I knew about this problem, but I was not aware that it is so serious. Thanks for adding suppressed Exceptions using Throwable#addSupressed().
        Hide
        Steve Rowe added a comment -

        Bulk close 4.7.1 issues

        Show
        Steve Rowe added a comment - Bulk close 4.7.1 issues

          People

          • Assignee:
            Simon Willnauer
            Reporter:
            Simon Willnauer
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development