Lucene - Core
  1. Lucene - Core
  2. LUCENE-3476

SearcherManager misses to close IR if manager is closed during reopen

    Details

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

      Description

      if we close SM while there is a thread calling maybReopen() and swapSearcher throws already closed exception we miss to close the searcher / reader.

      1. LUCENE-3476.patch
        6 kB
        Simon Willnauer
      2. LUCENE-3476.patch
        4 kB
        Simon Willnauer

        Activity

        Simon Willnauer created issue -
        Hide
        Simon Willnauer added a comment -

        here is a fix.

        Show
        Simon Willnauer added a comment - here is a fix.
        Simon Willnauer made changes -
        Field Original Value New Value
        Attachment LUCENE-3476.patch [ 12497011 ]
        Hide
        Michael McCandless added a comment -

        Nice! You removed swapSearcher's sync too. Thanks

        Show
        Michael McCandless added a comment - Nice! You removed swapSearcher's sync too. Thanks
        Simon Willnauer made changes -
        Assignee Simon Willnauer [ simonw ]
        Hide
        Michael McCandless added a comment -

        OK, mulling on this some more.... I don't think we should go to such
        great lengths to remove the sync'd from swapSearcher.

        That sync is harmless in practice (it's uncontended unless the app
        screws up and calls .close while another thread is calling
        .maybeReopen), and the changes necessary to make it truly lockless
        makes SearcherManager's code quite a bit more complex.

        Don't get me wrong: it's an impressive feat of concurrent programming
        to make swapSearcher lock-free, but just because you can remove a sync
        doesn't mean you should.

        But we should still fix the original issue, for those apps that do
        mess up: can't we simply move the swapSearcher inside the
        try/finally/success?

        Show
        Michael McCandless added a comment - OK, mulling on this some more.... I don't think we should go to such great lengths to remove the sync'd from swapSearcher. That sync is harmless in practice (it's uncontended unless the app screws up and calls .close while another thread is calling .maybeReopen), and the changes necessary to make it truly lockless makes SearcherManager's code quite a bit more complex. Don't get me wrong: it's an impressive feat of concurrent programming to make swapSearcher lock-free, but just because you can remove a sync doesn't mean you should. But we should still fix the original issue, for those apps that do mess up: can't we simply move the swapSearcher inside the try/finally/success?
        Hide
        Simon Willnauer added a comment -

        there is no need to make this non-blocking though I agree from a perf perspective. I personally don't like this unchecked exception in swapSearcher but since its internal that's ok. I had to make close synced too to make sure we can call close more than once (this is what the interface says in its jdocs).

        Show
        Simon Willnauer added a comment - there is no need to make this non-blocking though I agree from a perf perspective. I personally don't like this unchecked exception in swapSearcher but since its internal that's ok. I had to make close synced too to make sure we can call close more than once (this is what the interface says in its jdocs).
        Simon Willnauer made changes -
        Attachment LUCENE-3476.patch [ 12497209 ]
        Hide
        Simon Willnauer added a comment -

        committed to trunk in rev. 1177940
        backported to 3.x in rev. 1177943

        Show
        Simon Willnauer added a comment - committed to trunk in rev. 1177940 backported to 3.x in rev. 1177943
        Simon Willnauer made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Hide
        Michael McCandless added a comment -

        Thanks Simon!

        Show
        Michael McCandless added a comment - Thanks Simon!
        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
        Uwe Schindler made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development