Lucene - Core
  1. Lucene - Core
  2. LUCENE-2239

Revise NIOFSDirectory and its usage due to NIO limitations on Thread.interrupt

    Details

    • Type: Task Task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.4, 2.4.1, 2.9, 2.9.1, 3.0
    • Fix Version/s: 2.9.4, 3.0.3, 3.1, 4.0-ALPHA
    • Component/s: core/store
    • Labels:
      None
    • Lucene Fields:
      New, Patch Available

      Description

      I created this issue as a spin off from http://mail-archives.apache.org/mod_mbox/lucene-java-dev/201001.mbox/%3Cf18c9dde1001280051w4af2bc50u1cfd55f85e50914f@mail.gmail.com%3E

      We should decide what to do with NIOFSDirectory, if we want to keep it as the default on none-windows platforms and how we want to document this.

      1. LUCENE-2239.patch
        8 kB
        Simon Willnauer
      2. LUCENE-2239.patch
        3 kB
        Simon Willnauer
      3. LUCENE-2239.patch
        5 kB
        Simon Willnauer

        Activity

        Hide
        Simon Willnauer added a comment -

        This patch adds documentation to NIOFSDirectory and provides a testcase triggering the behavior. this might be little out of date now but I thought I add it for completeness

        Show
        Simon Willnauer added a comment - This patch adds documentation to NIOFSDirectory and provides a testcase triggering the behavior. this might be little out of date now but I thought I add it for completeness
        Hide
        Mark Miller added a comment -

        IMO, unless we see more reports where this is causing an issue, we just leave things as they are by default, with javadoc explaining the issue. We can introduce a windows deletion policy that can be used if you need it, but I don't see why it would need to be used as a default. I don't think many Lucene threads are being interrupted out there - we have seen no other reports and its now the non windows default in both Lucene and Solr.

        Show
        Mark Miller added a comment - IMO, unless we see more reports where this is causing an issue, we just leave things as they are by default, with javadoc explaining the issue. We can introduce a windows deletion policy that can be used if you need it, but I don't see why it would need to be used as a default. I don't think many Lucene threads are being interrupted out there - we have seen no other reports and its now the non windows default in both Lucene and Solr.
        Hide
        Martin Blech added a comment -

        It is causing an issue in a JAX-RS application that uses Sun's Jersey reference implementation and is deployed on the Grizzly servlet container. Apparently, Grizzly's ThreadPool implemenation uses Thread.interrupt() extensively.

        Show
        Martin Blech added a comment - It is causing an issue in a JAX-RS application that uses Sun's Jersey reference implementation and is deployed on the Grizzly servlet container. Apparently, Grizzly's ThreadPool implemenation uses Thread.interrupt() extensively.
        Hide
        Robert Muir added a comment -

        By the way, seems like this affects MMapDirectory too.

        ant test-core -Dtestcase=TestIndexWriter -Dtestmethod=testThreadInterruptDeadlock -Dtests.directory=MMapDirectory

            [junit] ------------- Standard Output ---------------
            [junit] FAILED; unexpected exception
            [junit] java.nio.channels.ClosedByInterruptException
            [junit]     at java.nio.channels.spi.AbstractInterruptibleChannel.end(AbstractInterruptibleChannel.java:184)
            [junit]     at sun.nio.ch.FileChannelImpl.map(FileChannelImpl.java:772)
        
        Show
        Robert Muir added a comment - By the way, seems like this affects MMapDirectory too. ant test-core -Dtestcase=TestIndexWriter -Dtestmethod=testThreadInterruptDeadlock -Dtests.directory=MMapDirectory [junit] ------------- Standard Output --------------- [junit] FAILED; unexpected exception [junit] java.nio.channels.ClosedByInterruptException [junit] at java.nio.channels.spi.AbstractInterruptibleChannel.end(AbstractInterruptibleChannel.java:184) [junit] at sun.nio.ch.FileChannelImpl.map(FileChannelImpl.java:772)
        Hide
        Simon Willnauer added a comment -

        By the way, seems like this affects MMapDirectory too.

        All AbstractInterruptibleChannel subclasses are affected unfortunately. We should at least document it in the java docs.

        simon

        Show
        Simon Willnauer added a comment - By the way, seems like this affects MMapDirectory too. All AbstractInterruptibleChannel subclasses are affected unfortunately. We should at least document it in the java docs. simon
        Hide
        Simon Willnauer added a comment -

        here is a new patch that add the essential information to the NIOFSDirectory and MMapDirectory.
        I wonder if we should refer to this issue in the doc, IMO a link is not necessary. I removed the TestCase from the previous patch since it was only to reproduce the problem in isolation.

        Show
        Simon Willnauer added a comment - here is a new patch that add the essential information to the NIOFSDirectory and MMapDirectory. I wonder if we should refer to this issue in the doc, IMO a link is not necessary. I removed the TestCase from the previous patch since it was only to reproduce the problem in isolation.
        Hide
        Robert Muir added a comment -

        Additionally i noticed that several quirks in the implementations (NIOFS, MMap, etc) are mentioned in FSDirectory's javadocs.

        This is something to consider: unless someone really digs thru the documentation, they might think they dont care about
        SimpleFS versus NIOFS and stick with FSDirectory.open.

        Then their application might work on windows (perhaps while being developed) but fail on unix (perhaps in production).
        So it might be nice to put a small mention in the FSDirectory docs too.

        Show
        Robert Muir added a comment - Additionally i noticed that several quirks in the implementations (NIOFS, MMap, etc) are mentioned in FSDirectory's javadocs. This is something to consider: unless someone really digs thru the documentation, they might think they dont care about SimpleFS versus NIOFS and stick with FSDirectory.open. Then their application might work on windows (perhaps while being developed) but fail on unix (perhaps in production). So it might be nice to put a small mention in the FSDirectory docs too.
        Hide
        Simon Willnauer added a comment -

        Good point Robert - instead of duplicating documentation we could recommend users to read the implementation specific documentation before using FSDirector#open().

        something like that:

        Currently this returns

        {@link NIOFSDirectory}

        on non-Windows JREs and

        {@link SimpleFSDirectory}

        on Windows. Since these directory implementation have slightly different behavior and limitations it is recommended to consult the implementation specific documentation for the platform your application is running on.

        simon

        Show
        Simon Willnauer added a comment - Good point Robert - instead of duplicating documentation we could recommend users to read the implementation specific documentation before using FSDirector#open(). something like that: Currently this returns {@link NIOFSDirectory} on non-Windows JREs and {@link SimpleFSDirectory} on Windows. Since these directory implementation have slightly different behavior and limitations it is recommended to consult the implementation specific documentation for the platform your application is running on. simon
        Hide
        Simon Willnauer added a comment -

        This patch adds a heads-up to FSDirectory to make uses aware of limitations for certain platforms.

        Show
        Simon Willnauer added a comment - This patch adds a heads-up to FSDirectory to make uses aware of limitations for certain platforms.
        Hide
        Simon Willnauer added a comment -

        Committed in revision 989785

        I think we should backport too

        Show
        Simon Willnauer added a comment - Committed in revision 989785 I think we should backport too
        Hide
        Simon Willnauer added a comment -

        Backported this to 2.9 (rev. 991551), 3.0 (rev. 991548) and 3.x (rev. 989785)

        Show
        Simon Willnauer added a comment - Backported this to 2.9 (rev. 991551), 3.0 (rev. 991548) and 3.x (rev. 989785)

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development