Re: RFR: 8296024: Usage of DirectBuffer::address should be guarded [v18]

[Per Minborg]

> 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 with a new target base due to a merge 
or a rebase. The pull request now contains 19 commits:

 - Merge master
 - Remove session exposure
 - Re-introduce yet another address vairable
 - Re-introduce address variables
 - Reformat and fix comment
 - Remove redundant method
 - Cleanup
 - Refactor to try-finally handling
 - Remove redundant guard and fix comment permanently
 - Rework Acquisition
 - ... and 9 more: https://git.openjdk.org/jdk/compare/a6139985...cd95bc86

-

Changes: https://git.openjdk.org/jdk/pull/11260/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=17
  Stats: 809 lines in 24 files changed: 377 ins; 172 del; 260 mod
  Patch: https://git.openjdk.org/jdk/pull/11260.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v17]

[Per Minborg]

On Tue, 29 Nov 2022 09:40:58 GMT, Per Minborg  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:
> 
>   Remove session exposure

I've converted this PR to a draft because we want to wait for the PR_20 branch 
being merged [JEP 434](https://openjdk.org/jeps/434)

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v17]

[Brian Burkhalter]

On Tue, 29 Nov 2022 09:40:58 GMT, Per Minborg  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:
> 
>   Remove session exposure

Marked as reviewed by bpb (Reviewer).

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v17]

[Paul Sandoz]

On Tue, 29 Nov 2022 09:40:58 GMT, Per Minborg  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:
> 
>   Remove session exposure

Marked as reviewed by psandoz (Reviewer).

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v17]

[Alan Bateman]

On Tue, 29 Nov 2022 09:40:58 GMT, Per Minborg  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:
> 
>   Remove session exposure

Marked as reviewed by alanb (Reviewer).

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v17]

[Maurizio Cimadamore]

On Tue, 29 Nov 2022 09:40:58 GMT, Per Minborg  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:
> 
>   Remove session exposure

This looks more symmetric, I like it. Since `acquire` was only really returning 
null if the underlying buffer had no associated session, I believe this 
rewriting preserves the semantics we had before (e.g. no accidental double 
releases).

-

Marked as reviewed by mcimadamore (Reviewer).

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v17]

[Per Minborg]

> 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:

  Remove session exposure

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/11260/files
  - new: https://git.openjdk.org/jdk/pull/11260/files/eedf41ae..619473ee

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=16
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=15-16

  Stats: 125 lines in 20 files changed: 11 ins; 13 del; 101 mod
  Patch: https://git.openjdk.org/jdk/pull/11260.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v16]

[Per Minborg]

On Mon, 28 Nov 2022 19:35:24 GMT, Paul Sandoz  wrote:

> 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.

I agree. A similar approach was done in a previous incarnation of the PR 
whereby we exposed a `ScopeAcqusition` that held an internal copy of the buffer 
and provided means to release. We feared that would cause performance 
regression. But your comment made me think of another way of doing 
acquire/release which would not expose the session and that would always be 
performant. I will explore this idea a bit more.

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v16]

[Paul Sandoz]

On Mon, 28 Nov 2022 10:47:47 GMT, Per Minborg  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

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v16]

[Maurizio Cimadamore]

On Mon, 21 Nov 2022 12:05:31 GMT, Alan Bateman  wrote:

>> The reason for the comment is to make it clear why `DirectBuffer::address` 
>> can be used directly without guarding. This will also reduce the probability 
>> of unnecessary guarding being added in the future. However, if the consensus 
>> is that these comments just adds confusion, I am happy to remove them.
>
> I'd prefer to see this comment removed from all places that are obviously 
> interacting with the direct buffer cache. These usages are try-finally to 
> acquire and return the temporary direct buffer cache back to the cache. 
> Talking about closable sessions here is definitely confusing.

> Thanks for persisting with it, latest version looks okay to me and the naming 
> issue can be sorted out after the JEP is integrated.


IMHO the renaming is not super important - the underlying abstraction managing 
the segment lifetime is still called MemorySession, even after 
https://git.openjdk.org/jdk/pull/10872. And, acquire/release are methods on 
MemorySession - so I think the current name might be more precise even after we 
integrate the latest API changes.

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v16]

[Alan Bateman]

On Mon, 28 Nov 2022 10:47:47 GMT, Per Minborg  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:
> 
>   Re-introduce yet another address vairable

Thanks for persisting with it, latest version looks okay to me and the naming 
issue can be sorted out after the JEP is integrated.

-

Marked as reviewed by alanb (Reviewer).

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v16]

[Per Minborg]

> 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:

  Re-introduce yet another address vairable

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/11260/files
  - new: https://git.openjdk.org/jdk/pull/11260/files/95c40581..eedf41ae

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=15
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=14-15

  Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/11260.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v15]

[Alan Bateman]

On Mon, 28 Nov 2022 10:36:40 GMT, Per Minborg  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:
> 
>   Re-introduce address variables

> The name was selected in anticipation of the proposed renaming in JEP 434. I 
> think once the JEP is merged, we can rename some of the methods (_Session_ -> 
> _Scope_) and then the user site naming would be much better. Another 
> alternative, which certainly can be done, is to rename `scope` to `session` 
> and then rename these back again when the JEP is merged. What is the 
> preferred way in your opinion?

Whatever is easiest. Given the fork for JDK 20 is coming up soon then it would 
be good to get the rename done soon after JEP 434 integrates, just to avoid JDK 
20 having the mix of names.

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v15]

[Per Minborg]

> 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:

  Re-introduce address variables

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/11260/files
  - new: https://git.openjdk.org/jdk/pull/11260/files/30aff118..95c40581

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=14
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=13-14

  Stats: 24 lines in 2 files changed: 11 ins; 0 del; 13 mod
  Patch: https://git.openjdk.org/jdk/pull/11260.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v14]

[Per Minborg]

On Thu, 24 Nov 2022 16:21:42 GMT, Per Minborg  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:
> 
>   Reformat and fix comment

> The latest version 
> ([30aff11](https://github.com/openjdk/jdk/commit/30aff1187d4978c59d1e6feebafc2d45aba3e5f2))
>  looks quite good but the naming at the use sites (`var scope = 
> NIO_ACCESS.acquireSession(buffer);`) is temporarily confusing. Will that be 
> fixed with the JEP 434 integrated or soon afterwards?

The name was selected in anticipation of the proposed renaming in JEP 434. I 
think once the JEP is merged, we can rename some of the methods (*Session* -> 
*Scope*) and then the user site naming would be much better. Another 
alternative, which certainly can be done, is to rename `scope` to `session` and 
then rename these back again when the JEP is merged. What is the preferred way 
in your opinion?

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v14]

[Alan Bateman]

On Thu, 24 Nov 2022 16:21:42 GMT, Per Minborg  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:
> 
>   Reformat and fix comment

src/java.base/share/classes/java/util/zip/Deflater.java line 594:

> 592: try {
> 593: result = deflateBufferBytes(zsRef.address(),
> 594: ((DirectBuffer) input).address() + 
> inputPos, inputRem,

Somewhat subjective but the original code, with inputAddress, was a easier to 
read and avoided having too much going on in the parameters to 
deflateBufferBytes. In any case, you probably should fix the formatting to 
avoid have different parameters with different indentations.

src/java.base/share/classes/java/util/zip/Deflater.java line 717:

> 715: result = deflateBytesBuffer(zsRef.address(),
> 716: inputArray, inputPos, inputLim - inputPos,
> 717: ((DirectBuffer) output).address() + 
> outputPos, outputRem,

Same here, I think I would keep outputAddress to keep the call to 
deflateByteBuffer simple.

src/java.base/share/classes/java/util/zip/Deflater.java line 740:

> 738: try {
> 739: result = 
> deflateBufferBuffer(zsRef.address(),
> 740: ((DirectBuffer) input).address() + 
> inputPos, inputRem,

Another one where inputAddress and outputAddress have been removed so the call 
to deflatBufferBuffer is much hard to digest.

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v14]

[Alan Bateman]

On Thu, 24 Nov 2022 16:21:42 GMT, Per Minborg  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:
> 
>   Reformat and fix comment

The latest version (30aff118) looks quite good but the naming at the use sites 
(`var scope = NIO_ACCESS.acquireSession(buffer);`) is temporarily confusing. 
Will that be fixed with the JEP  434 integrated or soon afterwards?

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v13]

[Alan Bateman]

On Fri, 25 Nov 2022 06:46:52 GMT, ExE Boss  wrote:

> I believe `UnsupportedOperationException` would be better for this.

It would but Per is right that this it is throwing ISE today, it's just that 
the PR adds an explicit check and that highlights that the wrong exception is 
thrown. Buffers that are views over memory that is thread confined can't be 
used with the Asynchronous APIs and this will needed to be specified. There 
is further work required in this area too because IOUtil was intended for 
synchronous I/O. The "async" support was added on a temporary basis and will 
need to be re-visited at some point.

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v13]

[ExE Boss]

On Thu, 24 Nov 2022 14:56:07 GMT, Per Minborg  wrote:

>> 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 Asynchronous read/write methods to decide 
>> which exception to specify.
>
> This was the old behaviour which was retained in this PR.

I believe `UnsupportedOperationException` would be better for this.

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v14]

[Per Minborg]

> 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:

  Reformat and fix comment

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/11260/files
  - new: https://git.openjdk.org/jdk/pull/11260/files/0546b23b..30aff118

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=13
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=12-13

  Stats: 26 lines in 3 files changed: 13 ins; 2 del; 11 mod
  Patch: https://git.openjdk.org/jdk/pull/11260.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v13]

[Per Minborg]

On Thu, 24 Nov 2022 12:06:44 GMT, Alan Bateman  wrote:

>> 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/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 Asynchronous read/write methods to decide which 
> exception to specify.

This was the old behaviour which was retained in this PR.

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v13]

[Per Minborg]

On Thu, 24 Nov 2022 11:58:12 GMT, Maurizio Cimadamore  
wrote:

>> Separately, also (optional) stylistic: the indenting on this and following 
>> class is a bit odd. There is a certain tendency to compress lines - e.g. to 
>> reach for one-liners, when in reality the body is not that trivial. If I 
>> were to write the code I'd insert a newline after the `{` and I'd also break 
>> apart initialization (e.g. no two statements separated by `;` in the same 
>> line).
>> 
>> Also, I noted that you start the body of the record (e.g. `{`) on a new 
>> line, which I also find odd.
>
> (to be clear, some of the above comments refer to the code that was already 
> there before your changes)

I also found the above as a bit odd but tried to stick with the existing 
implementation and style as much as possible.

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v13]

[Alan Bateman]

On Thu, 24 Nov 2022 11:42:52 GMT, Per Minborg  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 Asynchronous 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

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v13]

[Maurizio Cimadamore]

On Thu, 24 Nov 2022 11:42:52 GMT, Per Minborg  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/IOUtil.java line 519:

> 517: }
> 518: 
> 519: record LinkedRunnable(Runnable node, Runnable next)

Some (optional) style comments: I'm not sure this is a record. E.g. both this 
and the `Releaser` class are only used externally to call their `run` method. 
The fact that they are made up of components is immaterial to clients, which 
seems to suggest that an interface would be better - at least subjectively. If 
I were to write the code I will declare the implementation inside the 
factories, as local/anon classes.

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v13]

[Maurizio Cimadamore]

On Thu, 24 Nov 2022 11:57:33 GMT, Maurizio Cimadamore  
wrote:

>> src/java.base/share/classes/sun/nio/ch/IOUtil.java line 519:
>> 
>>> 517: }
>>> 518: 
>>> 519: record LinkedRunnable(Runnable node, Runnable next)
>> 
>> Some (optional) style comments: I'm not sure this is a record. E.g. both 
>> this and the `Releaser` class are only used externally to call their `run` 
>> method. The fact that they are made up of components is immaterial to 
>> clients, which seems to suggest that an interface would be better - at least 
>> subjectively. If I were to write the code I will declare the implementation 
>> inside the factories, as local/anon classes.
>
> Separately, also (optional) stylistic: the indenting on this and following 
> class is a bit odd. There is a certain tendency to compress lines - e.g. to 
> reach for one-liners, when in reality the body is not that trivial. If I were 
> to write the code I'd insert a newline after the `{` and I'd also break apart 
> initialization (e.g. no two statements separated by `;` in the same line).
> 
> Also, I noted that you start the body of the record (e.g. `{`) on a new line, 
> which I also find odd.

(to be clear, some of the above comments refer to the code that was already 
there before your changes)

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v13]

[Maurizio Cimadamore]

On Thu, 24 Nov 2022 11:54:43 GMT, Maurizio Cimadamore  
wrote:

>> 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/IOUtil.java line 519:
> 
>> 517: }
>> 518: 
>> 519: record LinkedRunnable(Runnable node, Runnable next)
> 
> Some (optional) style comments: I'm not sure this is a record. E.g. both this 
> and the `Releaser` class are only used externally to call their `run` method. 
> The fact that they are made up of components is immaterial to clients, which 
> seems to suggest that an interface would be better - at least subjectively. 
> If I were to write the code I will declare the implementation inside the 
> factories, as local/anon classes.

Separately, also (optional) stylistic: the indenting on this and following 
class is a bit odd. There is a certain tendency to compress lines - e.g. to 
reach for one-liners, when in reality the body is not that trivial. If I were 
to write the code I'd insert a newline after the `{` and I'd also break apart 
initialization (e.g. no two statements separated by `;` in the same line).

Also, I noted that you start the body of the record (e.g. `{`) on a new line, 
which I also find odd.

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v13]

[Maurizio Cimadamore]

On Thu, 24 Nov 2022 11:42:52 GMT, Per Minborg  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

Looks good - I'm glad that the changes to IOUtil turned out simpler than 
expected!

-

Marked as reviewed by mcimadamore (Reviewer).

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v13]

[Per Minborg]

> 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

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/11260/files
  - new: https://git.openjdk.org/jdk/pull/11260/files/682b6f5a..0546b23b

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=12
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=11-12

  Stats: 213 lines in 20 files changed: 12 ins; 105 del; 96 mod
  Patch: https://git.openjdk.org/jdk/pull/11260.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v12]

[Per Minborg]

On Wed, 23 Nov 2022 16:14:52 GMT, Maurizio Cimadamore  
wrote:

>> Per Minborg has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Cleanup
>
> src/java.base/share/classes/jdk/internal/access/JavaNioAccess.java line 125:
> 
>> 123:  * @see #releaseScope(Buffer, MemorySessionImpl)
>> 124:  */
>> 125: MemorySessionImpl acquireScopeOrNull(Buffer buffer);
> 
> I think this looks better - but naming-wise it's still a bit problematic. 
> This method really acquires the underlying session, not a scope. And also, 
> here we have `OrNull`, but the already existing `acquireSession` also has a 
> similar semantics w.r.t. null.
> 
> I suggest to rename:
> 
> * `acquireSession` -> `acquireSessionAsRunnable`
> * `acquireScopeOrNull` -> `acquireSession`
> * `releaseScope` -> `releaseSession`
> 
> Also, once we have `acquire/releaseSession`, it is not clear to me that we 
> still need `acquireSessionAsRunnable` in the JavaNIOAccess class - it seems 
> like you can create the Runnable where it's required (probably IOUtil), 
> simply by using straight acquire/release.

The name "scope" was used in anticipation of the new proposed Java 20 naming. 
But I can change it back to session again. We could always rename later.

> src/java.base/share/classes/jdk/internal/access/JavaNioAccess.java line 164:
> 
>> 162: int pageSize();
>> 163: 
>> 164: sealed interface ScopeAcquisition extends AutoCloseable {
> 
> Is this still needed?

Well spotted.

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v12]

[Maurizio Cimadamore]

On Wed, 23 Nov 2022 12:39:40 GMT, Per Minborg  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:
> 
>   Cleanup

src/java.base/share/classes/jdk/internal/access/JavaNioAccess.java line 125:

> 123:  * @see #releaseScope(Buffer, MemorySessionImpl)
> 124:  */
> 125: MemorySessionImpl acquireScopeOrNull(Buffer buffer);

I think this looks better - but naming-wise it's still a bit problematic. This 
method really acquires the underlying session, not a scope. And also, here we 
have `OrNull`, but the already existing `acquireSession` also has a similar 
semantics w.r.t. null.

I suggest to rename:

* `acquireSession` -> `acquireSessionAsRunnable`
* `acquireScopeOrNull` -> `acquireSession`
* `releaseScope` -> `releaseSession`

Also, once we have `acquire/releaseSession`, it is not clear to me that we 
still need `acquireSessionAsRunnable` in the JavaNIOAccess class - it seems 
like you can create the Runnable where it's required (probably IOUtil), simply 
by using straight acquire/release.

src/java.base/share/classes/jdk/internal/access/JavaNioAccess.java line 164:

> 162: int pageSize();
> 163: 
> 164: sealed interface ScopeAcquisition extends AutoCloseable {

Is this still needed?

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v12]

[Per Minborg]

> 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:

  Cleanup

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/11260/files
  - new: https://git.openjdk.org/jdk/pull/11260/files/88ae68b8..682b6f5a

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=11
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=10-11

  Stats: 23 lines in 9 files changed: 1 ins; 1 del; 21 mod
  Patch: https://git.openjdk.org/jdk/pull/11260.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v11]

[Per Minborg]

> 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:

  Refactor to try-finally handling

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/11260/files
  - new: https://git.openjdk.org/jdk/pull/11260/files/06c764ca..88ae68b8

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=10
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=09-10

  Stats: 677 lines in 20 files changed: 286 ins; 152 del; 239 mod
  Patch: https://git.openjdk.org/jdk/pull/11260.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v9]

[ExE Boss]

On Tue, 22 Nov 2022 13:49:45 GMT, Per Minborg  wrote:

>> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
>> line 914:
>> 
>>> 912:  * If so, make a copy to put the dst data in.
>>> 913:  */
>>> 914: @SuppressWarnings("try")
>> 
>> After looking at the implementation some more, I'm not sure this need 
>> fixing? E.g. this method is just using the address to compute some overlap - 
>> and return a buffer sliced accordingly. There's no access to the buffer data 
>> (except for the last part which does a `put`). The access will fail if the 
>> session is closed from underneath. I don't think this can crash the VM (in 
>> fact this code did not have a reachability fence to begin with).
>
> Well spotted. I will remove the guarding here.

This `@SuppressWarnings` annotation is no longer needed:
Suggestion:

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v10]

[Per Minborg]

> 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 guard and fix comment permanently

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/11260/files
  - new: https://git.openjdk.org/jdk/pull/11260/files/0526e3dc..06c764ca

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=09
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=08-09

  Stats: 59 lines in 2 files changed: 19 ins; 26 del; 14 mod
  Patch: https://git.openjdk.org/jdk/pull/11260.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v9]

[Per Minborg]

On Tue, 22 Nov 2022 09:23:40 GMT, Maurizio Cimadamore  
wrote:

>> Per Minborg has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Rework Acquisition
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
> line 914:
> 
>> 912:  * If so, make a copy to put the dst data in.
>> 913:  */
>> 914: @SuppressWarnings("try")
> 
> After looking at the implementation some more, I'm not sure this need fixing? 
> E.g. this method is just using the address to compute some overlap - and 
> return a buffer sliced accordingly. There's no access to the buffer data 
> (except for the last part which does a `put`). The access will fail if the 
> session is closed from underneath. I don't think this can crash the VM (in 
> fact this code did not have a reachability fence to begin with).

Well spotted. I will remove the guarding here.

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v9]

[Alan Bateman]

On Tue, 22 Nov 2022 09:29:14 GMT, Maurizio Cimadamore  
wrote:

>> Per Minborg has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Rework Acquisition
>
> src/java.base/share/classes/java/nio/Buffer.java line 827:
> 
>> 825: 
>> 826: @Override
>> 827: public Runnable acquireSessionOrNull(Buffer buffer, 
>> boolean async) {
> 
> If allocation/performance is an issue, a relatively straightforward way to 
> adjust the code would be to let go of the TWR entirely. E.g. we have code 
> that does this:
> 
> 
> Buffer b = ...
> try {
> // use buffer.address();
> } finally {
> Reference.reachabilityFence(b);
> }
> 
> 
> We could transform to this:
> 
> 
> Buffer b = ...
> acquire(b); // calls MemorySessionImpl.acquire0 if session is not null
> try {
> // use buffer.address();
> } finally {
> release(b); // calls MemorySessionImpl.release0 if session is not null 
> (and maybe adds a reachability fence, just in case)
> }
> 
> This leads to a simpler patch that is probably easier to validate. The 
> remaining IOUtil code will be using a different idiom, and perhaps we can, at 
> some point, go back and make that consistent too.

The AutoCloseable experiment was interesting to try but I think you are right, 
acquire try { .. } finally release would be simpler and also remove the 
concerns about the performance critical code paths.

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v9]

[Alan Bateman]

On Tue, 22 Nov 2022 09:38:35 GMT, Maurizio Cimadamore  
wrote:

>> src/jdk.sctp/unix/classes/sun/nio/ch/sctp/SctpMultiChannelImpl.java line 590:
>> 
>>> 588: int pos)
>>> 589: throws IOException {
>>> 590: try (var guard = NIO_ACCESS.acquireScope(bb)) {
>> 
>> Why was the old code not using reachability fences? Bug or feature?
>
> I see that there's a subsequent buffer call if `n > 0`, so that's probably 
> why the fence was skipped? (I also assume that the code calling this method 
> will access the buffer before/after, so reachability is never truly an issue 
> - but for session-backed buffers this needs fixing).
> 
> Also, stepping back, I note how, if `receive0` was a native call using 
> Linker, perhaps we wouldn't need all this manual address computation - we'd 
> just get a memory segment slice from the buffer and pass that to the handle 
> (which will perform the correct liveness check). E.g. maybe a better long 
> term solution would be to panama-ize this code?

Yes, once the memory/linker APIs are permanent then the SCTP implementation 
would be a good candidate to redo.

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v9]

[ExE Boss]

On Tue, 22 Nov 2022 09:11:44 GMT, Per Minborg  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:
> 
>   Rework Acquisition

src/java.base/share/classes/sun/nio/ch/DirectBuffer.java line 43:

> 41: // try (var guard = NIO_ACCESS.acquireSession(bb)) {
> 42: // performOperation(bb.address());
> 43: // }

Again, updating this comment was missed:
Suggestion:

// try (var guard = NIO_ACCESS.acquireScope(bb)) {
// performOperation(guard.address());
// }

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v9]

[Maurizio Cimadamore]

On Tue, 22 Nov 2022 09:32:32 GMT, Maurizio Cimadamore  
wrote:

>> Per Minborg has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Rework Acquisition
>
> src/jdk.sctp/unix/classes/sun/nio/ch/sctp/SctpMultiChannelImpl.java line 590:
> 
>> 588: int pos)
>> 589: throws IOException {
>> 590: try (var guard = NIO_ACCESS.acquireScope(bb)) {
> 
> Why was the old code not using reachability fences? Bug or feature?

I see that there's a subsequent buffer call if `n > 0`, so that's probably why 
the fence was skipped? (I also assume that the code calling this method will 
access the buffer before/after, so reachability is never truly an issue - but 
for session-backed buffers this needs fixing).

Also, stepping back, I note how, if `receive0` was a native call using Linker, 
perhaps we wouldn't need all this manual address computation - we'd just get a 
memory segment slice from the buffer and pass that to the handle (which will 
perform the correct liveness check). E.g. maybe a better long term solution 
would be to panama-ize this code?

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v9]

[Maurizio Cimadamore]

On Tue, 22 Nov 2022 09:11:44 GMT, Per Minborg  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:
> 
>   Rework Acquisition

src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 
914:

> 912:  * If so, make a copy to put the dst data in.
> 913:  */
> 914: @SuppressWarnings("try")

After looking at the implementation some more, I'm not sure this need fixing? 
E.g. this method is just using the address to compute some overlap - and return 
a buffer sliced accordingly. There's no access to the buffer data (except for 
the last part which does a `put`). The access will fail if the session is 
closed from underneath. I don't think this can crash the VM (in fact this code 
did not have a reachability fence to begin with).

src/java.base/share/classes/java/nio/Buffer.java line 827:

> 825: 
> 826: @Override
> 827: public Runnable acquireSessionOrNull(Buffer buffer, 
> boolean async) {

If allocation/performance is an issue, a relatively straightforward way to 
adjust the code would be to let go of the TWR entirely. E.g. we have code that 
does this:


Buffer b = ...
try {
// use buffer.address();
} finally {
Reference.reachabilityFence(b);
}


We could transform to this:


Buffer b = ...
acquire(b); // calls MemorySessionImpl.acquire0 if session is not null
try {
// use buffer.address();
} finally {
release(b); // calls MemorySessionImpl.release0 if session is not null (and 
maybe adds a reachability fence, just in case)
}

This leads to a simpler patch that is probably easier to validate. The 
remaining IOUtil code will be using a different idiom, and perhaps we can, at 
some point, go back and make that consistent too.

src/jdk.sctp/unix/classes/sun/nio/ch/sctp/SctpMultiChannelImpl.java line 590:

> 588: int pos)
> 589: throws IOException {
> 590: try (var guard = NIO_ACCESS.acquireScope(bb)) {

Why was the old code not using reachability fences? Bug or feature?

-

PR: https://git.openjdk.org/jdk/pull/112

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v9]

[Ismael Juma]

On Tue, 22 Nov 2022 09:11:44 GMT, Per Minborg  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:
> 
>   Rework Acquisition

> because this PR is touching several low level and performance critical code 
> paths

Indeed. Have we verified that performance doesn't regress with these changes?

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v6]

[Alan Bateman]

On Mon, 21 Nov 2022 15:29:11 GMT, Per Minborg  wrote:

> That is clear to me but I am trying to prevent future redundant guarding. 
> Anyway, I will remove the comments.

The faraway use sites look much better now. The performance sensitive usages 
need close attention. Can you summarise the changes to DatagramChannel, are you 
creating a NoOpScopeAcquisition for every  send/receive? This is low level code 
where we do do something different to avoid the try-with-resources.

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v4]

[Per Minborg]

On Mon, 21 Nov 2022 20:06:20 GMT, Alan Bateman  wrote:

>>> > Although this looks much simpler and concise, it means "a new object is 
>>> > created for each invocation"
>>> 
>>> My comment was actually to see if DirectBuffer could extend AutoCloseable 
>>> so that the acquire returns "this" for the non-session case. Doing the 
>>> equivalent for the session case might leak into MemorySessionImpl 
>>> implementing DirectBuffer which you probably don't want to do. If 
>>> NOP_CLOSE.close can do the Reference.reachabilityFence(this) then it would 
>>> at least improve some of the use-sites.
>> 
>> Not sure that is simpler. ByteBuffer <: AutoCloseable doesn't seem to make 
>> sense to me. I'm also not sure how much object allocation (all this stuff 
>> will become value types) should be the driving factor in these code paths.
>
> Right, I didn't mean BB <: AC but to see if we avoid needing to wrap it 
> because this PR is touching several low level and performance critical code 
> paths. For the faraway places then having the close do a 
> Reference.reachabilityFence(this) should avoid the surprise that the buffer 
> has to kept alive even though it appears that the try-with-resources is doing 
> it already.

I have reworked Acquisition. I think we could merge the old and new way in a 
separate PR.

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v9]

[Per Minborg]

> 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:

  Rework Acquisition

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/11260/files
  - new: https://git.openjdk.org/jdk/pull/11260/files/88ed3aa2..0526e3dc

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=08
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=07-08

  Stats: 249 lines in 19 files changed: 66 ins; 91 del; 92 mod
  Patch: https://git.openjdk.org/jdk/pull/11260.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v4]

[Alan Bateman]

On Mon, 21 Nov 2022 17:00:52 GMT, Maurizio Cimadamore  
wrote:

>>> Although this looks much simpler and concise, it means "a new object is 
>>> created for each invocation" 
>> 
>> My comment was actually to see if DirectBuffer could extend AutoCloseable so 
>> that the acquire returns "this" for the non-session case.  Doing the 
>> equivalent for the session case might leak into MemorySessionImpl 
>> implementing DirectBuffer which you probably don't want to do. If 
>> NOP_CLOSE.close can do the Reference.reachabilityFence(this) then it would 
>> at least improve some of the use-sites.
>
>> > Although this looks much simpler and concise, it means "a new object is 
>> > created for each invocation"
>> 
>> My comment was actually to see if DirectBuffer could extend AutoCloseable so 
>> that the acquire returns "this" for the non-session case. Doing the 
>> equivalent for the session case might leak into MemorySessionImpl 
>> implementing DirectBuffer which you probably don't want to do. If 
>> NOP_CLOSE.close can do the Reference.reachabilityFence(this) then it would 
>> at least improve some of the use-sites.
> 
> Not sure that is simpler. ByteBuffer <: AutoCloseable doesn't seem to make 
> sense to me. I'm also not sure how much object allocation (all this stuff 
> will become value types) should be the driving factor in these code paths.

Right, I didn't mean BB <: AC but to see if we avoid needing to wrap it because 
this PR is touching several low level and performance critical code paths. For 
the faraway places then having the close do a Reference.reachabilityFence(this) 
should avoid the surprise that the buffer has to kept alive even though it 
appears that the try-with-resources is doing it already.

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v4]

[Maurizio Cimadamore]

On Mon, 21 Nov 2022 16:34:46 GMT, Alan Bateman  wrote:

> > Although this looks much simpler and concise, it means "a new object is 
> > created for each invocation"
> 
> My comment was actually to see if DirectBuffer could extend AutoCloseable so 
> that the acquire returns "this" for the non-session case. Doing the 
> equivalent for the session case might leak into MemorySessionImpl 
> implementing DirectBuffer which you probably don't want to do. If 
> NOP_CLOSE.close can do the Reference.reachabilityFence(this) then it would at 
> least improve some of the use-sites.

Not sure that is simpler. ByteBuffer <: AutoCloseable doesn't seem to make 
sense to me. I'm also not sure how much object allocation (all this stuff will 
become value types) should be the driving factor in these code paths.

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v8]

[Maurizio Cimadamore]

On Mon, 21 Nov 2022 12:50:43 GMT, Per Minborg  wrote:

>> Would it be possible to re-examine acquireSession returning Runnable and 
>> acquireSessionAsAutoCloseable returning AutoCloseable. The naming is bit 
>> awkward at most of the use sites and maybe we can do better.
>
> I think it would be confusing to have overloads with the same name. 
> 
> The `aquireSession(Buffer, boolean)` method is only used once and is used in 
> `IOUtil` so we could rename it to `aquireSessionOrNull` and then rename 
> `acquireSessionAsAutoCloseable` to `acquireSession`.

IIRC, Runnable was chosen back then because of the issues with using TWR which 
was generating warnings (because resource not accessed in body of TWR). I don't 
have strong opinions on this, other than we should only have one way to do 
things, rather than 2-3. Since the JDK has capabilities to acquire and release 
a session w/o a TWR and/or Runnable, one might also consider whether exposing 
acquire/release separately, and just use those.

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v4]

[Alan Bateman]

On Mon, 21 Nov 2022 16:08:20 GMT, Per Minborg  wrote:

> Although this looks much simpler and concise, it means "a new object is 
> created for each invocation" 

My comment was actually to see if DirectBuffer could extend AutoCloseable so 
that the acquire returns "this" for the non-session case.  Doing the equivalent 
for the session case might leak into MemorySessionImpl implementing 
DirectBuffer which you probably don't want to do. If NOP_CLOSE.close can do the 
Reference.reachabilityFence(this) then it would at least improve some of the 
use-sites.

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v4]

[Per Minborg]

On Mon, 21 Nov 2022 14:07:38 GMT, Alan Bateman  wrote:

>> Per Minborg has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Rename methods
>
> src/java.base/share/classes/java/util/zip/Adler32.java line 105:
> 
>> 103: adler = updateByteBuffer(adler, 
>> ((DirectBuffer)buffer).address(), pos, rem);
>> 104: } finally {
>> 105: Reference.reachabilityFence(buffer);
> 
> The updated naming is a bit better but there are it still feels that that 
> there are too many things going on at the use sites ("guard", "session", 
> "reachability fence"). Ideally the acquire would return something with an 
> accessor for the direct buffer address but that may not be possible.  I 
> wonder if changing NOP_CLOSE.close to do reachabilityFence(this) would work 
> as expected and improve many of these use sites?

This can certainly be done. We could, for example, add a new method to the 
`JavaNioAccess` interface:

```AddressAcquisition acquireSession2(Buffer buffer); // to be renamed```

This would allow us to go from:


try (var guard = NIO_ACCESS.acquireSession(buffer)) {
adler = updateByteBuffer(adler, 
((DirectBuffer)buffer).address(), pos, rem);
} finally {
Reference.reachabilityFence(buffer);
}


to


try (var guard = NIO_ACCESS.acquireSession2(buffer)) {
adler = updateByteBuffer(adler, guard.address(), pos, rem);
}


Although this looks much simpler and concise, it means "a new object is created 
for each invocation" (*). This also allows us to remove the 
`@SupressWarnings("try")` annotations.

Here is how the undercarriage might look like:


@Override
public AddressAcquisition acquireSession2(Buffer buffer) {
var session = buffer.session();
if (session == null) {
return AddressAcquisition.create(buffer, () -> {});
}
session.acquire0();
return AddressAcquisition.create(buffer, session::release0);
}



and


sealed interface AddressAcquisition extends AutoCloseable {

long address();

@Override
void close();

static AddressAcquisition create(Buffer buffer, Runnable closeActions) {
return new AddressAcquisitionImpl(buffer, closeActions);
}

final class AddressAcquisitionImpl implements AddressAcquisition {

private final DirectBuffer directBuffer;
private final Runnable closeAction;

public AddressAcquisitionImpl(Buffer buffer,
  Runnable closeAction) {
this.directBuffer = (DirectBuffer) buffer;
this.closeAction = closeAction;
}

@Override
public long address() {
return directBuffer.address();
}

@Override
public void close() {
try {
closeAction.run();
} finally {
Reference.reachabilityFence(directBuffer);
}
}
}

}



(*) In reality,  a representation of the object might be allocated on the stack 
instead.

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v8]

[Per Minborg]

> 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 three additional 
commits since the last revision:

 - Merge branch 'guard-directbuffer-address' of https://github.com/minborg/jdk 
into guard-directbuffer-address
 - Update src/java.base/share/classes/jdk/internal/access/JavaNioAccess.java
   
   Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
 - Remove comments

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/11260/files
  - new: https://git.openjdk.org/jdk/pull/11260/files/17a72e9f..88ed3aa2

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=07
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=06-07

  Stats: 5 lines in 3 files changed: 0 ins; 4 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/11260.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v6]

[Per Minborg]

On Mon, 21 Nov 2022 14:14:14 GMT, Alan Bateman  wrote:

>> Per Minborg has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - Update src/java.base/share/classes/jdk/internal/access/JavaNioAccess.java
>>
>>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
>>  - Update src/java.base/share/classes/sun/nio/ch/DirectBuffer.java
>>
>>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
>
> src/java.base/share/classes/sun/nio/fs/NativeBuffers.java line 92:
> 
>> 90:  * allocated from the heap.
>> 91:  * 
>> 92:  * The returned NativeBuffer is guaranteed not to be asynchronously 
>> closeable.
> 
> This class, and the temporary buffer cache in sun.nio.ch.Util, are intended 
> to be used with try-finally. There isn't any notion of  asynchronously close 
> so maybe it would best to not add this comment to these sources.

That is clear to me but I am trying to prevent future redundant guarding. 
Anyway, I will remove the comments.

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v6]

[Alan Bateman]

On Mon, 21 Nov 2022 14:02:38 GMT, Per Minborg  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 two additional 
> commits since the last revision:
> 
>  - Update src/java.base/share/classes/jdk/internal/access/JavaNioAccess.java
>
>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
>  - Update src/java.base/share/classes/sun/nio/ch/DirectBuffer.java
>
>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>

src/java.base/share/classes/sun/nio/fs/NativeBuffers.java line 92:

> 90:  * allocated from the heap.
> 91:  * 
> 92:  * The returned NativeBuffer is guaranteed not to be asynchronously 
> closeable.

This class, and the temporary buffer cache in sun.nio.ch.Util, are intended to 
be used with try-finally. There isn't any notion of  asynchronously close so 
maybe it would best to not add this comment to these sources.

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v4]

[Alan Bateman]

On Mon, 21 Nov 2022 13:02:48 GMT, Per Minborg  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:
> 
>   Rename methods

src/java.base/share/classes/java/util/zip/Adler32.java line 105:

> 103: adler = updateByteBuffer(adler, 
> ((DirectBuffer)buffer).address(), pos, rem);
> 104: } finally {
> 105: Reference.reachabilityFence(buffer);

The updated naming is a bit better but there are it still feels that that there 
are too many things going on at the use sites ("guard", "session", 
"reachability fence"). Ideally the acquire would return something with an 
accessor for the direct buffer address but that may not be possible.  I wonder 
if changing NOP_CLOSE.close to do reachabilityFence(this) would work as 
expected and improve many of these use sites?

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v7]

[Per Minborg]

> 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 refreshed the contents of this pull request, and previous 
commits have been removed. Incremental views are not available. The pull 
request now contains six commits:

 - Update src/java.base/share/classes/sun/nio/ch/DirectBuffer.java
   
   Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
 - Cleanup static declarations
 - Rename methods
 - Remove comments
 - Cleanup after comments
 - Guard usages of DirectBuffer::address

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/11260/files
  - new: https://git.openjdk.org/jdk/pull/11260/files/a0de3bcf..17a72e9f

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=06
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=05-06

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/11260.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v6]

[Per Minborg]

> 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 two additional 
commits since the last revision:

 - Update src/java.base/share/classes/jdk/internal/access/JavaNioAccess.java
   
   Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
 - Update src/java.base/share/classes/sun/nio/ch/DirectBuffer.java
   
   Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/11260/files
  - new: https://git.openjdk.org/jdk/pull/11260/files/eca7c388..a0de3bcf

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=05
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=04-05

  Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/11260.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v4]

[ExE Boss]

On Mon, 21 Nov 2022 13:43:56 GMT, Per Minborg  wrote:

>> src/java.base/share/classes/sun/nio/ch/DirectBuffer.java line 41:
>> 
>>> 39: // An example of a guarded use of a memory address is shown here:
>>> 40: //
>>> 41: // try (var guard = NIO_ACCESS.acquireSessionAsAutoCloseable(bb)) {
>> 
>> Suggestion:
>> 
>> // try (var guard = NIO_ACCESS.acquireSession(bb)) {
>
> Are you happy with the proposed renaming now @ExE-Boss ? I think our 
> propositions crossed in time?

This change was missed in commit 
.

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v4]

[Per Minborg]

On Mon, 21 Nov 2022 13:32:43 GMT, ExE Boss  wrote:

>> Per Minborg has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Rename methods
>
> src/java.base/share/classes/sun/nio/ch/DirectBuffer.java line 41:
> 
>> 39: // An example of a guarded use of a memory address is shown here:
>> 40: //
>> 41: // try (var guard = NIO_ACCESS.acquireSessionAsAutoCloseable(bb)) {
> 
> Suggestion:
> 
> // try (var guard = NIO_ACCESS.acquireSession(bb)) {

Are you happy with the proposed renaming now @ExE-Boss ? I think our 
propositions crossed in time?

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v5]

[Per Minborg]

> 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:

  Cleanup static declarations

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/11260/files
  - new: https://git.openjdk.org/jdk/pull/11260/files/c081b4ae..eca7c388

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=04
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=03-04

  Stats: 4 lines in 4 files changed: 0 ins; 0 del; 4 mod
  Patch: https://git.openjdk.org/jdk/pull/11260.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v4]

[ExE Boss]

On Mon, 21 Nov 2022 13:02:48 GMT, Per Minborg  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:
> 
>   Rename methods

src/java.base/share/classes/jdk/internal/access/JavaNioAccess.java line 94:

> 92:  * required to guarantee safety.
> 93:  * {@snippet lang = java:
> 94:  * var handler = acquireSessionOrNoOp(buffer);

Suggestion:

 * var handler = acquireSessionOrNull(buffer, async);

src/java.base/share/classes/sun/nio/ch/DirectBuffer.java line 41:

> 39: // An example of a guarded use of a memory address is shown here:
> 40: //
> 41: // try (var guard = NIO_ACCESS.acquireSessionAsAutoCloseable(bb)) {

Suggestion:

// try (var guard = NIO_ACCESS.acquireSession(bb)) {

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v4]

[Per Minborg]

> 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:

  Rename methods

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/11260/files
  - new: https://git.openjdk.org/jdk/pull/11260/files/9fcf2bb3..c081b4ae

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=03
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=02-03

  Stats: 51 lines in 20 files changed: 0 ins; 6 del; 45 mod
  Patch: https://git.openjdk.org/jdk/pull/11260.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v3]

[Per Minborg]

On Mon, 21 Nov 2022 12:19:21 GMT, Alan Bateman  wrote:

>> maybe just `bufferLock` and and just `acquireBuffer` ?
>
> Would it be possible to re-examine acquireSession returning Runnable and 
> acquireSessionAsAutoCloseable returning AutoCloseable. The naming is bit 
> awkward at most of the use sites and maybe we can do better.

I think it would be confusing to have overloads with the same name. 

The `aquireSession(Buffer, boolean)` method is only used once and is used in 
`IOUtil` so we could rename it to `aquireSessionOrNull` and then rename 
`acquireSessionAsAutoCloseable` to `acquireSession`.

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v3]

[Per Minborg]

> 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 comments

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/11260/files
  - new: https://git.openjdk.org/jdk/pull/11260/files/9fafafac..9fcf2bb3

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=02
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=01-02

  Stats: 18 lines in 6 files changed: 4 ins; 14 del; 0 mod
  Patch: https://git.openjdk.org/jdk/pull/11260.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v2]

[Maurizio Cimadamore]

On Mon, 21 Nov 2022 12:03:35 GMT, Per Minborg  wrote:

>> src/java.base/share/classes/java/util/zip/Adler32.java line 102:
>> 
>>> 100: return;
>>> 101: if (buffer.isDirect()) {
>>> 102: try (var sessionAcquisition = 
>>> NIO_ACCESS.acquireSessionAsAutoCloseable(buffer)) {
>> 
>> We need to find something better than "sessionAcquisition", it looks very 
>> messy at all these use sites.
>
> Eventually, I hope most of them will be named `_`.

maybe just `bufferLock` and and just `acquireBuffer` ?

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v2]

[Per Minborg]

> 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:

  Cleanup after comments

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/11260/files
  - new: https://git.openjdk.org/jdk/pull/11260/files/23a6fd14..9fafafac

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=01
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=00-01

  Stats: 50 lines in 20 files changed: 6 ins; 0 del; 44 mod
  Patch: https://git.openjdk.org/jdk/pull/11260.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v2]

[Alan Bateman]

On Mon, 21 Nov 2022 12:16:01 GMT, Maurizio Cimadamore  
wrote:

>> Eventually, I hope most of them will be named `_`.
>
> maybe just `bufferLock` and and just `acquireBuffer` ?

Would it be possible to re-examine acquireSession returning Runnable and 
acquireSessionAsAutoCloseable returning AutoCloseable. The naming is awkward at 
most of the use sites and maybe we can do better.

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded

[Alan Bateman]

On Mon, 21 Nov 2022 11:36:44 GMT, Per Minborg  wrote:

>> 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?
>
> The reason for the comment is to make it clear why `DirectBuffer::address` 
> can be used directly without guarding. This will also reduce the probability 
> of unnecessary guarding being added in the future. However, if the consensus 
> is that these comments just adds confusion, I am happy to remove them.

I'd prefer to see this comment removed from all places that are obviously 
interacting with the direct buffer cache. These usages are try-finally to 
acquire and return the temporary direct buffer cache back to the cache. Talking 
about closable sessions here is definitely confusing.

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded

[Per Minborg]

On Mon, 21 Nov 2022 11:32:35 GMT, Alan Bateman  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/util/zip/Adler32.java line 102:
> 
>> 100: return;
>> 101: if (buffer.isDirect()) {
>> 102: try (var sessionAcquisition = 
>> NIO_ACCESS.acquireSessionAsAutoCloseable(buffer)) {
> 
> We need to find something better than "sessionAcquisition", it looks very 
> messy at all these use sites.

Eventually, I hope most of them will be named `_`.

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded

[Per Minborg]

On Mon, 21 Nov 2022 11:29:07 GMT, Alan Bateman  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/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?

The reason for the comment is to make it clear why `DirectBuffer::address` can 
be used directly without guarding. This will also reduce the probability of 
unnecessary guarding being added in the future. However, if the consensus is 
that these comments just adds confusion, I am happy to remove them.

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded

[Alan Bateman]

On Mon, 21 Nov 2022 10:53:07 GMT, Per Minborg  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/util/zip/Adler32.java line 102:

> 100: return;
> 101: if (buffer.isDirect()) {
> 102: try (var sessionAcquisition = 
> NIO_ACCESS.acquireSessionAsAutoCloseable(buffer)) {

We need to find something better than "sessionAcquisition", it looks very messy 
at all these use sites.

-

PR: https://git.openjdk.org/jdk/pull/11260

Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded

[Alan Bateman]

On Mon, 21 Nov 2022 10:53:07 GMT, Per Minborg  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

RFR: 8296024: Usage of DIrectBuffer::address should be guarded

[Per Minborg]

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.

-

Commit messages:
 - Guard usages of DirectBuffer::address

Changes: https://git.openjdk.org/jdk/pull/11260/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=11260&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8296024
  Stats: 277 lines in 26 files changed: 191 ins; 4 del; 82 mod
  Patch: https://git.openjdk.org/jdk/pull/11260.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/11260/head:pull/11260

PR: https://git.openjdk.org/jdk/pull/11260