Java 9 build 148 causes trouble in Apache Lucene/Solr/Elasticsearch

[Peter Levart]

Hi Uwe,


On 12/11/2016 01:57 PM, Uwe Schindler wrote:
>
> Hi,
>
> How about the following:
>
> -Check that the buffer is direct, if not throw IAE(“not direct buffer”)
>
> -Check that buffer has attachment==null (this tells you that it’s not 
> a slice/dup), if not throw IAE(“not allowed to free duplicates/slices”)
>
> -Finally do the standard if (cleaner!=null) cleaner.clean(), but don’t 
> throw any exceptions if cleaner is null (as this is implementation detail)
>
> This allows for empty buffers without cleaner that are still marked as 
> direct. But it disallows all slices or duplicates.
>

Yes, this would be the right logic I agree. It would silently ignore the 
requests to free memory for buffers constructed via JNI's 
NewDirectByteBuffer calls, but I suppose this would not be a problem in 
practice.

> I am fine with Alan’s proposal to restrict to MappedByteBuffer but 
> that’s out of my interest – I am happy to unmap mapped byte buffers. I 
> would also place the method in the legacy sun.misc.Unsafe only, the 
> JDK-internal private one is not accessible to the outside. Of course 
> for consistency it could be in both, but primarily it must be in 
> sun.misc.Unsafe – that’s also what most code is using anyways.
>

Yes, internally, at least in java.base, the code can always directly 
invoke the DirectBuffer's and Cleaner's methods...

Regards, Peter


> Uwe
>
> *From:*Peter Levart [mailto:peter.levart at gmail.com]
> *Sent:* Saturday, December 10, 2016 10:23 PM
> *To:* Uwe Schindler <uschindler at apache.org>; Chris Hegarty 
> <chris.hegarty at oracle.com>
> *Cc:* jigsaw-dev at openjdk.java.net; Core-Libs-Dev 
> <core-libs-dev at openjdk.java.net>
> *Subject:* Re: Java 9 build 148 causes trouble in Apache 
> Lucene/Solr/Elasticsearch
>
> On 12/10/2016 09:00 PM, Uwe Schindler wrote:
>
>     Hi,
>
>     We noticed that buffers with zero length also have no cleaner.
>     This is why we also have the null check in our code (see Github)
>     and the guardWithTest in the MethodHandle, although we never free
>     duplicates. So a noop is better imho.
>
>
> Oh yes, good catch. Then what about being noop just for zero length? I 
> don't know, maybe I'm just being paranoid and those who would use this 
> API know perfectly well what they are doing. I'm just imagining a 
> situation where one would create and keep just a duplicate of a direct 
> buffer and afterwards use it to try to deallocate the native memory. 
> This would be a noop, but the developer would think it works as GC 
> would finally do it for him. I think it's better to throw an exception 
> to prevent such situations...
>
> Regards, Peter
>
>
>
>     I like the Unsafe approach. To me both variants are fine.
>
>     Uwe
>
>     Am 10. Dezember 2016 20:47:46 MEZ schrieb Peter Levart
>     <peter.levart at gmail.com> <mailto:peter.levart at gmail.com>:
>
>         Hi Chris,
>
>         On 12/10/2016 06:11 PM, Chris Hegarty wrote:
>
>             How about: Unsafe::deallocate(ByteBuffer directBuffer)?
>
>                http://cr.openjdk.java.net/~chegar/Unsafe_deallocate/
>             <http://cr.openjdk.java.net/%7Echegar/Unsafe_deallocate/>
>
>
>         Apart from the fact that Unsafe is (was?) reserved for
>         low-level stuff, I think this approach is reasonable. Is the
>         method in jdk.internal.misc.Unsafe needed? You could add the
>         method just to the sun.misc.Unsafe (to keep internal Unsafe
>         free from hacks) and export the two packages selectively to
>         jdk.unsupported.
>
>
>             We could attempt to limit this to the direct buffer that "owns" the
>
>             memory, i.e. not a duplicate or a slice, but I'm not sure it is worth
>
>             it.
>
>
>         What you have here *is* limited to direct ByteBuffer(s) that
>         "own" the memory. Derived buffer(s) (duplicated or sliced) do
>         not have a Cleaner instance (they have an 'attachment' to keep
>         the 1st-level buffer reachable while they are reachable). I
>         would even make it more unforgiving by throwing an IAE if the
>         passed-in buffer didn't have a Cleaner. In addition I would
>         specify this behavior. For example:
>
>         "Deallocates the underlying memory associated with given
>         directBuffer if the buffer was obtained from either {@link
>         ByteBuffer#allocateDirect} or {@link FileChannel#map} methods.
>         In any other case (when the buffer is not a direct buffer or
>         was obtained by  {@link ByteBuffer#duplicate() duplicating} or
>         {@link ByteBuffer#slice(int, int) slicing} a direct buffer),
>         the method throws {@code IllegalArgumentException}.
>
>         Regards, Peter
>