Uploaded image for project: 'Lucene - Core'
  1. Lucene - Core
  2. LUCENE-3439

add checks/asserts if you search across a closed reader

Details

    • Bug
    • Status: Closed
    • Major
    • Resolution: Fixed
    • None
    • 3.5, 4.0-ALPHA
    • None
    • None
    • 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!)

      Attachments

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

        Activity

          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?

          mikemccand 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?
          rcmuir 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? 
            }
          
          rcmuir 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? }
          yseeley@gmail.com 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).

          yseeley@gmail.com 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).

          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.

          mikemccand 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.

          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.

          mikemccand 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.
          uschindler Uwe Schindler added a comment -

          Bulk close after release of 3.5

          uschindler Uwe Schindler added a comment - Bulk close after release of 3.5
          tomoko Tomoko Uchida added a comment -

          This issue was moved to GitHub issue: #4513.

          tomoko Tomoko Uchida added a comment - This issue was moved to GitHub issue: #4513 .

          People

            mikemccand Michael McCandless
            rcmuir Robert Muir
            Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: