Lucene - Core
  1. Lucene - Core
  2. LUCENE-3485

LuceneTaxonomyReader .decRef() may close the inner IR, renderring the LTR in a limbo.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 3.4
    • Fix Version/s: 3.5, 4.0-ALPHA
    • Component/s: modules/facet
    • Labels:
      None
    • Lucene Fields:
      New, Patch Available

      Description

      TaxonomyReader which supports ref-counting, has a decRef() method which delegates to an inner IndexReader and calls its .decRef(). The latter may close the reader (if the ref is zeroes) but the taxonomy would remain 'open' which will fail many of its method calls.

      Also, the LTR's .close() method does not work in the same manner as IndexReader's - which calls decRef(), and leaves the real closing logic to the decRef(). I believe this should be the right approach for the fix.

      1. LUCENE-3485.patch
        12 kB
        Gilad Barkai
      2. LUCENE-3485.patch
        8 kB
        Gilad Barkai

        Activity

        Hide
        Shai Erera added a comment -

        Good catch Gilad ! Do you intend to prepare a patch?

        Also, we probably shouldn't close the directory in close(), since it was given to us from the outside. So it's whoever created the directory responsibility to close it. We should however eliminate the two deprecated ctors which take a File – no need to have them around anymore, and we're not bound to back-compat in this module, yet .

        Show
        Shai Erera added a comment - Good catch Gilad ! Do you intend to prepare a patch? Also, we probably shouldn't close the directory in close(), since it was given to us from the outside. So it's whoever created the directory responsibility to close it. We should however eliminate the two deprecated ctors which take a File – no need to have them around anymore, and we're not bound to back-compat in this module, yet .
        Hide
        Gilad Barkai added a comment - - edited

        Thanks for the comment Shai.

        The only Directory that is closed is the one that is NOT given - but rather wrapped around a given File.
        I agree that this constructor should be removed, I did not take care of it just yet...

        Attached a patch - Moving closer to IndexReader's ref counting paradigm, also introducing ensureClose().

        Show
        Gilad Barkai added a comment - - edited Thanks for the comment Shai. The only Directory that is closed is the one that is NOT given - but rather wrapped around a given File. I agree that this constructor should be removed, I did not take care of it just yet... Attached a patch - Moving closer to IndexReader's ref counting paradigm, also introducing ensureClose().
        Hide
        Gilad Barkai added a comment -

        Removed deprecated constructors, no private directory to close now.

        Also fixed a small bug from the previous patch.

        Show
        Gilad Barkai added a comment - Removed deprecated constructors, no private directory to close now. Also fixed a small bug from the previous patch.
        Hide
        Shai Erera added a comment -

        Patch looks good ! Few minor comments:

        • I think that it's better to set closed=true in the end of doClose(), after everything succeeds.
        • In doClose, perhaps nullify the two caches? this can come instead of, or in addition to, clear()-ing them
        • doClose is package-private: is that on purpose, or did you mean to make it protected/private? Seems to me like it should be private, because you don't want to call it more than once.
        Show
        Shai Erera added a comment - Patch looks good ! Few minor comments: I think that it's better to set closed=true in the end of doClose(), after everything succeeds. In doClose, perhaps nullify the two caches? this can come instead of, or in addition to, clear()-ing them doClose is package-private: is that on purpose, or did you mean to make it protected/private? Seems to me like it should be private, because you don't want to call it more than once.
        Hide
        Shai Erera added a comment -

        I had another look at the patch, and something bothers me about ensureOpen() and how close is set to true. Currently, if I call close(), and another thread has an instance of TR, any operations he'll try to do will fail by ensureOpen().

        I think that we should remove 'close' and check on IR.refCount(). When it's <= 0, we're closed for business, otherwise the TR should not be marked close().

        I looked at IndexReader and two things:

        1. Its ensureOpen checks refCount()
        2. Its close looks entirely redundant ...
        Show
        Shai Erera added a comment - I had another look at the patch, and something bothers me about ensureOpen() and how close is set to true. Currently, if I call close(), and another thread has an instance of TR, any operations he'll try to do will fail by ensureOpen(). I think that we should remove 'close' and check on IR.refCount(). When it's <= 0, we're closed for business, otherwise the TR should not be marked close(). I looked at IndexReader and two things: Its ensureOpen checks refCount() Its close looks entirely redundant ...
        Hide
        Gilad Barkai added a comment -

        Thanks Shai, you are right, 'closed' should only be used inside close().
        About your previous comment on nullifying the caches, those are 'final' at the moment and i'm not sure it worthwhile unfinaling them just to be nullified - IIRC there are optimizations that may benefit from final variables, and those caches could be hotspots.

        I'll modify the ensureOpen and post a new patch.

        Show
        Gilad Barkai added a comment - Thanks Shai, you are right, 'closed' should only be used inside close(). About your previous comment on nullifying the caches, those are 'final' at the moment and i'm not sure it worthwhile unfinaling them just to be nullified - IIRC there are optimizations that may benefit from final variables, and those caches could be hotspots. I'll modify the ensureOpen and post a new patch.
        Hide
        Shai Erera added a comment -

        I'm fine with not null-ing the caches then. It's not a big deal.

        About close(), we should keep closed so that calling close() multiple times has no side-effects, but we should change ensureReopen().

        Show
        Shai Erera added a comment - I'm fine with not null-ing the caches then. It's not a big deal. About close(), we should keep closed so that calling close() multiple times has no side-effects, but we should change ensureReopen().
        Hide
        Gilad Barkai added a comment -

        New patch as per Shai's comments.

        Show
        Gilad Barkai added a comment - New patch as per Shai's comments.
        Hide
        Shai Erera added a comment -

        I removed some copy-paste errors from the test you added, and moved 'closed=true' to after indexReader.close().

        Committed revision 1179183 (3x).
        Committed revision 1179194 (trunk).

        Thanks Gilad !

        Show
        Shai Erera added a comment - I removed some copy-paste errors from the test you added, and moved 'closed=true' to after indexReader.close(). Committed revision 1179183 (3x). Committed revision 1179194 (trunk). Thanks Gilad !
        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

          People

          • Assignee:
            Shai Erera
            Reporter:
            Gilad Barkai
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development