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
    • 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-v2.patch
        3 kB
        Uwe Schindler

        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

          People

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

            Dates

            • Created:
              Updated:

              Development