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

[Maurizio Cimadamore]

On Fri, 5 Mar 2021 16:38:45 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

>> Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fix issue in ResourceScope::ofShared() javadoc
>
> src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemoryAddress.java line 103:
> 
>> 101:      * This method is equivalent to the following code:
>> 102:      * <pre>{@code
>> 103:     asSegmentRestricted(byteSize, null, ResourceScope.ofShared(Cleaner.create()));
> 
> Is this code still correct? Should it be `asSegmentRestricted(bytesSize, null, ResourceScope.globalScope());` ?

yep -good catch

> src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemoryAddress.java line 92:
> 
>> 90: 
>> 91:     /**
>> 92:      * Returns a new confined native memory segment with given size, and whose base address is this address. This method
> 
> Not confined, right?

yeah

> src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemoryAddress.java line 121:
> 
>> 119: 
>> 120:     /**
>> 121:      * Returns a new confined native memory segment with given size, and whose base address is this address. This method
> 
> Same here, not confined right? (it depends on the passed ResourceScope)

yep

> src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java line 383:
> 
>> 381:      * <p>
>> 382:      * If this segment is associated with a shared scope, calling certain I/O operations on the resulting buffer might result in
>> 383:      * an unspecified exception being thrown. Examples of such problematic operations are {@link FileChannel#read(ByteBuffer)},
> 
> Is this still the case, or can acquire now be used to prevent this?

For now this is still the case - but Chris has put together a patch which helps removing this restriction - so this will go away.

> src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ResourceScope.java line 63:
> 
>> 61:  * Resource scopes can be associated with a {@link Cleaner} instance (see {@link #ofConfined(Cleaner)}) - we call these
>> 62:  * resource scopes <em>managed</em> resource scopes. A managed resource scope is closed automatically once the scope instance
>> 63:  * becomes <em>unreachable</em>.
> 
> Maybe this could link to https://docs.oracle.com/en/java/javase/15/docs/api/java.base/java/lang/ref/package-summary.html#reachability ?

good idea!

> src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ResourceScope.java line 219:
> 
>> 217:      * @param attachment an attachment object which is kept alive by the returned resource scope (can be {@code null}).
>> 218:      * @param cleaner the cleaner to be associated with the returned scope. Can be {@code null}.
>> 219:      * @return a new confined scope, managed by {@code cleaner}; the resulting scope is closeable if {@code closeable == true}.
> 
> Is the `closeable == true` part still needed? Seems to be from a parameter that was removed.

whoops!

> src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/LibrariesHelper.java line 96:
> 
>> 94:             return new WeakReference<>(s);
>> 95:         });
>> 96:         return new LibraryLookupImpl(library, scopeRef.get());
> 
> I think there's still a narrow window here where `scopeRef` could be cleared, since the holder array is not necessarily alive until the end of the method AFAICS. I think this needs a reachability fence until after the call to get.
> Suggestion:
> 
>         });
>         ResourceScope scope = scopeRef.get();
>         Reference.reachabilityFence(holder[0]);
>         return new LibraryLookupImpl(library, scope);
> 
> (or maybe using a try-finally block)

ok

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

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