[
https://issues.apache.org/jira/browse/LUCENE-6989?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15148494#comment-15148494
]
Uwe Schindler edited comment on LUCENE-6989 at 2/16/16 12:28 PM:
-----------------------------------------------------------------
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:
{code:java}
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
}
}
{code}
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.
was (Author: thetaphi):
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:
{code:java}
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
}
}
{code}
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}}. f we want to backport we
must change this a bit and use {{Objects.equals}} instead:
{{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.
> Implement MMapDirectory unmapping for coming Java 9 changes
> -----------------------------------------------------------
>
> Key: LUCENE-6989
> URL: https://issues.apache.org/jira/browse/LUCENE-6989
> Project: Lucene - Core
> Issue Type: Task
> Components: core/store
> Reporter: Uwe Schindler
> Assignee: Uwe Schindler
> Priority: Critical
> Labels: Java9
> Attachments: LUCENE-6989-disable5x.patch, LUCENE-6989.patch,
> LUCENE-6989.patch, LUCENE-6989.patch, LUCENE-6989.patch
>
>
> Originally, the sun.misc.Cleaner interface was declared as "critical API" in
> [JEP 260|http://openjdk.java.net/jeps/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|http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-January/thread.html#38243].
> 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.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
