OpenSSL and panama-foreign

[Rémy Maucherat]

On Thu, Nov 11, 2021 at 6:01 PM Maurizio Cimadamore
<maurizio.cimadamore at oracle.com> wrote:
>
> Just to be clear - can you see the cleaner working with your current
> implementation?
>
> After thinking about your use case some more, I'm not convinced that
> what you do is enough to prevent reachability issues.
>
> That is - the MH is kept alive by the CLinker runtime (at least until
> the scope is closed). But, with `bindTo`, the MH also keeps `this` alive.
>
> It follows that, unless the scope is closed, `this` will never become
> unreachable. Which seems to defeat the point of registering `this`
> against a cleaner on construction?
>
> Maurizio
>
> P.S.
>
> I did some experiments with your panama-foreign branch, and it seems
> like, when running the `ab` test, all calls to the cleaner action comes
> from the shutdown method, which invokes the cleaner's cloneable
> manually. If I remove the call to Cleanable::clean in shutdown, then I
> don't see any call to the cleanup action being executed for the entire
> duration of the benchmark.

I have made some progress since then. Thanks for all the help !

The Java 17 version now resides here:
https://github.com/apache/tomcat/tree/main/modules/openssl-java17 (it
should be integrated in the Tomcat binary soon since the jar is very
small and the Java 17 target is reasonable) and all seem well.
Actually, except that it is slapped as "super experimental early
preview" and will have to stay that way for a while, it looks a lot
better to me than the JNI code in tomcat-native.

I have the following feedback for the API:
- It seems bound upcalls will always leak memory, there's no way to
release them. Is a functional fix possible in the future ?
- When using a close action attached to a scope (let's say an implicit
scope, which is what I am now using thanks to your good
recommendations), the action cannot use a reference to a memory
segment from the scope (it has to use a memory address instead),
otherwise the GC thinks it's a real reference to the scope and GC will
never happen.
- On JVM shutdown, the implicit scopes don't seem to be closed, I only
get my close action called on a GC. Am I missing something (again ;) )
?
- I know it is planned, but right now the handling of heap memory with
segments is not very useful. One of the first things I tried is
MemorySegment.ofByteBuffer, and it worked great ... with a direct
ByteBuffer. This seemed like a good opportunity for a big optimization
since it would save a lot of byte copying, and the code as a whole
could likely stop using direct ByteBuffer.
- Could there be some opportunities for stronger typing with the code
that jextract generates ? I (not very surprisingly) experienced a few
bugs caused by the wrong address being passed to a call. Example:
could there be a Pointer<TYPE> as a "fake" type for a MemorySegment,
but providing strong typing ?

Rémy


>
>
> On 11/11/2021 15:26, Maurizio Cimadamore wrote:
> > I'm not sure I follow your point entirely.
> >
> > But yes, if you do a bindTo(this), then using an implicit scope is not
> > very useful as long as the scope is reachable from `this`.
> >
> > We might be able to refine the implementation and improve this in the
> > future, but for now I think that's something we have to live with.
> >
> > Thanks
> > Maurizio
> >
> >
> > On 11/11/2021 14:41, Rémy Maucherat wrote:
> >> Unless I missed something the API has a side effect with upcalls. I
> >> understand after reading the javadoc where the suggestion comes from,
> >> as it makes memory management very much like Java. This is of course
> >> excellent. However, I use upcalls (as you know) and many of them are
> >> bound to an instance (since this makes state tracking very very nice).
> >> This however creates a reference that never goes away and an implicit
> >> scope is never going to get GCed and closed (I saw that with visualvm)
> >> and memory leaks. Initially I was planning to use a static Map<Long,
> >> State> for my state (OpenSSL allows passing references and getting
> >> them in callbacks, but this is a lot handier in C and doesn't make a
> >> lot of sense in Java), then I found MethodHandle.bindTo actually
> >> worked and this is quite magical.
> >>
> >> So with that in mind, what is the recommendation ? Should the API
> >> provide a way to remove the MethodHandle upcall ? Or should I avoid
> >> using bindTo instead ?