Lucene - Core
  1. Lucene - Core
  2. LUCENE-3439

add checks/asserts if you search across a closed reader

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.5, 4.0-ALPHA
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      if you try to search across a closed reader (and/or searcher too),
      there are no checks, not even assertions statements.

      this results in crazy scary stacktraces deep inside places like FSTs/various term dictionary implementations etc.

      In some situations, depending on codec, you wont even get an error (i'm sure its fun when you try to retrieve the stored fields!)

      1. LUCENE-3439.patch
        20 kB
        Michael McCandless
      2. LUCENE-3439_test.patch
        3 kB
        Robert Muir

        Activity

        Uwe Schindler made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Hide
        Uwe Schindler added a comment -

        Bulk close after release of 3.5

        Show
        Uwe Schindler added a comment - Bulk close after release of 3.5
        Michael McCandless made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Fix Version/s 3.5 [ 12317877 ]
        Fix Version/s 4.0 [ 12314025 ]
        Resolution Fixed [ 1 ]
        Michael McCandless made changes -
        Assignee Michael McCandless [ mikemccand ]
        Michael McCandless made changes -
        Attachment LUCENE-3439.patch [ 12495086 ]
        Hide
        Michael McCandless added a comment -

        Patch, adding missing ensureOpens, adding volatile to IW's close/closing bools, and fixing the scary bug where we invoke del policy (and possibly delete file) on a closed writer.

        Show
        Michael McCandless added a comment - Patch, adding missing ensureOpens, adding volatile to IW's close/closing bools, and fixing the scary bug where we invoke del policy (and possibly delete file) on a closed writer.
        Hide
        Michael McCandless added a comment -

        and we shouldn't penalize correct programs

        I agree we should keep the added cost for correct programs to a
        minimum, but there are many good reasons why we should try to throw
        AlreadyClosedException instead of something scary like NPE or
        EOFException, etc.:

        • Better usability – when you make a simple mistake (IW/IR.close in
          the wrong place) you get a clear indication what's wrong, and that
          saves you time/frustration/hair/iterations.
        • Earlier error detection: lower risk that an app will push to
          production without catching that IW/IR.close is wrong.
        • Save Lucene devs time: when users come to the list w/ a cryptic
          exception that after a few back & forths we discover was an errant
          IW/IR.close, that uses up lots of people's time, time that instead
          could be spent towards improving Lucene/Solr instead.
        • Protect Lucene's perceived quality: like it or not, Lucene (not
          the app) will sometimes be blamed when a user hits a cryptic
          exception because of an errant IW/IR.close. Blog posts will go
          up, tweets will get tweeted, emails to list or to other lists will
          go unanswered, people doing future Googling will hit these and
          see Lucene as buggy.
        • We can find our own bugs – in adding some missing ensureOpens to
          IW, here, I discovered a case where DirectoryReader.close calls
          IW.deleteUnusedFiles after IW was closed, and this invokes the
          DelPolicy, which is definitely dangerous.

        Net/net I think it's important that we try, when possible/reasonable,
        to have clear error detection and reporting, making it as easy as we
        can for the user to understand what's wrong. And if that means a
        miniscule extra cost to "correct" users, that's a fair tradeoff.

        While a volatile read on (current) x86 is a no-op, it still imposes a happens-before/after restriction via the memory model and hence prevents optimizations across that volatile read. Volatile reads may not always be a no-op on x86 either (and they already aren't on other CPUs).

        The thing is, we already call ensureOpen only in places where the
        added cost should be vanishingly small compared to what will follow;
        it'll still be vanishingly small as a volatile.

        Actually, IR's ensureOpen checks the refCount (AtomicLong)
        so we are already "effectively" volatile for IR, just not for IW.

        Using a volatile also doesn't buy us much either - it's still best-effort (the close could come after we have checked the volatile, but before we have done the read).

        Right this will always be a best effort check, but if we can do a
        "better" best effort, while still having no real added cost, I think
        we should improve.

        I'm not saying we should add ensureOpen() in hotspots. But I do think
        we should make IW's closed boolean volatile. The added cost will
        still be tiny, since we only call ensureOpen where it shouldn't
        matter, and it may allow us to throw ACE instead of a cryptic
        exception in some cases.

        Show
        Michael McCandless added a comment - and we shouldn't penalize correct programs I agree we should keep the added cost for correct programs to a minimum, but there are many good reasons why we should try to throw AlreadyClosedException instead of something scary like NPE or EOFException, etc.: Better usability – when you make a simple mistake (IW/IR.close in the wrong place) you get a clear indication what's wrong, and that saves you time/frustration/hair/iterations. Earlier error detection: lower risk that an app will push to production without catching that IW/IR.close is wrong. Save Lucene devs time: when users come to the list w/ a cryptic exception that after a few back & forths we discover was an errant IW/IR.close, that uses up lots of people's time, time that instead could be spent towards improving Lucene/Solr instead. Protect Lucene's perceived quality: like it or not, Lucene (not the app) will sometimes be blamed when a user hits a cryptic exception because of an errant IW/IR.close. Blog posts will go up, tweets will get tweeted, emails to list or to other lists will go unanswered, people doing future Googling will hit these and see Lucene as buggy. We can find our own bugs – in adding some missing ensureOpens to IW, here, I discovered a case where DirectoryReader.close calls IW.deleteUnusedFiles after IW was closed, and this invokes the DelPolicy, which is definitely dangerous. Net/net I think it's important that we try, when possible/reasonable, to have clear error detection and reporting, making it as easy as we can for the user to understand what's wrong. And if that means a miniscule extra cost to "correct" users, that's a fair tradeoff. While a volatile read on (current) x86 is a no-op, it still imposes a happens-before/after restriction via the memory model and hence prevents optimizations across that volatile read. Volatile reads may not always be a no-op on x86 either (and they already aren't on other CPUs). The thing is, we already call ensureOpen only in places where the added cost should be vanishingly small compared to what will follow; it'll still be vanishingly small as a volatile. Actually, IR's ensureOpen checks the refCount (AtomicLong) so we are already "effectively" volatile for IR, just not for IW. Using a volatile also doesn't buy us much either - it's still best-effort (the close could come after we have checked the volatile, but before we have done the read). Right this will always be a best effort check, but if we can do a "better" best effort, while still having no real added cost, I think we should improve. I'm not saying we should add ensureOpen() in hotspots. But I do think we should make IW's closed boolean volatile. The added cost will still be tiny, since we only call ensureOpen where it shouldn't matter, and it may allow us to throw ACE instead of a cryptic exception in some cases.
        Hide
        Yonik Seeley added a comment -

        I think the open/closed checks should still be non-volatile.
        While a volatile read on (current) x86 is a no-op, it still imposes a happens-before/after restriction via the memory model and hence prevents optimizations across that volatile read. Volatile reads may not always be a no-op on x86 either (and they already aren't on other CPUs).

        Using a volatile also doesn't buy us much either - it's still best-effort (the close could come after we have checked the volatile, but before we have done the read). We have enough volatile/synchronized reads in other places that a non-volatile check will suffice to catch bugs (and we shouldn't penalize correct programs).

        Show
        Yonik Seeley added a comment - I think the open/closed checks should still be non-volatile. While a volatile read on (current) x86 is a no-op, it still imposes a happens-before/after restriction via the memory model and hence prevents optimizations across that volatile read. Volatile reads may not always be a no-op on x86 either (and they already aren't on other CPUs). Using a volatile also doesn't buy us much either - it's still best-effort (the close could come after we have checked the volatile, but before we have done the read). We have enough volatile/synchronized reads in other places that a non-volatile check will suffice to catch bugs (and we shouldn't penalize correct programs).
        Robert Muir made changes -
        Field Original Value New Value
        Attachment LUCENE-3439_test.patch [ 12494689 ]
        Hide
        Robert Muir added a comment -

        IR/IW already try to do this

        I don't think so? I'll attach a patch with a trivial test basically demonstrating what I was seeing.

        It sometimes passes!!!!!

          public void test() throws Exception {
            TermRangeQuery query = TermRangeQuery.newStringRange("field", "a", "z", true, true);
            searcher.search(query, 5);
            searcher.close();
            reader.close();
            searcher.search(query, 5); // are you serious? 
          }
        
        Show
        Robert Muir added a comment - IR/IW already try to do this I don't think so? I'll attach a patch with a trivial test basically demonstrating what I was seeing. It sometimes passes!!!!! public void test() throws Exception { TermRangeQuery query = TermRangeQuery.newStringRange("field", "a", "z", true, true); searcher.search(query, 5); searcher.close(); reader.close(); searcher.search(query, 5); // are you serious? }
        Hide
        Michael McCandless added a comment -

        I think we should add real checks here, w/ volatile isOpen/isClosed bool, up and down the stack, to those methods where the volatile read cost will be tiny compared to the cost of the method or of the app using the returned result.

        IR/IW already try to do this (hmm: though not volatile for IW) but it sounds like we are missing checks inside the codecs?

        Show
        Michael McCandless added a comment - I think we should add real checks here, w/ volatile isOpen/isClosed bool, up and down the stack, to those methods where the volatile read cost will be tiny compared to the cost of the method or of the app using the returned result. IR/IW already try to do this (hmm: though not volatile for IW) but it sounds like we are missing checks inside the codecs?
        Robert Muir created issue -

          People

          • Assignee:
            Michael McCandless
            Reporter:
            Robert Muir
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development