On Thu, 24 Nov 2022 11:42:52 GMT, Per Minborg <[email protected]> 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 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 is using a "session acquisition" that is not used
>> explicitly in the try block itself. 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 `_`. Another
>> alternative was evaluated where a non-autocloseable resource was used.
>> However, it became relatively complicated to guarantee that the session was
>> always released and, at the same time, the existing 'final` block was always
>> executed properly (as they both might throw exceptions). In the end, the
>> proposed solution was much simpler and robust despite requiring a
>> non-referenced TwR variable.
>>
>> 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:
>
> Remove redundant method
src/java.base/share/classes/sun/nio/ch/DatagramChannelImpl.java line 790:
> 788: try {
> 789: int n = receive0(fd,
> 790: ((DirectBuffer)bb).address() + pos, rem,
Would it be possible to restore the original alignment, just to make it easier
to read.
src/java.base/share/classes/sun/nio/ch/DirectBuffer.java line 39:
> 37: // silent unrelated memory mutation and JVM crashes.
> 38: //
> 39: // Guards are available in the JavaNioAccess class.
Did you mean to leave the word "Guards" in the comment? I guess I would say
something "See JavaNioAccess for methods to acquire/release" or something like
that.
src/java.base/share/classes/sun/nio/ch/IOUtil.java line 480:
> 478: static MemorySessionImpl acquireScope(ByteBuffer bb, boolean async) {
> 479: if (async && NIO_ACCESS.isThreadConfined(bb)) {
> 480: throw new IllegalStateException("Confined session not
> supported");
No sure about ISE here. There is no mutable state on the input that would allow
the acquire to succeed. I don't think anyone has run into it yet but we will
need to look at the AsynchronousXXXX read/write methods to decide which
exception to specify.
src/java.base/share/classes/sun/nio/ch/IOUtil.java line 543:
> 541: @Override public void run() { releaseScope(bb, session); }
> 542: static Runnable of(ByteBuffer bb, MemorySessionImpl session) {
> return new Releaser(bb, session); }
> 543: static Runnable ofNullable(ByteBuffer bb, MemorySessionImpl
> session) {
Would it be possible to re-format this to make it more readable? There's just a
bit more going on compared to the original and it's harder to read.
-------------
PR: https://git.openjdk.org/jdk/pull/11260
