Lucene - Core
  1. Lucene - Core
  2. LUCENE-6989

Implement MMapDirectory unmapping for coming Java 9 changes

    Details

    • Type: Task Task
    • Status: In Progress
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: 6.0, 6.4
    • Component/s: core/store
    • Labels:

      Description

      Originally, the sun.misc.Cleaner interface was declared as "critical API" in JEP 260

      Unfortunately the decission was changed in favor of a oficially supported java.lang.ref.Cleaner API. A side effect of this change is to move all existing sun.misc.Cleaner APIs into a non-exported package. This causes our forceful unmapping to no longer work, because we can get the cleaner instance via reflection, but trying to invoke it will throw one of the new Jigsaw RuntimeException because it is completely inaccessible. This will make our forceful unmapping fail. There are also no changes in Garbage collector, the problem still exists.

      For more information see this mailing list thread.

      This commit will likely be done, making our unmapping efforts no longer working. Alan Bateman is aware of this issue and will open a new issue at OpenJDK to allow forceful unmapping without using the now private sun.misc.Cleaner. The idea is to let the internal class sun.misc.Cleaner implement the Runable interface, so we can simply cast to runable and call the run() method to unmap. The code would then work. This will lead to minor changes in our unmapper in MMapDirectory: An instanceof check and casting if possible.

      I opened this issue to keep track and implement the changes as soon as possible, so people will have working unmapping when java 9 comes out. Current Lucene versions will no longer work with Java 9.

      1. LUCENE-6989.patch
        11 kB
        Uwe Schindler
      2. LUCENE-6989.patch
        6 kB
        Uwe Schindler
      3. LUCENE-6989.patch
        6 kB
        Uwe Schindler
      4. LUCENE-6989.patch
        2 kB
        Uwe Schindler
      5. LUCENE-6989-disable5x.patch
        3 kB
        Uwe Schindler
      6. LUCENE-6989-disable5x.patch
        2 kB
        Uwe Schindler
      7. LUCENE-6989-fixbuild148.patch
        1 kB
        Uwe Schindler
      8. LUCENE-6989-v2.patch
        3 kB
        Uwe Schindler
      9. LUCENE-6989-v3-post-b148.patch
        10 kB
        Uwe Schindler
      10. LUCENE-6989-v3-post-b148.patch
        9 kB
        Uwe Schindler
      11. LUCENE-6989-v3-testFixes.patch
        9 kB
        Uwe Schindler

        Issue Links

          Activity

          Hide
          Uwe Schindler added a comment -

          The changes in OpenJDK are about to be committed, see review request. The final solution including a "hack" for us is:

          • The Cleaner instance returned by cleaner() is private to JDK, but implements Runnable
          • We can (if possible cast to Runnable and invoke the method as usual. It does not matter which class it implements, we only see Runnable.
          • The call to run() methods does some extra SecurityManager check, so you must grant an additional permission. This RuntimePermission is: secManager.checkPackageAccess("jdk.internal.ref");. We have to give this permission in tests, otherwise the unmapping fails at runtime (not in static startup check). We might improve the startup check to also catch this. The extra permission check was suggested by me, because otherwise its far "too easy" to call the cleaner, because it implements public interface. Before you needed runtime permission "accessClassInPackage.sun.misc", now you need "accessClassInPackage.jdk.internal.ref".

          I will add this permission and fully test the patch once we have first Java 9 EA builds with this change available. Before doing anything, I want to try it out first. Maybe we can do this at the weekend on FOSDEM together with OpenJDK people. I don't have time to build my own JDK now (and not enough knowledge to do that on windows). Maybe Robert can help me on Linux?

          Show
          Uwe Schindler added a comment - The changes in OpenJDK are about to be committed, see review request . The final solution including a "hack" for us is: The Cleaner instance returned by cleaner() is private to JDK, but implements Runnable We can (if possible cast to Runnable and invoke the method as usual. It does not matter which class it implements, we only see Runnable . The call to run() methods does some extra SecurityManager check, so you must grant an additional permission. This RuntimePermission is: secManager.checkPackageAccess("jdk.internal.ref"); . We have to give this permission in tests, otherwise the unmapping fails at runtime (not in static startup check). We might improve the startup check to also catch this. The extra permission check was suggested by me, because otherwise its far "too easy" to call the cleaner, because it implements public interface. Before you needed runtime permission "accessClassInPackage.sun.misc", now you need "accessClassInPackage.jdk.internal.ref". I will add this permission and fully test the patch once we have first Java 9 EA builds with this change available. Before doing anything, I want to try it out first. Maybe we can do this at the weekend on FOSDEM together with OpenJDK people. I don't have time to build my own JDK now (and not enough knowledge to do that on windows). Maybe Robert can help me on Linux?
          Hide
          Uwe Schindler added a comment -

          Preliminary patch

          Show
          Uwe Schindler added a comment - Preliminary patch
          Hide
          Uwe Schindler added a comment - - edited

          Second version of the patch - a complete rewrite:

          • Fail early by checking everything early!
          • Implement this with MethodHandles (reflection is only needed for the single setAccessible() place: MethodHandles allow to do any access checks at linking time, not runtime),
          • Also do the SecurityManager checks early (for the runable variant)

          This patch implements 3 variants of unmapping:

          • Java 7/8 using sun.misc.Cleaner
          • Java 9 variant 1 using Runnable (public API, we just need to get reference to the cleaner with setAccessible)
          • Java 9 proposed change to remove private cleaner completely and replace by java.lang.ref.Cleaner.Cleanable (public API, we just need to get reference to the cleaner with setAccessible)
          Show
          Uwe Schindler added a comment - - edited Second version of the patch - a complete rewrite: Fail early by checking everything early! Implement this with MethodHandles (reflection is only needed for the single setAccessible() place: MethodHandles allow to do any access checks at linking time, not runtime), Also do the SecurityManager checks early (for the runable variant) This patch implements 3 variants of unmapping: Java 7/8 using sun.misc.Cleaner Java 9 variant 1 using Runnable (public API, we just need to get reference to the cleaner with setAccessible) Java 9 proposed change to remove private cleaner completely and replace by java.lang.ref.Cleaner.Cleanable (public API, we just need to get reference to the cleaner with setAccessible)
          Hide
          Uwe Schindler added a comment -

          Updated patch:

          • Builds a single MethodHandle for unmapping logic
          • Adds missing assume in TestMmapDir
          Show
          Uwe Schindler added a comment - Updated patch: Builds a single MethodHandle for unmapping logic Adds missing assume in TestMmapDir
          Hide
          ASF subversion and git services added a comment -

          Commit 19c56f208521c151befef0c618e60f8ff288c529 in lucene-solr's branch refs/heads/master from Uwe Schindler
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=19c56f2 ]

          LUCENE-6989: Disable test if mmap unmapping is not working (otherwise will fail with later Java 9), until we committed real fix for LUCENE-6989; remove useless and broken test

          Show
          ASF subversion and git services added a comment - Commit 19c56f208521c151befef0c618e60f8ff288c529 in lucene-solr's branch refs/heads/master from Uwe Schindler [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=19c56f2 ] LUCENE-6989 : Disable test if mmap unmapping is not working (otherwise will fail with later Java 9), until we committed real fix for LUCENE-6989 ; remove useless and broken test
          Hide
          ASF subversion and git services added a comment -

          Commit 19c56f208521c151befef0c618e60f8ff288c529 in lucene-solr's branch refs/heads/master from Uwe Schindler
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=19c56f2 ]

          LUCENE-6989: Disable test if mmap unmapping is not working (otherwise will fail with later Java 9), until we committed real fix for LUCENE-6989; remove useless and broken test

          Show
          ASF subversion and git services added a comment - Commit 19c56f208521c151befef0c618e60f8ff288c529 in lucene-solr's branch refs/heads/master from Uwe Schindler [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=19c56f2 ] LUCENE-6989 : Disable test if mmap unmapping is not working (otherwise will fail with later Java 9), until we committed real fix for LUCENE-6989 ; remove useless and broken test
          Hide
          ASF subversion and git services added a comment -

          Commit bb8a1da97f9a5baaa3345bf03481764e014c1787 in lucene-solr's branch refs/heads/branch_5x from Uwe Schindler
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=bb8a1da ]

          LUCENE-6989: Disable test if mmap unmapping is not working (otherwise will fail with later Java 9), until we committed real fix for LUCENE-6989; remove useless and broken test

          Show
          ASF subversion and git services added a comment - Commit bb8a1da97f9a5baaa3345bf03481764e014c1787 in lucene-solr's branch refs/heads/branch_5x from Uwe Schindler [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=bb8a1da ] LUCENE-6989 : Disable test if mmap unmapping is not working (otherwise will fail with later Java 9), until we committed real fix for LUCENE-6989 ; remove useless and broken test
          Hide
          ASF subversion and git services added a comment -

          Commit bb8a1da97f9a5baaa3345bf03481764e014c1787 in lucene-solr's branch refs/heads/branch_5x from Uwe Schindler
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=bb8a1da ]

          LUCENE-6989: Disable test if mmap unmapping is not working (otherwise will fail with later Java 9), until we committed real fix for LUCENE-6989; remove useless and broken test

          Show
          ASF subversion and git services added a comment - Commit bb8a1da97f9a5baaa3345bf03481764e014c1787 in lucene-solr's branch refs/heads/branch_5x from Uwe Schindler [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=bb8a1da ] LUCENE-6989 : Disable test if mmap unmapping is not working (otherwise will fail with later Java 9), until we committed real fix for LUCENE-6989 ; remove useless and broken test
          Hide
          Uwe Schindler added a comment -

          Until this is solved, I committed the fixes for the mmap tests, if unmapping doesnt work (soon in Java 9 builds), so builds won't fail unless this is tested and committed with new JVMs.

          Show
          Uwe Schindler added a comment - Until this is solved, I committed the fixes for the mmap tests, if unmapping doesnt work (soon in Java 9 builds), so builds won't fail unless this is tested and committed with new JVMs.
          Hide
          Uwe Schindler added a comment -

          If we respin 5.5, we should backport this commit, too.

          Show
          Uwe Schindler added a comment - If we respin 5.5, we should backport this commit, too.
          Hide
          ASF subversion and git services added a comment -

          Commit bc7173944fb83fdbb6372800241273226a36faa3 in lucene-solr's branch refs/heads/branch_5_5 from Uwe Schindler
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=bc71739 ]

          LUCENE-6989: Disable test if mmap unmapping is not working (otherwise will fail with later Java 9), until we committed real fix for LUCENE-6989; remove useless and broken test

          Show
          ASF subversion and git services added a comment - Commit bc7173944fb83fdbb6372800241273226a36faa3 in lucene-solr's branch refs/heads/branch_5_5 from Uwe Schindler [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=bc71739 ] LUCENE-6989 : Disable test if mmap unmapping is not working (otherwise will fail with later Java 9), until we committed real fix for LUCENE-6989 ; remove useless and broken test
          Hide
          ASF subversion and git services added a comment -

          Commit bc7173944fb83fdbb6372800241273226a36faa3 in lucene-solr's branch refs/heads/branch_5_5 from Uwe Schindler
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=bc71739 ]

          LUCENE-6989: Disable test if mmap unmapping is not working (otherwise will fail with later Java 9), until we committed real fix for LUCENE-6989; remove useless and broken test

          Show
          ASF subversion and git services added a comment - Commit bc7173944fb83fdbb6372800241273226a36faa3 in lucene-solr's branch refs/heads/branch_5_5 from Uwe Schindler [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=bc71739 ] LUCENE-6989 : Disable test if mmap unmapping is not working (otherwise will fail with later Java 9), until we committed real fix for LUCENE-6989 ; remove useless and broken test
          Hide
          Uwe Schindler added a comment - - edited

          Today, the new build 105 of Java 9 came out.

          I modified the patch a bit to do correct interface casting (to Runnable), because it must be "explicit".

          Otherwise I added a new String constant UNMAP_NOT_SUPPORTED_REASON with the reason message why unmapping is not supported on the platform. If UNMAP_SUPPORTED is false, this will contain the message including instructions to enable it (e.g., which permissions must be given to the lucene-core.jar file). This message is also throws on IllegalArgument why somebody tries to enable unmapping.

          I will commit this now to trunk. I will not backport to 5.x for now, because this relies on Java 8 features.

          If we want to add support for Java 9 in older Lucene versions (Lucene 5), we have to rewrite parts of the code. But I don't think that's needed, because it will take a while until Java 9 comes out and we need more time for testing. People who want to use Java 9 in the future once it is out, should use Lucene 6 or don't use MMapDirectory.

          Show
          Uwe Schindler added a comment - - edited Today, the new build 105 of Java 9 came out. I modified the patch a bit to do correct interface casting (to Runnable), because it must be "explicit". Otherwise I added a new String constant UNMAP_NOT_SUPPORTED_REASON with the reason message why unmapping is not supported on the platform. If UNMAP_SUPPORTED is false, this will contain the message including instructions to enable it (e.g., which permissions must be given to the lucene-core.jar file). This message is also throws on IllegalArgument why somebody tries to enable unmapping. I will commit this now to trunk. I will not backport to 5.x for now, because this relies on Java 8 features. If we want to add support for Java 9 in older Lucene versions (Lucene 5), we have to rewrite parts of the code. But I don't think that's needed, because it will take a while until Java 9 comes out and we need more time for testing. People who want to use Java 9 in the future once it is out, should use Lucene 6 or don't use MMapDirectory.
          Hide
          ASF subversion and git services added a comment -

          Commit 7b6df2542d345ef1815693d43c9e18c6e64726bd in lucene-solr's branch refs/heads/master from Uwe Schindler
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=7b6df25 ]

          LUCENE-6989: Add preliminary support for MMapDirectory unmapping in Java 9

          Show
          ASF subversion and git services added a comment - Commit 7b6df2542d345ef1815693d43c9e18c6e64726bd in lucene-solr's branch refs/heads/master from Uwe Schindler [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=7b6df25 ] LUCENE-6989 : Add preliminary support for MMapDirectory unmapping in Java 9
          Hide
          Uwe Schindler added a comment -

          I keep this issue open until Lucene 6 is released or Java 9 comes out to track further changes.

          Show
          Uwe Schindler added a comment - I keep this issue open until Lucene 6 is released or Java 9 comes out to track further changes.
          Hide
          Uwe Schindler added a comment -

          This patch will be applied to 5.x branch and also for respin of 5.5. It disables unmapping for Java 9 in Lucene 5.x. The new code is to heavy and untested to be released in Lucene 5 branch.

          Show
          Uwe Schindler added a comment - This patch will be applied to 5.x branch and also for respin of 5.5. It disables unmapping for Java 9 in Lucene 5.x. The new code is to heavy and untested to be released in Lucene 5 branch.
          Hide
          ASF subversion and git services added a comment -

          Commit e977d34b64e1b0798fce351357db06e8ba8dcb49 in lucene-solr's branch refs/heads/branch_5x from Uwe Schindler
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=e977d34 ]

          LUCENE-6989: Disable MMapDirectory unmap-hack for Java 9

          Show
          ASF subversion and git services added a comment - Commit e977d34b64e1b0798fce351357db06e8ba8dcb49 in lucene-solr's branch refs/heads/branch_5x from Uwe Schindler [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=e977d34 ] LUCENE-6989 : Disable MMapDirectory unmap-hack for Java 9
          Hide
          ASF subversion and git services added a comment -

          Commit 7a28c454f244ab0cf7af0fa0d0d4b94216171701 in lucene-solr's branch refs/heads/branch_5_5 from Uwe Schindler
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=7a28c45 ]

          LUCENE-6989: Disable MMapDirectory unmap-hack for Java 9

          Show
          ASF subversion and git services added a comment - Commit 7a28c454f244ab0cf7af0fa0d0d4b94216171701 in lucene-solr's branch refs/heads/branch_5_5 from Uwe Schindler [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=7a28c45 ] LUCENE-6989 : Disable MMapDirectory unmap-hack for Java 9
          Hide
          ASF subversion and git services added a comment -

          Commit 7d58232ed2d2a0dcdbb3f827d95c6ce365121aac in lucene-solr's branch refs/heads/master from Uwe Schindler
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=7d58232 ]

          LUCENE-6989: Forward-port changes entry for 5.5

          Show
          ASF subversion and git services added a comment - Commit 7d58232ed2d2a0dcdbb3f827d95c6ce365121aac in lucene-solr's branch refs/heads/master from Uwe Schindler [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=7d58232 ] LUCENE-6989 : Forward-port changes entry for 5.5
          Hide
          ASF subversion and git services added a comment -

          Commit 0e9307bb84696b2091c26cd4df26974cf9a5db1f in lucene-solr's branch refs/heads/master from Uwe Schindler
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=0e9307b ]

          LUCENE-6989: Add Javadocs about required permissions to enable MMAP unmapping

          Show
          ASF subversion and git services added a comment - Commit 0e9307bb84696b2091c26cd4df26974cf9a5db1f in lucene-solr's branch refs/heads/master from Uwe Schindler [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=0e9307b ] LUCENE-6989 : Add Javadocs about required permissions to enable MMAP unmapping
          Hide
          ASF subversion and git services added a comment -

          Commit 4eacebd89ccf8abec0989691fad4a6aa2b408378 in lucene-solr's branch refs/heads/branch_5x from Uwe Schindler
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=4eacebd ]

          LUCENE-6989: fix javadocs bug (Java 8)

          Show
          ASF subversion and git services added a comment - Commit 4eacebd89ccf8abec0989691fad4a6aa2b408378 in lucene-solr's branch refs/heads/branch_5x from Uwe Schindler [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=4eacebd ] LUCENE-6989 : fix javadocs bug (Java 8)
          Hide
          ASF subversion and git services added a comment -

          Commit 8164272e07454ea8ff7a0db348639b6a777311f1 in lucene-solr's branch refs/heads/branch_5_5 from Uwe Schindler
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=8164272 ]

          LUCENE-6989: fix javadocs bug (Java 8)

          Show
          ASF subversion and git services added a comment - Commit 8164272e07454ea8ff7a0db348639b6a777311f1 in lucene-solr's branch refs/heads/branch_5_5 from Uwe Schindler [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=8164272 ] LUCENE-6989 : fix javadocs bug (Java 8)
          Hide
          Robert Muir added a comment -

          It seems wrong that hundreds of tests would fail with 5.x if its unmapping hack is not "up to speed".

          Instead, UNMAP_SUPPORTED should be set to false and we should not require patches that add "if java9" logic and release respins: is there a missing catch block?

          Show
          Robert Muir added a comment - It seems wrong that hundreds of tests would fail with 5.x if its unmapping hack is not "up to speed". Instead, UNMAP_SUPPORTED should be set to false and we should not require patches that add "if java9" logic and release respins: is there a missing catch block?
          Hide
          Uwe Schindler added a comment - - edited

          I discussed that a minute ago with Robert privately.

          There are several problems with the old code. Because the old code solely uses reflection, we cannot detect all problems unless we fully try to unmap something (whoich looks wasteful). We can slightly improve the check, but as it will fail with java 9 anyways, the simpliest fix was to disable unmapping with Java 9 and enable it from Lucene 6+.

          If we want this working correctly in Lucene 5, we have to wait a bit and backport the new code (needs some changes because Objects#nonNull(Object) does not exist in Java 7). The new code is better than the old one, because it no longer uses reflection to call methods. It uses MethodHandles which are early-bound. This allows to do all checks like javac is doing up-front. All methods must exist, all types must be accessible and we also know that calling the method actually works (access). E.g. during reflection, it may happen only at invoke() time that the code finds out that parameters dont match or some security policy prevents calling.

          With MethodHandles, everything is checked up-front:

          • Do all required methods exist?
          • Can we also call all methods without triggering security exceptions?
          • If we cast parameters, does casting work?

          The final MethodHandle void unmapper(ByteBuffer) is compiled like a Java Class / Lambda and was checked by Bytecode Verifier for correctness - it is constructed like the following code:

          void unmapper(ByteBuffer byteBuffer) {
            SomeTypeThatDoesntMatter cleaner = ((java.nio.DirectByteBuffer) byteBuffer).cleaner();
            if (Objects.nonNull(cleaner)) {
              // Java 7/8:
              cleaner.clean();
              // Java 9:
              ((Runnable) cleaner).run();
            } else {
              noop(cleaner); // the noop is needed because MethodHandles#guardWithTest always needs ELSE
            }
          }
          

          See https://github.com/apache/lucene-solr/blob/0e9307bb84696b2091c26cd4df26974cf9a5db1f/lucene/core/src/java/org/apache/lucene/store/MMapDirectory.java#L364-L368 for impl. The noop(SomeTypeThatDoesNotMatter) is needed, because MethodHandles#guardWithTest always requires an "else" clause. John Rose inspired me how to do this (see https://bugs.openjdk.java.net/browse/JDK-8029079): It creates a MethodHandle, returning a "null" constant of type "Void.class", casted to "void.class", and finally dropping the sole (cleaner) argument. So this method returns nothing and just consumes the argument. This handle is used as "else" clause. The boolean check is delegated to Objects#nonNull. If we want to backport we must change this a bit and use Objects.equals instead (+ invert logic and exchange then/else): lookup.findStatic(Objects.class, "equals", methodType(boolean.class, Object.class, Object.class)).bindTo(null) instead current lookup.findStatic(Objects.class, "nonNull", methodType(boolean.class, Object.class)).

          At runtime, no errors can occur anymore (unless the unmapping inside the JVM itsself fails with some unexpcected exception). But no security exceptions or linkage errors can occur anymore.

          Show
          Uwe Schindler added a comment - - edited I discussed that a minute ago with Robert privately. There are several problems with the old code. Because the old code solely uses reflection, we cannot detect all problems unless we fully try to unmap something (whoich looks wasteful). We can slightly improve the check, but as it will fail with java 9 anyways, the simpliest fix was to disable unmapping with Java 9 and enable it from Lucene 6+. If we want this working correctly in Lucene 5, we have to wait a bit and backport the new code (needs some changes because Objects#nonNull(Object) does not exist in Java 7). The new code is better than the old one, because it no longer uses reflection to call methods. It uses MethodHandles which are early-bound. This allows to do all checks like javac is doing up-front. All methods must exist, all types must be accessible and we also know that calling the method actually works (access). E.g. during reflection, it may happen only at invoke() time that the code finds out that parameters dont match or some security policy prevents calling. With MethodHandles, everything is checked up-front: Do all required methods exist? Can we also call all methods without triggering security exceptions? If we cast parameters, does casting work? The final MethodHandle void unmapper(ByteBuffer) is compiled like a Java Class / Lambda and was checked by Bytecode Verifier for correctness - it is constructed like the following code: void unmapper(ByteBuffer byteBuffer) { SomeTypeThatDoesntMatter cleaner = ((java.nio.DirectByteBuffer) byteBuffer).cleaner(); if (Objects.nonNull(cleaner)) { // Java 7/8: cleaner.clean(); // Java 9: (( Runnable ) cleaner).run(); } else { noop(cleaner); // the noop is needed because MethodHandles#guardWithTest always needs ELSE } } See https://github.com/apache/lucene-solr/blob/0e9307bb84696b2091c26cd4df26974cf9a5db1f/lucene/core/src/java/org/apache/lucene/store/MMapDirectory.java#L364-L368 for impl. The noop(SomeTypeThatDoesNotMatter) is needed, because MethodHandles#guardWithTest always requires an "else" clause. John Rose inspired me how to do this (see https://bugs.openjdk.java.net/browse/JDK-8029079 ): It creates a MethodHandle, returning a "null" constant of type "Void.class", casted to "void.class", and finally dropping the sole (cleaner) argument. So this method returns nothing and just consumes the argument. This handle is used as "else" clause. The boolean check is delegated to Objects#nonNull . If we want to backport we must change this a bit and use Objects.equals instead (+ invert logic and exchange then/else): lookup.findStatic(Objects.class, "equals", methodType(boolean.class, Object.class, Object.class)).bindTo(null) instead current lookup.findStatic(Objects.class, "nonNull", methodType(boolean.class, Object.class)) . At runtime, no errors can occur anymore (unless the unmapping inside the JVM itsself fails with some unexpcected exception). But no security exceptions or linkage errors can occur anymore.
          Hide
          Uwe Schindler added a comment -

          Improved patch for 5.x to catch more problems instead of just disabling Java 9.

          The good thing: it will also find (most, but not necessarily all) missing security checks. It will work with Java 9 (+ Java 9 Jigsaw) up to build 104.

          Show
          Uwe Schindler added a comment - Improved patch for 5.x to catch more problems instead of just disabling Java 9. The good thing: it will also find (most, but not necessarily all) missing security checks. It will work with Java 9 (+ Java 9 Jigsaw) up to build 104.
          Hide
          Robert Muir added a comment -

          thanks Uwe, this 5.x patch is a good improvement.

          Show
          Robert Muir added a comment - thanks Uwe, this 5.x patch is a good improvement.
          Hide
          ASF subversion and git services added a comment -

          Commit 6b0672722933b37a61f2ad50ba40df760f9f4c22 in lucene-solr's branch refs/heads/branch_5x from Uwe Schindler
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=6b06727 ]

          LUCENE-6989: Improve MMapDirectory's unmapping checks to catch more non-working cases

          Show
          ASF subversion and git services added a comment - Commit 6b0672722933b37a61f2ad50ba40df760f9f4c22 in lucene-solr's branch refs/heads/branch_5x from Uwe Schindler [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=6b06727 ] LUCENE-6989 : Improve MMapDirectory's unmapping checks to catch more non-working cases
          Hide
          ASF subversion and git services added a comment -

          Commit 2a228b3920a07f930f7afb6a42d0d20e184a943c in lucene-solr's branch refs/heads/branch_5_5 from Uwe Schindler
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=2a228b3 ]

          LUCENE-6989: Improve MMapDirectory's unmapping checks to catch more non-working cases

          Show
          ASF subversion and git services added a comment - Commit 2a228b3920a07f930f7afb6a42d0d20e184a943c in lucene-solr's branch refs/heads/branch_5_5 from Uwe Schindler [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=2a228b3 ] LUCENE-6989 : Improve MMapDirectory's unmapping checks to catch more non-working cases
          Hide
          ASF subversion and git services added a comment -

          Commit 44b58ee4f8829581565bd22cfae01c5e1f864b36 in lucene-solr's branch refs/heads/master from Uwe Schindler
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=44b58ee ]

          LUCENE-6989: Merge changes

          Show
          ASF subversion and git services added a comment - Commit 44b58ee4f8829581565bd22cfae01c5e1f864b36 in lucene-solr's branch refs/heads/master from Uwe Schindler [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=44b58ee ] LUCENE-6989 : Merge changes
          Hide
          Michael McCandless added a comment -

          Uwe Schindler can this be closed now?

          Show
          Michael McCandless added a comment - Uwe Schindler can this be closed now?
          Hide
          Uwe Schindler added a comment - - edited

          No, keep it open, as the final word is not yet spoken. The next changes in Java is underway.

          In addition I have a small change for this commit, which makes code more easier to understand (comes a bit later). I am also debugging issues with build 105 at the moment.

          This issue is about master branch. The fixes in 5.x was just the "backport".

          Show
          Uwe Schindler added a comment - - edited No, keep it open, as the final word is not yet spoken. The next changes in Java is underway. In addition I have a small change for this commit, which makes code more easier to understand (comes a bit later). I am also debugging issues with build 105 at the moment. This issue is about master branch. The fixes in 5.x was just the "backport".
          Hide
          Uwe Schindler added a comment -

          Here is a rewrite of the Java 8 / Lucene 6 code to make it easier to understand (the casting of the Runnable interface).

          This helped me to debug the Java 9 b105 issue we have seen today.

          Show
          Uwe Schindler added a comment - Here is a rewrite of the Java 8 / Lucene 6 code to make it easier to understand (the casting of the Runnable interface). This helped me to debug the Java 9 b105 issue we have seen today.
          Hide
          ASF subversion and git services added a comment -

          Commit 0f29b3ec7fd638341915f83384656e72dff868ec in lucene-solr's branch refs/heads/master from Uwe Schindler
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=0f29b3e ]

          LUCENE-6989: Make casting to Runnable interface in cleaner hack easier to understand

          Show
          ASF subversion and git services added a comment - Commit 0f29b3ec7fd638341915f83384656e72dff868ec in lucene-solr's branch refs/heads/master from Uwe Schindler [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=0f29b3e ] LUCENE-6989 : Make casting to Runnable interface in cleaner hack easier to understand
          Hide
          Uwe Schindler added a comment - - edited

          Hi,
          I was able to reproduce the Hotspot bug and opened an OpenJDK issue about the following problem we have seen with build 105 of Java 9:

          Caused by: java.lang.IncompatibleClassChangeError: Found class jdk.internal.ref.Cleaner, but interface was expected
          	at org.apache.lucene.store.MMapDirectory.lambda$initUnmapHack$0(MMapDirectory.java:374)
          	at java.security.AccessController.doPrivileged(Native Method)
          	at org.apache.lucene.store.MMapDirectory.lambda$initUnmapHack$1(MMapDirectory.java:372)
          

          The OpenJDK issue is: https://bugs.openjdk.java.net/browse/JDK-8150436

          The issue only affects Java 9, it is not a problem with our code or the internal Cleaner. It can be reproduced with a simple Runnable interface and a bit of guardWithTest wrapping.

          Show
          Uwe Schindler added a comment - - edited Hi, I was able to reproduce the Hotspot bug and opened an OpenJDK issue about the following problem we have seen with build 105 of Java 9: Caused by: java.lang.IncompatibleClassChangeError: Found class jdk.internal.ref.Cleaner, but interface was expected at org.apache.lucene.store.MMapDirectory.lambda$initUnmapHack$0(MMapDirectory.java:374) at java.security.AccessController.doPrivileged(Native Method) at org.apache.lucene.store.MMapDirectory.lambda$initUnmapHack$1(MMapDirectory.java:372) The OpenJDK issue is: https://bugs.openjdk.java.net/browse/JDK-8150436 The issue only affects Java 9, it is not a problem with our code or the internal Cleaner. It can be reproduced with a simple Runnable interface and a bit of guardWithTest wrapping.
          Hide
          Uwe Schindler added a comment -

          I added a separate Jenkins job to ASF Jenkins that runs tests with -Dtests.directory=MMapDirectory. Curretly this does not run on Policeman, because it breaks with Java 9 build 105. This job is mainly to stress test the new unmap to find bugs early:

          https://builds.apache.org/job/Lucene-Solr-Tests-trunk-mmap-Java8/

          Show
          Uwe Schindler added a comment - I added a separate Jenkins job to ASF Jenkins that runs tests with -Dtests.directory=MMapDirectory . Curretly this does not run on Policeman, because it breaks with Java 9 build 105. This job is mainly to stress test the new unmap to find bugs early: https://builds.apache.org/job/Lucene-Solr-Tests-trunk-mmap-Java8/
          Hide
          Uwe Schindler added a comment -

          FYI, i downloaded build 106 of Jigsaw: New unmap Hack also works with Jigsaw (the new one using Runnable):

          $ java -version
          java version "9-ea"
          Java(TM) SE Runtime Environment (build 9-ea+106-jigsaw-nightly-h4488-20160219)
          Java HotSpot(TM) 64-Bit Server VM (build 9-ea+106-jigsaw-nightly-h4488-20160219, mixed mode)
          

          It is definitely using the new jdk.internal.ref.Cleaner class. It also only works if you give the new required permissions. Of course it still fails from time to time because of the Hotspot bug (see previous comment).

          Show
          Uwe Schindler added a comment - FYI, i downloaded build 106 of Jigsaw: New unmap Hack also works with Jigsaw (the new one using Runnable): $ java -version java version "9-ea" Java(TM) SE Runtime Environment (build 9-ea+106-jigsaw-nightly-h4488-20160219) Java HotSpot(TM) 64-Bit Server VM (build 9-ea+106-jigsaw-nightly-h4488-20160219, mixed mode) It is definitely using the new jdk.internal.ref.Cleaner class. It also only works if you give the new required permissions. Of course it still fails from time to time because of the Hotspot bug (see previous comment).
          Hide
          Uwe Schindler added a comment -

          I got the issue ID of the interface Hotspot problem: https://bugs.openjdk.java.net/browse/JDK-8150436
          (I updated above comment).

          Show
          Uwe Schindler added a comment - I got the issue ID of the interface Hotspot problem: https://bugs.openjdk.java.net/browse/JDK-8150436 (I updated above comment).
          Hide
          ASF subversion and git services added a comment -

          Commit 5f9db018337c8173201b1b542e8ea7c38feecd00 in lucene-solr's branch refs/heads/master from Uwe Schindler
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=5f9db01 ]

          LUCENE-6989: Also print missing permission if unmap hack does not work; rename method to make stack trace look nice while executing unmapping

          Show
          ASF subversion and git services added a comment - Commit 5f9db018337c8173201b1b542e8ea7c38feecd00 in lucene-solr's branch refs/heads/master from Uwe Schindler [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=5f9db01 ] LUCENE-6989 : Also print missing permission if unmap hack does not work; rename method to make stack trace look nice while executing unmapping
          Hide
          Sanne Grinovero added a comment -

          Hi all, is there an update on this? I see several patches were committed, and the Hotspot issue JDK-8150436 is marked resolved, yet this issue is not.

          I'm particularly interested in the backport to 5.5 (actually ideally to 5.3); if someone could guide me I'll try to help with it.

          Thanks!

          Show
          Sanne Grinovero added a comment - Hi all, is there an update on this? I see several patches were committed, and the Hotspot issue JDK-8150436 is marked resolved, yet this issue is not. I'm particularly interested in the backport to 5.5 (actually ideally to 5.3); if someone could guide me I'll try to help with it. Thanks!
          Hide
          Uwe Schindler added a comment -

          This is all working, but I want to wait until Java 9 is released, because this hack could still break until the release of Java 9 (Jenkins will catch this).

          Backporting to 5.5 should be possible, but I won't do this. Lucene 5.x is not compatible to Java 9 for other reasons, too, because it has no Jigsaw support. So this would not really help.

          Show
          Uwe Schindler added a comment - This is all working, but I want to wait until Java 9 is released, because this hack could still break until the release of Java 9 (Jenkins will catch this). Backporting to 5.5 should be possible, but I won't do this. Lucene 5.x is not compatible to Java 9 for other reasons, too, because it has no Jigsaw support. So this would not really help.
          Hide
          Uwe Schindler added a comment - - edited

          The problem with backporting is usage of Java 8 APIs: To allow backporting to 5.5, we must change the unmapper bytecode, because Objects.nonNull(Object) does not exist in Java 7. We have to work around that with a hack (I had some in mind), I have to find the records.

          UPDATE: The Java 7 workaround is a change to the nonNull check to use Objects.equals(Object,Object) with a bound null reference as 1st arg and later in the code invert the if/then/else logic (swap the 2 last args in MethodHandles.guardWithTest):

          final MethodHandle nullTest = lookup.findStatic(Objects.class, "equals", methodType(boolean.class, Object.class, Object.class))
            .bindTo(null)
            .asType(methodType(boolean.class, cleanerClass));
          

          I did not test / compile this code, was just hacked into this issue

          Show
          Uwe Schindler added a comment - - edited The problem with backporting is usage of Java 8 APIs: To allow backporting to 5.5, we must change the unmapper bytecode, because Objects.nonNull(Object) does not exist in Java 7. We have to work around that with a hack (I had some in mind), I have to find the records. UPDATE: The Java 7 workaround is a change to the nonNull check to use Objects.equals(Object,Object) with a bound null reference as 1st arg and later in the code invert the if/then/else logic (swap the 2 last args in MethodHandles.guardWithTest ): final MethodHandle nullTest = lookup.findStatic(Objects.class, "equals" , methodType( boolean .class, Object .class, Object .class)) .bindTo( null ) .asType(methodType( boolean .class, cleanerClass)); I did not test / compile this code, was just hacked into this issue
          Hide
          Uwe Schindler added a comment -

          In Java 9 build 148, the hack broke again, but unfortunately fatal. There are 2 problems:

          • Java 9 now prevents access to the ByteBuffer. This is the total desaster
          • Lucene has a bug that it fails when testing the availability of unmapping, because setAccessible throws InaccessibleObjectException that is subclass of RuntimeException and not ReflectiveOperationException. We have to fix this, as otherwise loading of the class MMapDircetory fails completely.
          Show
          Uwe Schindler added a comment - In Java 9 build 148, the hack broke again, but unfortunately fatal. There are 2 problems: Java 9 now prevents access to the ByteBuffer. This is the total desaster Lucene has a bug that it fails when testing the availability of unmapping, because setAccessible throws InaccessibleObjectException that is subclass of RuntimeException and not ReflectiveOperationException. We have to fix this, as otherwise loading of the class MMapDircetory fails completely.
          Hide
          Uwe Schindler added a comment -

          Here is a patch for the Lucene issue that prevents loading of class on the crazy new InaccessibleObjectException (extends RuntimeException). We should apply this in any case to all open branches.

          This does not fix the issue in build 148 of Java 9, which makes unmapping impossible. I am working on resolving this with OpenJDK/Oracle.

          Show
          Uwe Schindler added a comment - Here is a patch for the Lucene issue that prevents loading of class on the crazy new InaccessibleObjectException (extends RuntimeException). We should apply this in any case to all open branches. This does not fix the issue in build 148 of Java 9, which makes unmapping impossible. I am working on resolving this with OpenJDK/Oracle.
          Hide
          Uwe Schindler added a comment -

          With this patch the class loads again, although it does not work:

             [junit4] Suite: org.apache.lucene.store.TestMultiMMap
             [junit4] IGNOR/A 0.03s | TestMultiMMap.testDeleteFile
             [junit4]    > Assumption #1: test requires a jre that supports unmapping: Unmapping is not supported on this platform, because internal Java APIs are not compatible to this Lucene version: java.lang.reflect.InaccessibleObjectException: Unable to make public jdk.internal.ref.Cleaner java.nio.DirectByteBuffer.cleaner() accessible: module java.base does not "opens java.nio" to unnamed module @4823fcdd
          
          Show
          Uwe Schindler added a comment - With this patch the class loads again, although it does not work: [junit4] Suite: org.apache.lucene.store.TestMultiMMap [junit4] IGNOR/A 0.03s | TestMultiMMap.testDeleteFile [junit4] > Assumption #1: test requires a jre that supports unmapping: Unmapping is not supported on this platform, because internal Java APIs are not compatible to this Lucene version: java.lang.reflect.InaccessibleObjectException: Unable to make public jdk.internal.ref.Cleaner java.nio.DirectByteBuffer.cleaner() accessible: module java.base does not "opens java.nio" to unnamed module @4823fcdd
          Hide
          ASF subversion and git services added a comment -

          Commit 22d04a7c1149c1af42dc2890a416fc45e4d0aa5e in lucene-solr's branch refs/heads/master from Uwe Schindler
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=22d04a7 ]

          LUCENE-6989: Fix Exception handling in MMapDirectory's unmap hack support code to work with Java 9's new InaccessibleObjectException that does not extend ReflectiveAccessException in Java 9.

          Show
          ASF subversion and git services added a comment - Commit 22d04a7c1149c1af42dc2890a416fc45e4d0aa5e in lucene-solr's branch refs/heads/master from Uwe Schindler [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=22d04a7 ] LUCENE-6989 : Fix Exception handling in MMapDirectory's unmap hack support code to work with Java 9's new InaccessibleObjectException that does not extend ReflectiveAccessException in Java 9.
          Hide
          ASF subversion and git services added a comment -

          Commit d00ab65dccbef7add8b433a338c2ec3cea980916 in lucene-solr's branch refs/heads/branch_6x from Uwe Schindler
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=d00ab65 ]

          LUCENE-6989: Fix Exception handling in MMapDirectory's unmap hack support code to work with Java 9's new InaccessibleObjectException that does not extend ReflectiveAccessException in Java 9.

          Show
          ASF subversion and git services added a comment - Commit d00ab65dccbef7add8b433a338c2ec3cea980916 in lucene-solr's branch refs/heads/branch_6x from Uwe Schindler [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=d00ab65 ] LUCENE-6989 : Fix Exception handling in MMapDirectory's unmap hack support code to work with Java 9's new InaccessibleObjectException that does not extend ReflectiveAccessException in Java 9.
          Hide
          Uwe Schindler added a comment -

          I committed the fix for detection, but the build still fails a bit later. Groovy is also broken, so you cannot run tests.

          Show
          Uwe Schindler added a comment - I committed the fix for detection, but the build still fails a bit later. Groovy is also broken, so you cannot run tests.
          Hide
          Uwe Schindler added a comment - - edited

          For now I reverted back to build 147 of JDK 9 on Policeman Jenkins as the whole build system no longer works. I will check out with JDK guys how to proceed.

          In addition to this mmap unmapping problem, RAMUsageEstimator in the test framework is also broken, and Groovy. Both seem to be affected by the same issue: setAccessible is completely forbidden, without any special cases that are allowed (see https://bugs.openjdk.java.net/browse/JDK-8148117 and https://bugs.openjdk.java.net/browse/JDK-8132928 for more details). I think that's a bug.

          Show
          Uwe Schindler added a comment - - edited For now I reverted back to build 147 of JDK 9 on Policeman Jenkins as the whole build system no longer works. I will check out with JDK guys how to proceed. In addition to this mmap unmapping problem, RAMUsageEstimator in the test framework is also broken, and Groovy. Both seem to be affected by the same issue: setAccessible is completely forbidden, without any special cases that are allowed (see https://bugs.openjdk.java.net/browse/JDK-8148117 and https://bugs.openjdk.java.net/browse/JDK-8132928 for more details). I think that's a bug.
          Show
          Uwe Schindler added a comment - I opened thread: http://mail.openjdk.java.net/pipermail/jigsaw-dev/2016-December/010454.html
          Hide
          Uwe Schindler added a comment -

          The current reply by Alan Bateman was:

          sun.misc.Cleaner was indeed on the original list of APIs for JEP 260 to
          identify as a "critical internal API". It turned out not to be useful
          because it would have required some way to get the Cleaner in the first
          place. That lead to the "new" hack that is reading the private "cleaner"
          field from DBB and treating it as a Runnable. That hack now breaks
          because setAccessible has changed in jdk-9+148 to align with the JSR 376
          proposal tracked as #AwkwardStrongEncapsulation.

          No need to panic though, there is an update to JEP 260 coming soon for
          this specific need. Details TDB but it will probably be a method in
          jdk.unsupported module. It does mean that libraries using the old (or
          "new") hacks will need to change. I hope it will be seen as a reasonable
          compromise for this generally awkward issue.

          I am free to change the hack again...

          Show
          Uwe Schindler added a comment - The current reply by Alan Bateman was: sun.misc.Cleaner was indeed on the original list of APIs for JEP 260 to identify as a "critical internal API". It turned out not to be useful because it would have required some way to get the Cleaner in the first place. That lead to the "new" hack that is reading the private "cleaner" field from DBB and treating it as a Runnable. That hack now breaks because setAccessible has changed in jdk-9+148 to align with the JSR 376 proposal tracked as #AwkwardStrongEncapsulation. No need to panic though, there is an update to JEP 260 coming soon for this specific need. Details TDB but it will probably be a method in jdk.unsupported module. It does mean that libraries using the old (or "new") hacks will need to change. I hope it will be seen as a reasonable compromise for this generally awkward issue. I am free to change the hack again...
          Hide
          Sanne Grinovero added a comment -

          Thanks Uwe for accurately keeping track of all this!

          I'm following it all on the OpenJDK mailing list but it's great to keep track of all details here.

          As you know I'm interested in both Hibernate and Lucene - as I integrate them into Hibernate Search and bundle them into WildFly. WildFly has had strong modularity since years so we have some experience packaging Lucene for it, and for OSGi as well.
          Jason Greene, the WildFly platform architect just blogged about our open issues; you might find it interesting too: http://wildfly.org/news/2016/12/12/Jigsaws-Missing-Pieces/

          Definitely looking forward to integrate a compatible Lucene version, at minimal disruptions for end users, which in our case implies to hopefully be able to backport any needed patch as far as Lucene version 5.3. We expose Lucene APIs directly, so upgrading would break app compatibility which is yet another form of user inconvenience; I guess it might need to come to that, we'll see when Jigsaw plans will become clearer.

          Show
          Sanne Grinovero added a comment - Thanks Uwe for accurately keeping track of all this! I'm following it all on the OpenJDK mailing list but it's great to keep track of all details here. As you know I'm interested in both Hibernate and Lucene - as I integrate them into Hibernate Search and bundle them into WildFly. WildFly has had strong modularity since years so we have some experience packaging Lucene for it, and for OSGi as well. Jason Greene, the WildFly platform architect just blogged about our open issues; you might find it interesting too: http://wildfly.org/news/2016/12/12/Jigsaws-Missing-Pieces/ Definitely looking forward to integrate a compatible Lucene version, at minimal disruptions for end users, which in our case implies to hopefully be able to backport any needed patch as far as Lucene version 5.3. We expose Lucene APIs directly, so upgrading would break app compatibility which is yet another form of user inconvenience; I guess it might need to come to that, we'll see when Jigsaw plans will become clearer.
          Hide
          Uwe Schindler added a comment - - edited

          Here is a first patch for the planned Java 9 unmapping.

          Based on the discussions on the OpenJDK mailing list the following must be noted and implemented.

          • setAccessible() is NO LONGER POSSIBLE on any class from Java's public and documented class library - simple and easy - no excuses or workarounds anymore. There are still a few special cases (like retrieving sun.misc.Unsafe, but thats unsupported API anyways). This is a separate issue and requires, that RAMUsageEstimator in Lucene's tests and RandomizedRunner needs to be disabled (Dawid Weiss). I will open one.
          • The first implementation of the Java 9 support is now obsolete, because not even the Runnable approach works with above restriction.
          • We proposed some "temporary workaround" for unmapping for the time beeing. This workaround sounds reasonable and first patches for OpenJDK are under discussion, but might take some time to be committed. The idea is: As sun.misc.Unsafe has a special role in the JDK (you can still get an instance of it if you respect security manager and use the well-known pattern to get it) and the unmapping of mmapped buffers is "unsafe" per se - why not add a method to unmap ByteBuffers to sun.misc.Unsafe (easy job!). OpenJDK patch / CR is here: http://cr.openjdk.java.net/~chegar/Unsafe_invokeCleaner/
          • Java 10 might get official support for unmapping (I discussed this with Hotspot developers during beers at FOSDEM 2016), the issue is here: https://bugs.openjdk.java.net/browse/JDK-4724038 - thanks to Andrew Haley!

          The attached Lucene patch implements the addition in http://cr.openjdk.java.net/~chegar/Unsafe_invokeCleaner/ (untested because lack of compiled JDK with that patch - I am working on that). It removes the pre-build-148 hack using Runnable and restores Java 8 behaviour, but adds a check before that, which triggers in coming Java 9 and is used for unmapping:

          • Try to load class sun.misc.Unsafe (this requires security permission, which we already have).
          • Lookup the new public (virtual method) there. This requires no security manager interaction.
          • If this works fine, we need to get theUnsafe and bind it to our MethodHandle, so its gets a single-arg MH. This requires that we call setAccessible, that is on the special-cases and supported by Java 9 for the time beeing. This also requires security permission, but not a new one.

          After that point the whole code is compatible and can be used instead of the more complex MH of previous versions (using guardWithTest for null check and filterReturnValue). This MH is returned as BufferCleaner functional interface - as before. The remaining code is the same, just a bit refactoring. I also added an extra check for direct buffers only. This mimics Java 9's check that would then also work in Java 8.

          Please note: The new interface no longer needs the extra permission that was added for Java 9, it only needs RuntimePermission("accessClassInPackage.sun.misc") and ReflectPermission("suppressAccessChecks")!

          This patch no longer support unmapping in older Java 9 versions, so we cannot apply it until we have testable JDK 9 EA builds.

          Show
          Uwe Schindler added a comment - - edited Here is a first patch for the planned Java 9 unmapping. Based on the discussions on the OpenJDK mailing list the following must be noted and implemented. setAccessible() is NO LONGER POSSIBLE on any class from Java's public and documented class library - simple and easy - no excuses or workarounds anymore. There are still a few special cases (like retrieving sun.misc.Unsafe , but thats unsupported API anyways). This is a separate issue and requires, that RAMUsageEstimator in Lucene's tests and RandomizedRunner needs to be disabled ( Dawid Weiss ). I will open one. The first implementation of the Java 9 support is now obsolete, because not even the Runnable approach works with above restriction. We proposed some "temporary workaround" for unmapping for the time beeing. This workaround sounds reasonable and first patches for OpenJDK are under discussion, but might take some time to be committed. The idea is: As sun.misc.Unsafe has a special role in the JDK (you can still get an instance of it if you respect security manager and use the well-known pattern to get it) and the unmapping of mmapped buffers is "unsafe" per se - why not add a method to unmap ByteBuffers to sun.misc.Unsafe (easy job!). OpenJDK patch / CR is here: http://cr.openjdk.java.net/~chegar/Unsafe_invokeCleaner/ Java 10 might get official support for unmapping (I discussed this with Hotspot developers during beers at FOSDEM 2016), the issue is here: https://bugs.openjdk.java.net/browse/JDK-4724038 - thanks to Andrew Haley! The attached Lucene patch implements the addition in http://cr.openjdk.java.net/~chegar/Unsafe_invokeCleaner/ (untested because lack of compiled JDK with that patch - I am working on that). It removes the pre-build-148 hack using Runnable and restores Java 8 behaviour, but adds a check before that, which triggers in coming Java 9 and is used for unmapping: Try to load class sun.misc.Unsafe (this requires security permission, which we already have). Lookup the new public (virtual method) there. This requires no security manager interaction. If this works fine, we need to get theUnsafe and bind it to our MethodHandle , so its gets a single-arg MH. This requires that we call setAccessible, that is on the special-cases and supported by Java 9 for the time beeing. This also requires security permission, but not a new one. After that point the whole code is compatible and can be used instead of the more complex MH of previous versions (using guardWithTest for null check and filterReturnValue ). This MH is returned as BufferCleaner functional interface - as before. The remaining code is the same, just a bit refactoring. I also added an extra check for direct buffers only. This mimics Java 9's check that would then also work in Java 8. Please note: The new interface no longer needs the extra permission that was added for Java 9, it only needs RuntimePermission("accessClassInPackage.sun.misc") and ReflectPermission("suppressAccessChecks") ! This patch no longer support unmapping in older Java 9 versions, so we cannot apply it until we have testable JDK 9 EA builds.
          Hide
          Uwe Schindler added a comment -

          For the Groovy issue when running tests: A new release of Groovy is coming for support Java 9 b148+: http://mail.openjdk.java.net/pipermail/jigsaw-dev/2016-December/010474.html

          Will be released with Groovy 2.4.8. I'll update asap (separate issue)!

          Show
          Uwe Schindler added a comment - For the Groovy issue when running tests: A new release of Groovy is coming for support Java 9 b148+: http://mail.openjdk.java.net/pipermail/jigsaw-dev/2016-December/010474.html Will be released with Groovy 2.4.8. I'll update asap (separate issue)!
          Hide
          ASF subversion and git services added a comment -

          Commit 8a2c71d747456bbef30775f8c966e4c02c48f5bc in lucene-solr's branch refs/heads/LUCENE-6989-v2 from Uwe Schindler
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=8a2c71d ]

          LUCENE-6989: Unmapping byte buffers: Preview version of the Java 9 b148++ patch

          Show
          ASF subversion and git services added a comment - Commit 8a2c71d747456bbef30775f8c966e4c02c48f5bc in lucene-solr's branch refs/heads/ LUCENE-6989 -v2 from Uwe Schindler [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=8a2c71d ] LUCENE-6989 : Unmapping byte buffers: Preview version of the Java 9 b148++ patch
          Hide
          Uwe Schindler added a comment - - edited

          I created a separate branch LUCENE-6989-v2 so OpenJDK devs can test!

          Show
          Uwe Schindler added a comment - - edited I created a separate branch LUCENE-6989 -v2 so OpenJDK devs can test!
          Hide
          Uwe Schindler added a comment -

          Hi Sanne,
          I think we can work on making Lucene 5.5 aware of the new unmapping, but I would wait until shortly before Java 9 is released. The current state is changing too much, so if we release a bugfix 5.5 release it would for sure not be the last. The same would apply for Lucene 5.3.x, although I am not sure if we would ever release a bugfix for that one. What prevents you from upgrading to at least 5.5? FYI, Lucene 5 would possibly work with Java 9, it just cannot unmap.

          Show
          Uwe Schindler added a comment - Hi Sanne, I think we can work on making Lucene 5.5 aware of the new unmapping, but I would wait until shortly before Java 9 is released. The current state is changing too much, so if we release a bugfix 5.5 release it would for sure not be the last. The same would apply for Lucene 5.3.x, although I am not sure if we would ever release a bugfix for that one. What prevents you from upgrading to at least 5.5? FYI, Lucene 5 would possibly work with Java 9, it just cannot unmap.
          Hide
          ASF subversion and git services added a comment -

          Commit 71d3a5039dbcca6066cc7f6e3ac2f1564afd12b0 in lucene-solr's branch refs/heads/LUCENE-6989-v2 from Uwe Schindler
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=71d3a50 ]

          LUCENE-6989: Fix annotation of hack

          Show
          ASF subversion and git services added a comment - Commit 71d3a5039dbcca6066cc7f6e3ac2f1564afd12b0 in lucene-solr's branch refs/heads/ LUCENE-6989 -v2 from Uwe Schindler [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=71d3a50 ] LUCENE-6989 : Fix annotation of hack
          Hide
          ASF subversion and git services added a comment -

          Commit b8fe1ff83fa1243172b2812bd976a6011f671914 in lucene-solr's branch refs/heads/LUCENE-6989-v2 from Uwe Schindler
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=b8fe1ff ]

          LUCENE-6989: Rename variable

          Show
          ASF subversion and git services added a comment - Commit b8fe1ff83fa1243172b2812bd976a6011f671914 in lucene-solr's branch refs/heads/ LUCENE-6989 -v2 from Uwe Schindler [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=b8fe1ff ] LUCENE-6989 : Rename variable
          Hide
          Dawid Weiss added a comment -

          There are still a few special cases (like retrieving sun.misc.Unsafe, but thats unsupported API anyways). This is a separate issue and requires, that RAMUsageEstimator in Lucene's tests and RandomizedRunner needs to be disabled.

          Hmm... There are no Unsafe references in RR, are there? As for setAccessible, they could be present in certain rules but should be guarded with try-catch blocks. Let me know what's not working, I'll fix.

          Show
          Dawid Weiss added a comment - There are still a few special cases (like retrieving sun.misc.Unsafe, but thats unsupported API anyways). This is a separate issue and requires, that RAMUsageEstimator in Lucene's tests and RandomizedRunner needs to be disabled. Hmm... There are no Unsafe references in RR, are there? As for setAccessible, they could be present in certain rules but should be guarded with try-catch blocks. Let me know what's not working, I'll fix.
          Hide
          Uwe Schindler added a comment -

          I am not sure if you have a clone of RAMUsageEstimator inside RR, this is why I asked the question. The RAMUsageEstimator (as it is in Lucene) dooes not work at all with Jigsaw anymore, because you can no longer look into String.class or whatever!

          Show
          Uwe Schindler added a comment - I am not sure if you have a clone of RAMUsageEstimator inside RR, this is why I asked the question. The RAMUsageEstimator (as it is in Lucene) dooes not work at all with Jigsaw anymore, because you can no longer look into String.class or whatever!
          Hide
          Uwe Schindler added a comment -

          Unsafe is not the problem. The problem is RUE!

          Show
          Uwe Schindler added a comment - Unsafe is not the problem. The problem is RUE!
          Hide
          Uwe Schindler added a comment -

          To conclude: The RAMUsageEstimator/Tester/whatever that sums up all static fields before and after test no longer works with Java 9.

          Show
          Uwe Schindler added a comment - To conclude: The RAMUsageEstimator/Tester/whatever that sums up all static fields before and after test no longer works with Java 9.
          Hide
          Dawid Weiss added a comment -

          This is part of StaticFieldsInvariantRule; if you remove it form LuceneTestCase the rest will work fine (should).

          Show
          Dawid Weiss added a comment - This is part of StaticFieldsInvariantRule ; if you remove it form LuceneTestCase the rest will work fine (should).
          Hide
          Uwe Schindler added a comment -

          That's my plan, if it detects Java 9, I will disable it.

          Show
          Uwe Schindler added a comment - That's my plan, if it detects Java 9, I will disable it.
          Hide
          Sanne Grinovero added a comment -

          We have more recent releases of Hibernate Search using Lucene 5.5.x, but we typically aim to support older releases as well, for some reasonable time. It just so happens that Lucene 5.3 isn't that old yet in our perspective. While I constantly work to motivate people to move to the latest, for many Lucene 5.3 is working just great.

          The OSS communities we target typically will not expect API changes in a maintenance release, and we happen to (proudly) expose Lucene as public API, as I believe that hiding it all under some wrapping layer would not be able to be as powerful. Since we expose Lucene as public API implies I can't really update my dependency to Lucene with other than a micro (bugfix) release, when doing a micro/bugfix release myself: people got used that a Lucene major/minor update will only happen in an Hibernate Search major/minor update.

          Of course if that's not feasible, we might have to advise that those older releases won't be compatible with Java 9; that's a possible outcome, I guess we'll see how the final Java 9 release will make this doable. See you at FOSDEM, hopefully with my colleague Andrew Haley as well

          Show
          Sanne Grinovero added a comment - We have more recent releases of Hibernate Search using Lucene 5.5.x, but we typically aim to support older releases as well, for some reasonable time. It just so happens that Lucene 5.3 isn't that old yet in our perspective. While I constantly work to motivate people to move to the latest, for many Lucene 5.3 is working just great. The OSS communities we target typically will not expect API changes in a maintenance release, and we happen to (proudly) expose Lucene as public API, as I believe that hiding it all under some wrapping layer would not be able to be as powerful. Since we expose Lucene as public API implies I can't really update my dependency to Lucene with other than a micro (bugfix) release, when doing a micro/bugfix release myself: people got used that a Lucene major/minor update will only happen in an Hibernate Search major/minor update. Of course if that's not feasible, we might have to advise that those older releases won't be compatible with Java 9; that's a possible outcome, I guess we'll see how the final Java 9 release will make this doable. See you at FOSDEM, hopefully with my colleague Andrew Haley as well
          Hide
          Uwe Schindler added a comment -

          I opened LUCENE-7595 to investigate.

          Show
          Uwe Schindler added a comment - I opened LUCENE-7595 to investigate.
          Hide
          Uwe Schindler added a comment -

          I opened LUCENE-7596.

          Show
          Uwe Schindler added a comment - I opened LUCENE-7596 .
          Hide
          Uwe Schindler added a comment - - edited

          The Unsafe API addition for unmapping will appear in Java 9 build 150: https://bugs.openjdk.java.net/browse/JDK-8171377

          Show
          Uwe Schindler added a comment - - edited The Unsafe API addition for unmapping will appear in Java 9 build 150: https://bugs.openjdk.java.net/browse/JDK-8171377
          Hide
          Uwe Schindler added a comment - - edited

          Hi,
          I built a JDK image with that patch and tried to run the Lucene Java9 unmapper: Works!

          So it's ready!

          Show
          Uwe Schindler added a comment - - edited Hi, I built a JDK image with that patch and tried to run the Lucene Java9 unmapper: Works! So it's ready!
          Hide
          ASF subversion and git services added a comment -

          Commit ffc957fdb3c21d110ab23392ed91e74cfc1f169d in lucene-solr's branch refs/heads/LUCENE-6989-v2 from Uwe Schindler
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=ffc957f ]

          LUCENE-6989: Refactor code and add documentation

          Show
          ASF subversion and git services added a comment - Commit ffc957fdb3c21d110ab23392ed91e74cfc1f169d in lucene-solr's branch refs/heads/ LUCENE-6989 -v2 from Uwe Schindler [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=ffc957f ] LUCENE-6989 : Refactor code and add documentation
          Hide
          ASF subversion and git services added a comment -

          Commit 64c6f359949b62fe981255516ba2286c0adcc190 in lucene-solr's branch refs/heads/LUCENE-6989-v2 from Uwe Schindler
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=64c6f35 ]

          LUCENE-6989: Comments and final cleanup

          Show
          ASF subversion and git services added a comment - Commit 64c6f359949b62fe981255516ba2286c0adcc190 in lucene-solr's branch refs/heads/ LUCENE-6989 -v2 from Uwe Schindler [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=64c6f35 ] LUCENE-6989 : Comments and final cleanup
          Hide
          Uwe Schindler added a comment -

          Final patch, will commit this the next days.

          Show
          Uwe Schindler added a comment - Final patch, will commit this the next days.
          Hide
          ASF subversion and git services added a comment -

          Commit 7e03427fa14a024ce257babcb8362d2451941e21 in lucene-solr's branch refs/heads/master from Uwe Schindler
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=7e03427 ]

          LUCENE-6989: Make MMapDirectory's unmap hack work with Java 9 EA (b150+): Unmapping uses new sun.misc.Unsafe#invokeCleaner(ByteBuffer).

          Show
          ASF subversion and git services added a comment - Commit 7e03427fa14a024ce257babcb8362d2451941e21 in lucene-solr's branch refs/heads/master from Uwe Schindler [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=7e03427 ] LUCENE-6989 : Make MMapDirectory's unmap hack work with Java 9 EA (b150+): Unmapping uses new sun.misc.Unsafe#invokeCleaner(ByteBuffer).
          Hide
          Uwe Schindler added a comment -

          I committed to master as a first step to do initial testing (so it does not break Java 8).

          Show
          Uwe Schindler added a comment - I committed to master as a first step to do initial testing (so it does not break Java 8).
          Hide
          Uwe Schindler added a comment -

          Here is a patch to fix some tests that hardcode MMapDirectory (and also the FSDirectory randomizer), to only use MMapDirectory on Windows, if it supports unmapping. Otherwise tests will fail. I will commit this in a minute.

          Show
          Uwe Schindler added a comment - Here is a patch to fix some tests that hardcode MMapDirectory (and also the FSDirectory randomizer), to only use MMapDirectory on Windows, if it supports unmapping. Otherwise tests will fail. I will commit this in a minute.
          Hide
          ASF subversion and git services added a comment -

          Commit d5e87898b1842b0a0792a3b342e9bed76bc6ee62 in lucene-solr's branch refs/heads/master from Uwe Schindler
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=d5e8789 ]

          LUCENE-6989: Fix some tests that hardcode MMapDirectory (and also the FSDirectory randomizer), to only use MMapDirectory on Windows, if it supports unmapping. Otherwise tests will fail.

          Show
          ASF subversion and git services added a comment - Commit d5e87898b1842b0a0792a3b342e9bed76bc6ee62 in lucene-solr's branch refs/heads/master from Uwe Schindler [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=d5e8789 ] LUCENE-6989 : Fix some tests that hardcode MMapDirectory (and also the FSDirectory randomizer), to only use MMapDirectory on Windows, if it supports unmapping. Otherwise tests will fail.
          Hide
          ASF subversion and git services added a comment -

          Commit 1d92eed93fb6c33f174256a90fcf5ba779575255 in lucene-solr's branch refs/heads/branch_6x from Uwe Schindler
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=1d92eed ]

          LUCENE-6989: Make MMapDirectory's unmap hack work with Java 9 EA (b150+): Unmapping uses new sun.misc.Unsafe#invokeCleaner(ByteBuffer).

          Show
          ASF subversion and git services added a comment - Commit 1d92eed93fb6c33f174256a90fcf5ba779575255 in lucene-solr's branch refs/heads/branch_6x from Uwe Schindler [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=1d92eed ] LUCENE-6989 : Make MMapDirectory's unmap hack work with Java 9 EA (b150+): Unmapping uses new sun.misc.Unsafe#invokeCleaner(ByteBuffer).
          Hide
          ASF subversion and git services added a comment -

          Commit ba5b8184560b255d9bd10288735d2a2a951203c7 in lucene-solr's branch refs/heads/branch_6x from Uwe Schindler
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=ba5b818 ]

          LUCENE-6989: Fix some tests that hardcode MMapDirectory (and also the FSDirectory randomizer), to only use MMapDirectory on Windows, if it supports unmapping. Otherwise tests will fail.

          Show
          ASF subversion and git services added a comment - Commit ba5b8184560b255d9bd10288735d2a2a951203c7 in lucene-solr's branch refs/heads/branch_6x from Uwe Schindler [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=ba5b818 ] LUCENE-6989 : Fix some tests that hardcode MMapDirectory (and also the FSDirectory randomizer), to only use MMapDirectory on Windows, if it supports unmapping. Otherwise tests will fail.
          Hide
          Uwe Schindler added a comment -

          I tested with the official b150 download (not yet public visible), so I backported the commits to branch_6x.

          I cannot install build 150 yet on Jenkins, because the other 2 issues have to be solved. Currently I am working on RAMUsageTester. Also, a Groovy update was not yet released.

          Show
          Uwe Schindler added a comment - I tested with the official b150 download (not yet public visible), so I backported the commits to branch_6x. I cannot install build 150 yet on Jenkins, because the other 2 issues have to be solved. Currently I am working on RAMUsageTester. Also, a Groovy update was not yet released.
          Hide
          Michael McCandless added a comment -

          Hmm why is the Fix Version here both 6.0 and 6.4

          Shouldn't it be just 6.4, meaning "if you want unmap to work correctly with Java 9 you should upgrade to Lucene 6.4"?

          Show
          Michael McCandless added a comment - Hmm why is the Fix Version here both 6.0 and 6.4 Shouldn't it be just 6.4, meaning "if you want unmap to work correctly with Java 9 you should upgrade to Lucene 6.4"?
          Hide
          Uwe Schindler added a comment -

          There is a changes entry in 6.0, too, although that approach is no longer working. This is a long-term issue, so I'd close it once Java 9 is released.

          Show
          Uwe Schindler added a comment - There is a changes entry in 6.0, too, although that approach is no longer working. This is a long-term issue, so I'd close it once Java 9 is released.

            People

            • Assignee:
              Uwe Schindler
              Reporter:
              Uwe Schindler
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:

                Development