[foreign-memaccess] [Rev 01] RFR: Preserve memory scope for buffer segments

[Maurizio Cimadamore]

No worries - as I was tweaking a related area, I've addressed these 
comments as part of

https://github.com/openjdk/panama-foreign/pull/117

Cheers
Maurizio

On 16/04/2020 18:59, Paul Sandoz wrote:
> On Thu, 16 Apr 2020 16:05:13 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:
>
>>> Not 100% sure about this, but wanted to share this PR for evaluation. Basically, I've realized that we are also not
>>> preserving the memory scope of the original segment when we do a segment -> buffer -> segment transition.
>>> This means that, e.g. code like this:
>>>
>>> MemorySegment segment = MemorySegment.allocateNative(16);
>>> segment = MemorySegment.ofByteBuffer(segment.asByteBuffer());
>>> segment.close(); // no cleanup of native memory
>>>
>>> will not really 'close' the original segment. Similarly, if the original segment is being worked upon by some other
>>> thread (through a spliterator) it is possible for the morphed segment to lose this info, and to allow a close - if the
>>> scope is created afresh (as is now).  This patch fixes this, but I'm torn as to whether it makes things better;
>>> preserving access modes and thread ownership is mostly about not allowing clients to use the byte buffer escape hatch
>>> as a way to defy the segment restrictions. I don't think preserving scope falls into the same bucket - e.g. this is a
>>> separate question as to whether a segment S2 obtained through S1 -> BB -> S2 is just a *view* of S1 (with same temporal
>>> bounds), or is just an independent segment, with new temporal bounds. I think both answers are legitimate, and it's
>>> mostly a question of making up our mind.  My take is that inferring scope like this can be seen a bit too magic - and
>>> you are never sure of what happens when you do a MemorySegment::close, so perhaps what we have w/o this patch is
>>> preferrable. What do you think?
>> Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
>>
>>    Added test (contributed by Jorn Vernee)
> test/jdk/java/foreign/TestSharedAccess.java line 142:
>
>> 141:     @Test(expectedExceptions=IllegalStateException.class)
>> 142:     public void testBadCloseWithPendingAcquireBuffer() throws InterruptedException {
>> 143:         MemorySegment segment = MemorySegment.allocateNative(16);
> I think this test can be improved to avoid the 5s execution time, plus a dangling thread hanging around for 500s.  With
> some latches we can ensure the main thread calls `segment.close()` while another other thread is within the
> `spliterator.forEachRemaining` (and further ensure the runnable task completes before the test completes).  I'll send a
> pull request.
>
> -------------
>
> PR: https://git.openjdk.java.net/panama-foreign/pull/113