On Mon, 21 Nov 2022 10:53:07 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
> 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.
src/java.base/share/classes/java/nio/Buffer.java line 785:
> 783: static final JavaNioAccess.SessionAcquisition
> NO_OP_CLOSE = new JavaNioAccess.SessionAcquisition() {
> 784: @Override
> 785: public void close() throws RuntimeException {}
The throws RuntimeException is not needed here. Also would it be possible to
reflow the comment to avoid the wildly long line.
src/java.base/share/classes/jdk/internal/access/JavaNioAccess.java line 161:
> 159:
> 160: @Override
> 161: void close() throws RuntimeException;
You can drop throws RuntimeEXcepton here too.
src/java.base/share/classes/sun/nio/ch/DirectBuffer.java line 38:
> 36: // try (var sessionAcquisition =
> NIO_ACCESS.acquireSessionAsAutoCloseable(bb)) {
> 37: // performOperation(bb.address());
> 38: // }
This comment is very messy and needs to be cleaned up.
src/java.base/share/classes/sun/nio/ch/NioSocketImpl.java line 252:
> 250: try {
> 251: // 'dst' is guaranteed not to be associated with a closeable
> session.
> 252: // Hence, there is no need for acquiring any session.
This comment is will be confusing to anyone reading this code. Is this really
needed?
-------------
PR: https://git.openjdk.org/jdk/pull/11260
