OpenSSL and panama-foreign

[Maurizio Cimadamore]

> I have made some progress since then. Thanks for all the help !
>
> The Java 17 version now resides here:
> https://urldefense.com/v3/__https://github.com/apache/tomcat/tree/main/modules/openssl-java17__;!!ACWV5N9M2RV99hQ!Z-4p41ECvTd-Gf3IWB2Y7qYBN0gMId9qKAXx2f5FgE00I0Pc4TdJ7hD97faRbwzTxmrN2Nw$  (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.
Hi Remy, thanks for trying out the bits and the feedback so far. Some 
replies
>
> 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 ?

In this case, the issue is that if you use implicit scope with an 
upcall, and the upcall has a strong reference (via its method handle) 
back to the scope, there is an (unavoidable, I think) issue: the method 
handle is stored somewhere (as a global JNI handle) in the VM stub. This 
is enough to keep the MH alive (which we need to), but also, indirectly 
(as the scope is attached to the MH in your case), to keep the scope 
alive. If we create a weak handle on the VM stub side, that would solve 
the issue in terms of the stub not keeping the reachability on the scope 
- but it will create other issue (e.g. an upcall crashing because the MH 
it refers to has disappeared). Jorn and I have thought about this case a 
bit - at the moment we don't have any clever ideas - I think using bound 
MHs with VM upcalls is something to be used with care, given 
implications on reachability.

But we'll keep thinking of course.


> - 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.

This is, I think, to be expected - as this is in line with 
recommendentation for cleanup action for Cleaners - e.g. that they have 
to be "static" so as not to capture reference to the object being collected.

With the API, there is typically an option to "drop down" at the memory 
address level. In 18, a MemoryAddress is just a wrapper around a long, 
so it won't keep a scope alive. You can convert the address back to a 
segment if needed (MemorySegment::ofAddress) - not sure whether this 
will help your particular use case, but it's an option to keep in mind.

> - 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 ;) )
> ?

We register the scope against a cleaner, it is possible that cleaner 
actions are only called under GC pressure, but not on VM shutdown. I'd 
say that there's no guarantee as to _when_ these actions can be called 
(it's all GC implementation dependent).

I think your question is: how do I create a memory resource that is 
destroyed implicitly on VM shutdown? We do not support that use case at 
the moment; an implicit scope is just a scope backed by a cleaner.

> - 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.
Not sure I follow here. Do you intend to say that 
MemorySegment.ofByteBuffer doesn't work with an heap byte buffer (that 
would be odd, but if true I'd like to know more!) - or that 
MemorySegment are not very useful when interacting with heap arrays 
(which I think has been addressed in 18) ?
> - 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 ?

We remain on the outlook for such opportunities - at the moment, 
introducing these types is not possible w/o a significant cost in terms 
of instances being created and/or additional overhead on memory 
dereference (or both). Some future language/VM features (Valhalla) might 
significantly change the peformance trade off, iin which case we will 
obviously reconsider.

Thanks
Maurizio

>
> 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 ?