[foreign-memaccess+abi] RFR: Improve ResourceScope javadoc [v2]

[Maurizio Cimadamore]

On 21/04/2021 02:24, leerho wrote:
> Maurizio,
>
> You are a step ahead of me as I had some of the same concerns, not only
> with the top-level docs, but the method docs as well.  Before I saw this
> thread I spent several hours flipping back and forth between the method
> docs and the top-level docs and running snips of code trying to grok what
> was really going on.  So I wrote the attached in the spirit of trying to
> help make the docs more clear.   Forgive me as I haven't had time to go
> through your changes yet, but I want to share some of these ideas and
> perhaps some of it will be useful to you.
>
> The biggest concern I have is that with the current API (prior to your
> changes) I didn't see a way to safely close the confined or shared scopes
> in the presence of handles without the risk of an exception.  So I propose
> a "hasHandle()" method.  And, as you will see from my table and comments at
> the end, having acquire() return null may make sense for some of the shared
> and confined cases as well

I think I see where you are coming from - it's not the first time that 
this concern is raised. Let me try to clarify what's going on and why.

First, I don't think we can offer any API in the spirit of what your 
propose e.g. hasHandle/handleCount, ...

That's because there would be absolutely no guarantee that the value 
returned will prevent an exception - e.g.

```
if (!scope.hasHandle()) // check here {
     scope.close(); // whoops - what if scope has been closed since we 
last checked?
}
```

So, I think these methods promote bad usage of the API.

We could add methods such as `tryClose` and the like - which might 
return boolean if the thing is not closed. But.

The point I make is that when you get an IllegalStateException when 
calling close() it is generally a _big thing_. These kind of issues 
should never be resolved by "whoops, let's try again later?"; as the 
javadoc calls out, the last thing we want developers to do, is this:

```
while (true) {
try {
     scope.close();
     break;
} catch (IllegalStateException ise) {
    continue;
}
```


If the scope is a shared scope, doing the above will _bottleneck the 
performance of the entire Java application_. (not dissimilar, although 
less costly, to calling System.gc() in a loop).

The idea is that if you get an IllegalStateException, there is likely a 
lifecycle issue in the application, a race between access/close or a 
race between acquire/close. Both cases are examples of lack of 
synchronization in the user code. The memory access API will guarantee 
that, no matter what, the JVM will _not_ crash, no matter what clients 
do. But if what clients do is morally wrong, I don't think the API 
should enable them to write "bad" code.

Now, I understand that, in the previous iteration of the API, each 
segment had its own scope, and it was tempting to wrap every segment 
with a try with resources - which could sometimes lead to issues (e.g. 
slices). This is the main reason as to why the AutoCloseable unit of 
currency is no longer MemorySegment, but ResourceScope.

We envision that segments will be create in a more centralized way, 
perhaps using segment allocators, or memory pools (the experiment done 
in [1] is a very good approach) - so that clients won't have to worry 
about "closing" that much (as that "close" operation will likely happen 
at the boundary of the application - where clients can insert any kind 
of expensive synchronization mechanism to make really sure that 
resources are no longer needed).

Of course, this remains a low level API - and when interacting with non 
trusted clients, acquiring scopes might be a problem - for instance:

```
try (ResourceScope scope = ...) {
     MemorySegment segment = MemorySegment.allocateNative(100, scope);
     plugin.execute(segment);
}
```

Is this code correct? Well, it depends. If the plugin code does an 
acquire and never releases (which is a bug), then the TWR will fail on 
close. But this kind of issue is widespread in all low level memory APIs 
which provide some kind of reference counting mechanism - for instance, 
in [2] you will find some "guidelines" for working with reference 
counted objects in Netty; of course these are guidelines - clients can 
still be uncooperative, and misuse the mechanism.

The acquire/handle API mechanism we have added here has some advantages 
over plain reference counting: it is _asymmetric_ meaning that you can 
only release an handle that you have acquired. So you can't do stuff like:

```
scope.release().release().release().close();
```

But some of the issues/dangers associated with reference counting are 
still there, of course, and are, I think, unavoidable in a framework 
which provides deterministic memory deallocation.

Maurizio

[1] - https://git.openjdk.java.net/panama-foreign/pull/509
[2] - https://netty.io/wiki/reference-counted-objects.html


>
> I also prefer to have method docs that are more-or-less self-contained so
> the user doesn't have to guess what circumstances will create exceptions.
>
> I'm sure you have better ideas :)
>
> Lee.
>
>
>
> On Tue, Apr 20, 2021 at 8:48 AM Paul Sandoz <psandoz at openjdk.java.net>
> wrote:
>
>> On Tue, 20 Apr 2021 12:19:44 GMT, Maurizio Cimadamore <
>> mcimadamore at openjdk.org> wrote:
>>
>>>> Chris and I had some discussions around the new `ResourceScope` API,
>> and realized that the toplevel javadoc doesn't make it easy for developers
>> to grasp all the various dimensions of resource scopes.
>>>> This is mostly caused by the fact that the concept of implicit closure
>> is used to refer to _all_ scopes that support some form of GC-backed
>> closure - and this brings confusion, as we have added the concept of
>> "implicit" scopes to denote those scopes that *cannot* be closed explicitly.
>>>> I have rewritten the javadoc to talk about explicit and implicit
>> scopes; we now say, in the section on explicit scopes, that explicit scopes
>> can sometimes be associated with a Cleaner, but it is now clearer that
>> that's mostly a fallback option.
>>>> When working on this, I think that we should probably make
>> `ResourceScope::acquire` fail for implicit scope - either return null or
>> throw. In other words, while acquiring handles makes sense to prevent
>> deterministic deallocation, it seems like it doesn't make a lot of sense to
>> prevent implicit deallocation (we already have reachability fences for
>> this). What the implementation does right now permanently capture the scope
>> instance in a handle instance, so the original implicit scope will never be
>> closed as long as handles are reachable, even after close. In other words,
>> having an handle that can be closed doesn't make sense for implicit scopes,
>> since `Handle::close` doesn't do anything intersting in that case (or is
>> not sufficient).
>>>> Any thoughts?
>>> Maurizio Cimadamore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>>    Address review comments
>> Marked as reviewed by psandoz (Committer).
>>
>> -------------
>>
>> PR: https://git.openjdk.java.net/panama-foreign/pull/510
>>