[foreign-memaccess+abi] RFR: 8263018: Improve API for lifecycle of native resources [v2]

[Jorn Vernee]

On Wed, 10 Mar 2021 15:49:52 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/NativeMemorySegmentImpl.java line 121:
>> 
>>> 119:         AbstractMemorySegmentImpl segment = new NativeMemorySegmentImpl(min.toRawLongValue(), bytesSize, defaultAccessModes(bytesSize), scope);
>>> 120:         if (cleanupAction != null) {
>>> 121:             scope.addOnClose(cleanupAction);
>> 
>> Why is this one not using `addOrCleanupIfFail` as well?
>
> This is a bit controversial: should the cleanup action be called if there's a race and, by the time we create the unsafe segment, the scope is already closed? Normally, we call `addOrCleanupIfFail` because we don't want e.g. `MemorySegment::allocateNative` to leave resource around if the scope happens to be closed - but in this case? I think I stick by the current decision - otherwise we're essentially making a guarantee that the cleanup action provided by the user will always be called, which I'm not sure I wanna make.

Hmm right, the user might not be cleaning up a resource in the `onClose`. I.e. they can catch the exception and then clean up if needed (or crash and fix their concurrency bug). Probably not worth it looking at this too much. Thanks for the answer.

>> src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/aarch64/AArch64VaList.java line 312:
>> 
>>> 310:                     consumeGPSlots(1);
>>> 311: 
>>> 312:                     try (ResourceScope scope = ResourceScope.ofConfined()) {
>> 
>> This seems like it might be confined to another thread than before now (if the access is happening from a different thread than the one that created the VaList).
>> 
>> I think this could be solved by adding an `asConfined` overload that explicitly takes a thread to confine to, and then here do:
>> 
>> try (ResourceScope scope = ResourceScope.ofConfined(segment.scope().ownerThread())) {
>> 
>> WDYT?
>
> not sure I get the suggestion, but when looking at this my feeling was that this method was just copying some data off to some other segment which is then allocated using the provided allocator. I believe what is missing here is a check that the guy calling read() can in fact do that (e.g. if the valist is confined, is the thread the same?) - but I'm not sure that, other than the check, we need something else?

Yeah, AFAIK the only thing we need to check here is that the caller can actually read the segment. In fact, I don't think we need to create a scope here at all, since we don't return the created scope, we can just use the global scope I think.

>> src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/x64/windows/WinVaList.java line 181:
>> 
>>> 179: 
>>> 180:         public Builder(ResourceScope scope) {
>>> 181:             ((MemoryScope)scope).checkValidStateSlow();
>> 
>> How come this state check only happens in the Windows VaList Builder implementation?
>
> because in the other cases it happens implicitly via allocation

Ah, ok

-------------

PR: https://git.openjdk.java.net/panama-foreign/pull/466