On Mon, 28 Nov 2022 10:47:47 GMT, Per Minborg <pminb...@openjdk.org> wrote:
>> This PR proposes the introduction of **guarding** of the use of
>> `DirectBuffer::address` within the JDK. With the introduction of the Foreign
>> Function and Memory access, it is possible to derive Buffer instances that
>> are backed by native memory that, in turn, can be closed asynchronously by
>> the user (and not only via a `Cleaner` when there is no other reference to
>> the `Buffer` object). If another thread is invoking `MemorySession::close`
>> while a thread is doing work using raw addresses, the outcome is undefined.
>> This means the JVM might crash or even worse, silent modification of
>> unrelated memory might occur.
>>
>> Design choices in this PR:
>>
>> There is already a method `MemorySession::whileAlive` that takes a runnable
>> and that will perform the provided action while acquiring the underlying`
>> MemorySession` (if any). However, using this method entailed relatively
>> large changes while converting larger/nested code segments into lambdas.
>> This would also risk introducing lambda capturing. So, instead, a
>> ~~try-with-resources~~ *try-finally* friendly access method was added. This
>> made is more easy to add guarding and did not risk lambda capturing. Also,
>> introducing lambdas in certain fundamental JDK classes might incur bootstrap
>> problems.
>>
>> The aforementioned ~~TwR~~ TF is using a ~~"session acquisition" that is not
>> used explicitly in the try block itself~~ session used in the *finally*
>> block. ~~This raises a warning that is suppressed using
>> `@SuppressWarnings("try")`. In the future, we might be able to remove these
>> suppressions by using the reserved variable name `_`.~~
>>
>> In some cases, where is is guaranteed that the backing memory session is
>> non-closeable, we do not have to guard the use of `DirectBuffer::address`.
>> ~~These cases have been documented in the code.~~
>>
>> On some occasions, a plurality of acquisitions are made. This would never
>> lead to deadlocks as acquisitions are fundamentally concurrent counters and
>> not resources that only one thread can "own".
>>
>> I have added comments (and not javadocs) before the declaration of the
>> non-public-api `DirectBuffer::address` method, that the use of the returned
>> address needs to be guarded. It can be discussed if this is preferable or
>> not.
>>
>> This PR spawns several areas of responsibility and so, I expect more than
>> one reviewer before promoting the PR.
>
> Per Minborg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Re-introduce yet another address vairable
The approach looks good, and almost the least intrusive (see comment).
src/java.base/share/classes/java/nio/Buffer.java line 838:
> 836:
> 837: @Override
> 838: public void releaseSession(Buffer buffer,
> MemorySessionImpl scope) {
I prefer methods that do not expose the scope implementation so access is
limited to just to the acquire/release methods, but i am unsure of the
performance implications. These methods might not reliably inline, and we may
need to ensure that first (which is also separately a good thing). I think it
requires that the shared secret fields are stable and that there is only one
implementation of `JavaNioAccess`, which there is, but we can enforce via
sealing. Something to consider as a further iteration.
-------------
Marked as reviewed by psandoz (Reviewer).
PR: https://git.openjdk.org/jdk/pull/11260
