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

[Maurizio Cimadamore]

On Mon, 8 Mar 2021 16:43:54 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/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.

> src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/ResourceList.java line 55:
> 
>> 53:         public abstract void cleanup();
>> 54: 
>> 55:         final static ResourceCleanup DUMMY_CLEANUP = new ResourceCleanup() {
> 
> Had to look at this twice. For some reason I thought DUMMY_CLEANUP was the head of an empty list. Maybe this field could be renamed to `CLOSED_CLEANUP`?

yeah - I agree that it's used as a way to signal "list is closed".

> src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/ResourceList.java line 42:
> 
>> 40:     }
>> 41: 
>> 42:     final void cleanup(ResourceCleanup first) {
> 
> Should/could this be static?

no strong preference

> src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/ResourceList.java line 127:
> 
>> 125:                 while (true) {
>> 126:                     prev = (ResourceCleanup) FST.getAcquire(this);
>> 127:                     // no need to check for DUMMY, since only one thread can get here!
> 
> I see this comment is true since `MemoryScope::justClose` is called just before this, which for a shared scope takes care of the races. This also seems to be based on the assumption that a shared scope is always used together with a shared ResourceList and vice-versa. But, in the current design it still seems possible to mix up the two?
> 
> Would it make sense to change the grouping of classes like so:
> 
> class MemoryScope {
>     ...
>     static class ResourceList {...}
> }
> 
> class ConfinedScope {
>     ...
>     private static class ConfinedResourceList {...}
> }
> 
> class SharedScope {
>     ...
>     private static class SharedResourceList {...}
> }
> 
> That way it seems more clear that ConfinedResourceList and SharedResourceList are implementation details of ConfinedScope and SharedScope respectively?

Yeah - I started off this way, then moved off to the current organization as MemoryScope was getting biggie - but yes, we should do something better here

> src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/ResourceList.java line 133:
> 
>> 131:                 }
>> 132:                 cleanup(prev);
>> 133:             }
> 
> I think an exception should be thrown along the `else` branch. Since there seems to be no way that the list could already have been closed at this point? (Would rather make sure and throw an exception here).
> 
> Maybe just turn it into more of an assert style if?
> 
> if (fst == ResourceList.DUMMY_CLEANUP)
>     throw new IllegalStateException("Already closed?");
> 
> Same for the confined one.

ok, good idea

> src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/MemoryScope.java line 255:
> 
>> 253:             public void close() {
>> 254:                 checkValidState(); // thread check
>> 255:                 if (!released) {
> 
> `released` flag should be updated as well here? (Otherwise it's possible to release the same handle twice, without it being a no-op the second time AFAICS).

of course!

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

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