RFR: 8222884: ConcurrentClassDescLookup.java times out intermittently

[Jaikiran Pai]

Can I please get a review of this test-only change which proposes to address 
intermittent failures in 
`test/jdk/java/io/Serializable/concurrentClassDescLookup/ConcurrentClassDescLookup.java`?

This addresses https://bugs.openjdk.org/browse/JDK-8222884. As noted in that 
issue, the test in its current form has a potential race. The test main thread 
launches 50 "successful lookup" tasks and 50 "failed lookup" tasks. Each task 
is a `Thread`. The test passes an `Object` instance shared by 50 of these tasks 
and each task when it starts execution in its `run()` method does a `wait()` on 
that `Object` instance. The test main thread then after launching 50 tasks, 
sleeps for a second (to let the 50 `Thread`s start execution and arrive at the 
`Object.wait()`) and then after waking up the main thread does a `notifyAll()` 
on that `Object` instance. In theory and likely in practice too, it's possible 
that (especially) for the last batch of 50 tasks (the "failing lookup" tasks), 
the `notifyAll()` might end up getting invoked before one or more task 
Thread(s) have actually done a `wait()`. That can then mean that one or more 
task `Thread`(s) will keep `wait()`ing indefinitely with no one 
 `notify()`ing the `Object` instance to proceed with the task execution.

The commit in this PR removes the use of `Object.wait()/notifyAll()` and 
instead uses a `CountdownLatch` to allow for a more deterministic barrier. The 
test continues to pass with this change. Additionally, this change was run 
against a CI setup, by Matthias, where it was previously failing 
intermittently. Matthias reports that it hasn't failed after this change.

-

Commit messages:
 - 8222884: ConcurrentClassDescLookup.java times out intermittently

Changes: https://git.openjdk.org/jdk/pull/19659/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk=19659=00
  Issue: https://bugs.openjdk.org/browse/JDK-8222884
  Stats: 36 lines in 1 file changed: 7 ins; 7 del; 22 mod
  Patch: https://git.openjdk.org/jdk/pull/19659.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19659/head:pull/19659

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

Re: RFR: 8173970: jar tool should have a way to extract to a directory [v7]

[Jaikiran Pai]

> Can I please get a review for this patch which proposes to implement the 
> enhancement request noted in https://bugs.openjdk.java.net/browse/JDK-8173970?
> 
> The commit in this PR introduces the `-o` and `--output-dir` option to the 
> `jar` command. The option takes a path to a destination directory as a value 
> and extracts the contents of the jar into that directory. This is an optional 
> option and the changes in the commit continue to maintain backward 
> compatibility where the jar is extracted into current directory, if no `-o` 
> or `--output-dir` option has been specified.
> 
> As far as I know, there hasn't been any discussion on what the name of this 
> new option should be. I was initially thinking of using `-d` but that is 
> currently used by the `jar` command for a different purpose. So I decided to 
> use `-o` and `--output-dir`. This is of course open for change depending on 
> any suggestions in this PR.
> 
> The commit in this PR also updates the `jar.properties` file which contains 
> the English version of the jar command's `--help` output. However, no changes 
> have been done to the internationalization files corresponding to this one 
> (for example: `jar_de.properties`), because I don't know what process needs 
> to be followed to have those files updated (if at all they need to be 
> updated).
> 
> The commit also includes a jtreg testcase which verifies the usage of this 
> new option.

Jaikiran Pai has updated the pull request with a new target base due to a merge 
or a rebase. The pull request now contains 17 commits:

 - merge latest from master branch
 - merge latest from master branch
 - cleanup testExtractNoDestDirWithPFlag() test
 - merge latest from master branch
 - merge latest from master branch
 - convert the new test to junit
 - merge latest from master branch
 - Lance's review - include tests for --extract long form option
 - cleanup after each test
 - Adjust test for new error messages
 - ... and 7 more: https://git.openjdk.org/jdk/compare/7b43a8cd...2bd13ece

-

Changes: https://git.openjdk.org/jdk/pull/2752/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk=2752=06
  Stats: 569 lines in 4 files changed: 558 ins; 0 del; 11 mod
  Patch: https://git.openjdk.org/jdk/pull/2752.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/2752/head:pull/2752

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

Re: RFR: 8026127: Deflater/Inflater documentation incomplete/misleading [v3]

[Jaikiran Pai]

On Thu, 6 Jun 2024 14:02:03 GMT, Jaikiran Pai  wrote:

>> Can I please get a review of this doc-only change which proposes to improve 
>> the code snippet that's in `java.util.zip.Deflater` and 
>> `java.util.zip.Inflater` to better explain the usage of those classes? This 
>> addresses https://bugs.openjdk.org/browse/JDK-8026127.
>> 
>> The commit in the PR cleans up the snippet to correctly compress/decompress 
>> till the `Deflater` and `Inflater` are `finished()`.  Additionally, the 
>> snippet also shows that the `Deflater` and `Inflater` are expected to be 
>> closed when their usage it done, to release the resources held by those 
>> instances.
>> 
>> I've run `make docs-image` locally to verify that the generated snippet 
>> content as well as the link from `Inflater` work fine in the rendered 
>> javadoc HTML.
>
> Jaikiran Pai has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - minor change to the comment
>  - move the snippet to an external snippet

Thank you Lance for the review.

-

PR Comment: https://git.openjdk.org/jdk/pull/19507#issuecomment-2153639493

Integrated: 8026127: Deflater/Inflater documentation incomplete/misleading

[Jaikiran Pai]

On Sat, 1 Jun 2024 04:33:57 GMT, Jaikiran Pai  wrote:

> Can I please get a review of this doc-only change which proposes to improve 
> the code snippet that's in `java.util.zip.Deflater` and 
> `java.util.zip.Inflater` to better explain the usage of those classes? This 
> addresses https://bugs.openjdk.org/browse/JDK-8026127.
> 
> The commit in the PR cleans up the snippet to correctly compress/decompress 
> till the `Deflater` and `Inflater` are `finished()`.  Additionally, the 
> snippet also shows that the `Deflater` and `Inflater` are expected to be 
> closed when their usage it done, to release the resources held by those 
> instances.
> 
> I've run `make docs-image` locally to verify that the generated snippet 
> content as well as the link from `Inflater` work fine in the rendered javadoc 
> HTML.

This pull request has now been integrated.

Changeset: d8af5894
Author:Jaikiran Pai 
URL:   
https://git.openjdk.org/jdk/commit/d8af58941b5dedb9774c0971895c4924e57ac28b
Stats: 155 lines in 3 files changed: 96 ins; 57 del; 2 mod

8026127: Deflater/Inflater documentation incomplete/misleading

Reviewed-by: lancea

-

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

Re: RFR: 8026127: Deflater/Inflater documentation incomplete/misleading [v3]

[Jaikiran Pai]

> Can I please get a review of this doc-only change which proposes to improve 
> the code snippet that's in `java.util.zip.Deflater` and 
> `java.util.zip.Inflater` to better explain the usage of those classes? This 
> addresses https://bugs.openjdk.org/browse/JDK-8026127.
> 
> The commit in the PR cleans up the snippet to correctly compress/decompress 
> till the `Deflater` and `Inflater` are `finished()`.  Additionally, the 
> snippet also shows that the `Deflater` and `Inflater` are expected to be 
> closed when their usage it done, to release the resources held by those 
> instances.
> 
> I've run `make docs-image` locally to verify that the generated snippet 
> content as well as the link from `Inflater` work fine in the rendered javadoc 
> HTML.

Jaikiran Pai has updated the pull request incrementally with two additional 
commits since the last revision:

 - minor change to the comment
 - move the snippet to an external snippet

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19507/files
  - new: https://git.openjdk.org/jdk/pull/19507/files/098212a6..7a72a760

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=19507=02
 - incr: https://webrevs.openjdk.org/?repo=jdk=19507=01-02

  Stats: 158 lines in 3 files changed: 97 ins; 58 del; 3 mod
  Patch: https://git.openjdk.org/jdk/pull/19507.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19507/head:pull/19507

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

Re: RFR: 8333714: Cleanup the usages of CHECK_EXCEPTION_NULL_FAIL macro in java launcher [v2]

[Jaikiran Pai]

> Can I please get a review for this change which proposes to remove the 
> `CHECK_EXCEPTION_NULL_FAIL` macro from the `java` launcher code?
> 
> This addresses https://bugs.openjdk.org/browse/JDK-8333714. As noted in that 
> JBS issue, in a recent PR discussion, it was suggested 
> https://github.com/openjdk/jdk/pull/18786#issuecomment-2147452633 that this 
> macro should be removed and the failure of a JNI specified operation (the 
> ones for which this macro is being used) should be determined based on a 
> `NULL` value returned from that function. The commit in this PR removes this 
> macros and updates the call sites to do a `NULL` check.
> 
> Given the nature of this change, no new tests have been added. tier1, tier2 
> and tier3 testing passed successfully with these changes.

Jaikiran Pai has updated the pull request incrementally with one additional 
commit since the last revision:

  improve comments

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19576/files
  - new: https://git.openjdk.org/jdk/pull/19576/files/1c2a3dfb..1b3d630a

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=19576=01
 - incr: https://webrevs.openjdk.org/?repo=jdk=19576=00-01

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

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

RFR: 8333714: Cleanup the usages of CHECK_EXCEPTION_NULL_FAIL macro in java launcher

[Jaikiran Pai]

Can I please get a review for this change which proposes to remove the 
`CHECK_EXCEPTION_NULL_FAIL` macro from the `java` launcher code?

This addresses https://bugs.openjdk.org/browse/JDK-8333714. As noted in that 
JBS issue, in a recent PR discussion, it was suggested 
https://github.com/openjdk/jdk/pull/18786#issuecomment-2147452633 that this 
macro should be removed and the failure of a JNI specified operation (the ones 
for which this macro is being used) should be determined based on a `NULL` 
value returned from that function. The commit in this PR removes this macros 
and updates the call sites to do a `NULL` check.

Given the nature of this change, no new tests have been added. tier1, tier2 and 
tier3 testing passed successfully with these changes.

-

Commit messages:
 - simplify function comments
 - 8333714: Cleanup the usages of CHECK_EXCEPTION_NULL_FAIL macro in java 
launcher

Changes: https://git.openjdk.org/jdk/pull/19576/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk=19576=00
  Issue: https://bugs.openjdk.org/browse/JDK-8333714
  Stats: 76 lines in 1 file changed: 45 ins; 9 del; 22 mod
  Patch: https://git.openjdk.org/jdk/pull/19576.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19576/head:pull/19576

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

Re: RFR: 8026127: Deflater/Inflater documentation incomplete/misleading [v2]

[Jaikiran Pai]

> Can I please get a review of this doc-only change which proposes to improve 
> the code snippet that's in `java.util.zip.Deflater` and 
> `java.util.zip.Inflater` to better explain the usage of those classes? This 
> addresses https://bugs.openjdk.org/browse/JDK-8026127.
> 
> The commit in the PR cleans up the snippet to correctly compress/decompress 
> till the `Deflater` and `Inflater` are `finished()`.  Additionally, the 
> snippet also shows that the `Deflater` and `Inflater` are expected to be 
> closed when their usage it done, to release the resources held by those 
> instances.
> 
> I've run `make docs-image` locally to verify that the generated snippet 
> content as well as the link from `Inflater` work fine in the rendered javadoc 
> HTML.

Jaikiran Pai has updated the pull request with a new target base due to a merge 
or a rebase. The incremental webrev excludes the unrelated changes brought in 
by the merge/rebase. The pull request contains two additional commits since the 
last revision:

 - merge latest from master branch
 - 8026127: Deflater/Inflater documentation incomplete/misleading

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19507/files
  - new: https://git.openjdk.org/jdk/pull/19507/files/d8d86bcb..098212a6

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=19507=01
 - incr: https://webrevs.openjdk.org/?repo=jdk=19507=00-01

  Stats: 19514 lines in 482 files changed: 12009 ins; 5593 del; 1912 mod
  Patch: https://git.openjdk.org/jdk/pull/19507.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19507/head:pull/19507

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

Re: RFR: 8326227: Rounding error that may distort computeNextGaussian results [v3]

[Jaikiran Pai]

On Thu, 9 May 2024 03:55:15 GMT, Chris Hennick  wrote:

>> Chris Hennick has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Bug fix: add-exports was for wrong package
>
> Keep open. 
> 
> @JimLaskey it looks like you're the author of most of the surrounding code; 
> can you please confirm that there's a rounding error in the existing 
> implementation and that this PR will fix it? Or if the `git blame` logs are 
> giving you more credit than you're willing to take, then could you please 
> assign it to one ot the real authors or, if that's not possible, to someone 
> else who's in a relevant role and can capable of reviewing and eventually 
> merging this PR?

Hello Chris @Pr0methean, please keep this open. Some of us are checking if we 
can find someone experienced with this code to provide reviews.

-

PR Comment: https://git.openjdk.org/jdk/pull/17703#issuecomment-2151485139

Integrated: 8206447: InflaterInputStream.skip receives long but it's limited to Integer.MAX_VALUE

[Jaikiran Pai]

On Mon, 3 Jun 2024 05:06:00 GMT, Jaikiran Pai  wrote:

> Can I please get a review of this change which updates the API specification 
> of `java.util.zip.InflaterInputStream.skip()` method to match its current 
> implementation?
> 
> `InflaterInputStream.skip()`, just like `DeflaterInputStream.skip()`, takes a 
> `long` value `n` representing the number of bytes to skip. When a value 
> greater than `Integer.MAX_VALUE` is passed to this 
> `InflaterInputStream.skip()` method, the current implementation in that 
> method only skips at most `Integer.MAX_VALUE` bytes. 
> `DeflaterInputStream.skip()` too has the same behaviour. However, in the case 
> of `DeflaterInputStream.skip()`, this semantic is clearly noted in that 
> method's API documentation. `InflaterInputStream.skip()` currently doesn't 
> specify this behaviour.
> 
> The commit in this PR updates the `InflaterInputStream.skip()` to specify the 
> current implementation.
> 
> I'll open a CSR for this change.

This pull request has now been integrated.

Changeset: d7d1afb0
Author:Jaikiran Pai 
URL:   
https://git.openjdk.org/jdk/commit/d7d1afb0a84e771870e9f43e08c4a63c8fdccdd9
Stats: 21 lines in 2 files changed: 11 ins; 2 del; 8 mod

8206447: InflaterInputStream.skip receives long but it's limited to 
Integer.MAX_VALUE

Reviewed-by: lancea, alanb

-

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

Re: RFR: 8206447: InflaterInputStream.skip receives long but it's limited to Integer.MAX_VALUE [v4]

[Jaikiran Pai]

On Wed, 5 Jun 2024 12:14:07 GMT, Jaikiran Pai  wrote:

>> Can I please get a review of this change which updates the API specification 
>> of `java.util.zip.InflaterInputStream.skip()` method to match its current 
>> implementation?
>> 
>> `InflaterInputStream.skip()`, just like `DeflaterInputStream.skip()`, takes 
>> a `long` value `n` representing the number of bytes to skip. When a value 
>> greater than `Integer.MAX_VALUE` is passed to this 
>> `InflaterInputStream.skip()` method, the current implementation in that 
>> method only skips at most `Integer.MAX_VALUE` bytes. 
>> `DeflaterInputStream.skip()` too has the same behaviour. However, in the 
>> case of `DeflaterInputStream.skip()`, this semantic is clearly noted in that 
>> method's API documentation. `InflaterInputStream.skip()` currently doesn't 
>> specify this behaviour.
>> 
>> The commit in this PR updates the `InflaterInputStream.skip()` to specify 
>> the current implementation.
>> 
>> I'll open a CSR for this change.
>
> Jaikiran Pai has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   clarify blocking semantic

Thank you all for the reviews and the inputs to the specification text. The CSR 
has been approved, so I'll integrate this now.

-

PR Comment: https://git.openjdk.org/jdk/pull/19515#issuecomment-2150413847

Re: RFR: 8206447: InflaterInputStream.skip receives long but it's limited to Integer.MAX_VALUE [v4]

[Jaikiran Pai]

> Can I please get a review of this change which updates the API specification 
> of `java.util.zip.InflaterInputStream.skip()` method to match its current 
> implementation?
> 
> `InflaterInputStream.skip()`, just like `DeflaterInputStream.skip()`, takes a 
> `long` value `n` representing the number of bytes to skip. When a value 
> greater than `Integer.MAX_VALUE` is passed to this 
> `InflaterInputStream.skip()` method, the current implementation in that 
> method only skips at most `Integer.MAX_VALUE` bytes. 
> `DeflaterInputStream.skip()` too has the same behaviour. However, in the case 
> of `DeflaterInputStream.skip()`, this semantic is clearly noted in that 
> method's API documentation. `InflaterInputStream.skip()` currently doesn't 
> specify this behaviour.
> 
> The commit in this PR updates the `InflaterInputStream.skip()` to specify the 
> current implementation.
> 
> I'll open a CSR for this change.

Jaikiran Pai has updated the pull request incrementally with one additional 
commit since the last revision:

  clarify blocking semantic

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19515/files
  - new: https://git.openjdk.org/jdk/pull/19515/files/a43e96de..b7b870d5

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=19515=03
 - incr: https://webrevs.openjdk.org/?repo=jdk=19515=02-03

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

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

Re: RFR: 8206447: InflaterInputStream.skip receives long but it's limited to Integer.MAX_VALUE [v3]

[Jaikiran Pai]

> Can I please get a review of this change which updates the API specification 
> of `java.util.zip.InflaterInputStream.skip()` method to match its current 
> implementation?
> 
> `InflaterInputStream.skip()`, just like `DeflaterInputStream.skip()`, takes a 
> `long` value `n` representing the number of bytes to skip. When a value 
> greater than `Integer.MAX_VALUE` is passed to this 
> `InflaterInputStream.skip()` method, the current implementation in that 
> method only skips at most `Integer.MAX_VALUE` bytes. 
> `DeflaterInputStream.skip()` too has the same behaviour. However, in the case 
> of `DeflaterInputStream.skip()`, this semantic is clearly noted in that 
> method's API documentation. `InflaterInputStream.skip()` currently doesn't 
> specify this behaviour.
> 
> The commit in this PR updates the `InflaterInputStream.skip()` to specify the 
> current implementation.
> 
> I'll open a CSR for this change.

Jaikiran Pai has updated the pull request incrementally with one additional 
commit since the last revision:

  review feedback updates from the CSR

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19515/files
  - new: https://git.openjdk.org/jdk/pull/19515/files/e13e65d2..a43e96de

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=19515=02
 - incr: https://webrevs.openjdk.org/?repo=jdk=19515=01-02

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

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

Re: RFR: 8329581: Java launcher no longer prints a stack trace [v10]

[Jaikiran Pai]

On Fri, 31 May 2024 14:34:16 GMT, Sonia Zaldana Calles  
wrote:

>> Sonia Zaldana Calles has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Decreasing diff size addressing unnecessary changes
>
> Hi all,  
> 
> I think there's some consensus that we need some follow up cleanup issues for 
> the JNI spec, renaming constants, fixing return codes, etc. 
> 
> Seeing how that grows the scope of the issue quite a bit, I'd like to push 
> this patch and track the other issues brought up separately. 
> 
> If there are no objections about the current state, I'd like to integrate 
> some time next week. Let me know your thoughts.
> 
> cc: @jaikiran, @AlanBateman

@SoniaZaldana CI testing of this PR along with latest master commits passed 
without any failures. So you can now integrate this.

-

PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2148719775

Re: RFR: 8333270: HandlersOnComplexResetUpdate and HandlersOnComplexUpdate tests fail with "Unexpected reference" if timeoutFactor is less than 1/3 [v2]

[Jaikiran Pai]

On Tue, 4 Jun 2024 15:17:33 GMT, Daniel Fuchs  wrote:

>> HandlersOnComplexResetUpdate and HandlersOnComplexUpdate tests verify that 
>> loggers are GC'ed (or not GC'ed) after a reset or an update. For that they 
>> poll a ReferenceQueue in a loop. The number of iteration is adjusted 
>> according to the jtreg timeout factor. However, the logic in the test did 
>> not expect that the timeout might be less than 1.
>> 
>> This fix does two things:
>> 
>> 1. fix the adjustCount logic - so that the number of iteration can only be 
>> increased
>> 2. use remove(timeout) instead of poll() in order to minimize the waiting 
>> time.
>
> Daniel Fuchs has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Review feedback: use Reference::refersTo consistently

The changes look good to me.

-

Marked as reviewed by jpai (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19503#pullrequestreview-2096922118

Re: RFR: 8329581: Java launcher no longer prints a stack trace [v10]

[Jaikiran Pai]

On Fri, 31 May 2024 14:34:16 GMT, Sonia Zaldana Calles  
wrote:

>> Sonia Zaldana Calles has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Decreasing diff size addressing unnecessary changes
>
> Hi all,  
> 
> I think there's some consensus that we need some follow up cleanup issues for 
> the JNI spec, renaming constants, fixing return codes, etc. 
> 
> Seeing how that grows the scope of the issue quite a bit, I'd like to push 
> this patch and track the other issues brought up separately. 
> 
> If there are no objections about the current state, I'd like to integrate 
> some time next week. Let me know your thoughts.
> 
> cc: @jaikiran, @AlanBateman

Hello Sonia @SoniaZaldana, so based on Alan's latest inputs, the next thing to 
do on this PR is to refresh the PR to merge it with the latest master branch 
changes and then run tier1, tier2 and tier3 tests to make sure they continue to 
pass and have this integrated soon.

I'll file follow up issue(s) and also trigger CI testing of this PR.

-

PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2147476871

Re: RFR: 8326951: since-checker - missing @ since tags [v9]

[Jaikiran Pai]

On Mon, 13 May 2024 20:39:14 GMT, Nizar Benalla  wrote:

>> I added `@since` tags for methods/constructors that do not match the 
>> `@since` of the enclosing class.
>> 
>> The `write` method already existed in `PrintStream` in earlier versions and 
>> instances of it could always call this method, since it extends 
>> `FilterOutputStream` [which has the 
>> method](https://github.com/openjdk/jdk6/blob/3e49aa876353eaa215cde71eb21acc9b7f9872a0/jdk/src/share/classes/java/io/FilterOutputStream.java#L96).
>> 
>> for `MappedByteBuffer slice()` and `MappedByteBuffer slice(int index, int 
>> length)`, the return type changed from `ByteBuffer ` to `MappedByteBuffer`. 
>> And the checker tool differentiates between them because of that.
>> 
>> This is similar to #18032 and #18373 
>> 
>> For context, I am writing tests to check for accurate use of `@since` tags 
>> in documentation comments in source code.
>> We're following these rules for now:
>> 
>> ### Rule 1: Introduction of New Elements
>> 
>> - If an element is new in JDK N, with no equivalent in JDK N-1, it must 
>> include `@since N`.
>>   - Exception: Member elements (fields, methods, nested classes) may omit 
>> `@since` if their version matches the value specified for the enclosing 
>> class or interface.
>> 
>> ### Rule 2: Existing Elements in Subsequent JDK Versions
>> 
>> - If an element exists in JDK N, with an equivalent in JDK N-1, it should 
>> not include `@since N`.
>> 
>> ### Rule 3: Handling Missing `@since` Tags in methods if there is no `@since`
>> 
>> - When inspecting methods, prioritize the `@since` annotation of the 
>> supertype's overridden method.
>> - If unavailable or if the enclosing class's `@since` is newer, use the 
>> enclosing element's `@since`.
>> 
>>   I.e. if A extends B, and we add a method to B in JDK N, and add an 
>> override of the method to A in JDK M (M > N), we will use N as the effective 
>> `@since` for the method.
>
> Nizar Benalla has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   empty commit and merge

These changes look good to me. Thank you Nizar for the work on the `@since` 
checker and these PRs in individual areas.

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

> 340:  * @since 11
> 341:  */
> 342: public void setDictionary(ByteBuffer dictionary) {

This method was introduced in Java 11 through 
https://bugs.openjdk.org/browse/JDK-6341887. It appears to be an oversight that 
a `@since 11` wasn't added to this method in that change, because other new 
methods introduced in that change did come with a `@since 11`. So this addition 
of `@since 11` now looks fine to me.

-

Marked as reviewed by jpai (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18055#pullrequestreview-2096192264
PR Review Comment: https://git.openjdk.org/jdk/pull/18055#discussion_r1625879138

Re: RFR: 8326951: since-checker - missing @ since tags [v9]

[Jaikiran Pai]

On Mon, 13 May 2024 20:39:14 GMT, Nizar Benalla  wrote:

>> I added `@since` tags for methods/constructors that do not match the 
>> `@since` of the enclosing class.
>> 
>> The `write` method already existed in `PrintStream` in earlier versions and 
>> instances of it could always call this method, since it extends 
>> `FilterOutputStream` [which has the 
>> method](https://github.com/openjdk/jdk6/blob/3e49aa876353eaa215cde71eb21acc9b7f9872a0/jdk/src/share/classes/java/io/FilterOutputStream.java#L96).
>> 
>> for `MappedByteBuffer slice()` and `MappedByteBuffer slice(int index, int 
>> length)`, the return type changed from `ByteBuffer ` to `MappedByteBuffer`. 
>> And the checker tool differentiates between them because of that.
>> 
>> This is similar to #18032 and #18373 
>> 
>> For context, I am writing tests to check for accurate use of `@since` tags 
>> in documentation comments in source code.
>> We're following these rules for now:
>> 
>> ### Rule 1: Introduction of New Elements
>> 
>> - If an element is new in JDK N, with no equivalent in JDK N-1, it must 
>> include `@since N`.
>>   - Exception: Member elements (fields, methods, nested classes) may omit 
>> `@since` if their version matches the value specified for the enclosing 
>> class or interface.
>> 
>> ### Rule 2: Existing Elements in Subsequent JDK Versions
>> 
>> - If an element exists in JDK N, with an equivalent in JDK N-1, it should 
>> not include `@since N`.
>> 
>> ### Rule 3: Handling Missing `@since` Tags in methods if there is no `@since`
>> 
>> - When inspecting methods, prioritize the `@since` annotation of the 
>> supertype's overridden method.
>> - If unavailable or if the enclosing class's `@since` is newer, use the 
>> enclosing element's `@since`.
>> 
>>   I.e. if A extends B, and we add a method to B in JDK N, and add an 
>> override of the method to A in JDK M (M > N), we will use N as the effective 
>> `@since` for the method.
>
> Nizar Benalla has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   empty commit and merge

src/java.base/share/classes/java/util/concurrent/ThreadLocalRandom.java line 
513:

> 511:  */
> 512: @Override
> 513: public float nextFloat(float bound) {

These 2 `nextFloat(...)` methods which take parameters were introduced in Java 
17 https://bugs.openjdk.org/browse/JDK-8248862, whereas the no-arg 
`nextFloat()` method existed prior to that. So adding `@since 17` to these 2 
methods looks correct to me.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18055#discussion_r1625871382

Re: RFR: 8326951: since-checker - missing @ since tags [v9]

[Jaikiran Pai]

On Mon, 13 May 2024 20:39:14 GMT, Nizar Benalla  wrote:

>> I added `@since` tags for methods/constructors that do not match the 
>> `@since` of the enclosing class.
>> 
>> The `write` method already existed in `PrintStream` in earlier versions and 
>> instances of it could always call this method, since it extends 
>> `FilterOutputStream` [which has the 
>> method](https://github.com/openjdk/jdk6/blob/3e49aa876353eaa215cde71eb21acc9b7f9872a0/jdk/src/share/classes/java/io/FilterOutputStream.java#L96).
>> 
>> for `MappedByteBuffer slice()` and `MappedByteBuffer slice(int index, int 
>> length)`, the return type changed from `ByteBuffer ` to `MappedByteBuffer`. 
>> And the checker tool differentiates between them because of that.
>> 
>> This is similar to #18032 and #18373 
>> 
>> For context, I am writing tests to check for accurate use of `@since` tags 
>> in documentation comments in source code.
>> We're following these rules for now:
>> 
>> ### Rule 1: Introduction of New Elements
>> 
>> - If an element is new in JDK N, with no equivalent in JDK N-1, it must 
>> include `@since N`.
>>   - Exception: Member elements (fields, methods, nested classes) may omit 
>> `@since` if their version matches the value specified for the enclosing 
>> class or interface.
>> 
>> ### Rule 2: Existing Elements in Subsequent JDK Versions
>> 
>> - If an element exists in JDK N, with an equivalent in JDK N-1, it should 
>> not include `@since N`.
>> 
>> ### Rule 3: Handling Missing `@since` Tags in methods if there is no `@since`
>> 
>> - When inspecting methods, prioritize the `@since` annotation of the 
>> supertype's overridden method.
>> - If unavailable or if the enclosing class's `@since` is newer, use the 
>> enclosing element's `@since`.
>> 
>>   I.e. if A extends B, and we add a method to B in JDK N, and add an 
>> override of the method to A in JDK M (M > N), we will use N as the effective 
>> `@since` for the method.
>
> Nizar Benalla has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   empty commit and merge

src/java.base/share/classes/java/util/Properties.java line 190:

> 188:  * @since 10
> 189:  */
> 190: public Properties(int initialCapacity) {

This constructor was introduced in Java 10 through 
https://bugs.openjdk.org/browse/JDK-8189319, so adding `@since 10` here looks 
correct to me.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18055#discussion_r1625864049

Re: RFR: 8326951: since-checker - missing @ since tags [v9]

[Jaikiran Pai]

On Mon, 13 May 2024 20:39:14 GMT, Nizar Benalla  wrote:

>> I added `@since` tags for methods/constructors that do not match the 
>> `@since` of the enclosing class.
>> 
>> The `write` method already existed in `PrintStream` in earlier versions and 
>> instances of it could always call this method, since it extends 
>> `FilterOutputStream` [which has the 
>> method](https://github.com/openjdk/jdk6/blob/3e49aa876353eaa215cde71eb21acc9b7f9872a0/jdk/src/share/classes/java/io/FilterOutputStream.java#L96).
>> 
>> for `MappedByteBuffer slice()` and `MappedByteBuffer slice(int index, int 
>> length)`, the return type changed from `ByteBuffer ` to `MappedByteBuffer`. 
>> And the checker tool differentiates between them because of that.
>> 
>> This is similar to #18032 and #18373 
>> 
>> For context, I am writing tests to check for accurate use of `@since` tags 
>> in documentation comments in source code.
>> We're following these rules for now:
>> 
>> ### Rule 1: Introduction of New Elements
>> 
>> - If an element is new in JDK N, with no equivalent in JDK N-1, it must 
>> include `@since N`.
>>   - Exception: Member elements (fields, methods, nested classes) may omit 
>> `@since` if their version matches the value specified for the enclosing 
>> class or interface.
>> 
>> ### Rule 2: Existing Elements in Subsequent JDK Versions
>> 
>> - If an element exists in JDK N, with an equivalent in JDK N-1, it should 
>> not include `@since N`.
>> 
>> ### Rule 3: Handling Missing `@since` Tags in methods if there is no `@since`
>> 
>> - When inspecting methods, prioritize the `@since` annotation of the 
>> supertype's overridden method.
>> - If unavailable or if the enclosing class's `@since` is newer, use the 
>> enclosing element's `@since`.
>> 
>>   I.e. if A extends B, and we add a method to B in JDK N, and add an 
>> override of the method to A in JDK M (M > N), we will use N as the effective 
>> `@since` for the method.
>
> Nizar Benalla has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   empty commit and merge

src/java.base/share/classes/java/nio/MappedByteBuffer.java line 404:

> 402:  */
> 403: @Override
> 404: public abstract MappedByteBuffer slice();

This and the other 3 methods on which this `@since 17` is being added here were 
introduced in Java 17, through https://bugs.openjdk.org/browse/JDK-4833719. 
Before that change, these methods would have been available on this 
`MappedByteBuffer` through the super `ByteBuffer` class and those methods have 
been specified to return a `ByteBuffer`. After that change in JDK-4833719, 
these methods now return a `MappedByteBuffer` which is an observable change. So 
adding `@since 17` to these methods looks correct to me.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18055#discussion_r1625858949

Re: RFR: 8326951: since-checker - missing @ since tags [v9]

[Jaikiran Pai]

On Mon, 13 May 2024 20:39:14 GMT, Nizar Benalla  wrote:

>> I added `@since` tags for methods/constructors that do not match the 
>> `@since` of the enclosing class.
>> 
>> The `write` method already existed in `PrintStream` in earlier versions and 
>> instances of it could always call this method, since it extends 
>> `FilterOutputStream` [which has the 
>> method](https://github.com/openjdk/jdk6/blob/3e49aa876353eaa215cde71eb21acc9b7f9872a0/jdk/src/share/classes/java/io/FilterOutputStream.java#L96).
>> 
>> for `MappedByteBuffer slice()` and `MappedByteBuffer slice(int index, int 
>> length)`, the return type changed from `ByteBuffer ` to `MappedByteBuffer`. 
>> And the checker tool differentiates between them because of that.
>> 
>> This is similar to #18032 and #18373 
>> 
>> For context, I am writing tests to check for accurate use of `@since` tags 
>> in documentation comments in source code.
>> We're following these rules for now:
>> 
>> ### Rule 1: Introduction of New Elements
>> 
>> - If an element is new in JDK N, with no equivalent in JDK N-1, it must 
>> include `@since N`.
>>   - Exception: Member elements (fields, methods, nested classes) may omit 
>> `@since` if their version matches the value specified for the enclosing 
>> class or interface.
>> 
>> ### Rule 2: Existing Elements in Subsequent JDK Versions
>> 
>> - If an element exists in JDK N, with an equivalent in JDK N-1, it should 
>> not include `@since N`.
>> 
>> ### Rule 3: Handling Missing `@since` Tags in methods if there is no `@since`
>> 
>> - When inspecting methods, prioritize the `@since` annotation of the 
>> supertype's overridden method.
>> - If unavailable or if the enclosing class's `@since` is newer, use the 
>> enclosing element's `@since`.
>> 
>>   I.e. if A extends B, and we add a method to B in JDK N, and add an 
>> override of the method to A in JDK M (M > N), we will use N as the effective 
>> `@since` for the method.
>
> Nizar Benalla has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   empty commit and merge

src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 7925:

> 7923:  * @since 17
> 7924:  */
> 7925: public static MethodHandle tableSwitch(MethodHandle fallback, 
> MethodHandle... targets) {

This method was introduced in Java 17 through 
https://bugs.openjdk.org/browse/JDK-8263087. So this addition of `@since 17` 
looks fine to me.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18055#discussion_r1625845162

Re: RFR: 8326951: since-checker - missing @ since tags [v9]

[Jaikiran Pai]

On Mon, 13 May 2024 20:39:14 GMT, Nizar Benalla  wrote:

>> I added `@since` tags for methods/constructors that do not match the 
>> `@since` of the enclosing class.
>> 
>> The `write` method already existed in `PrintStream` in earlier versions and 
>> instances of it could always call this method, since it extends 
>> `FilterOutputStream` [which has the 
>> method](https://github.com/openjdk/jdk6/blob/3e49aa876353eaa215cde71eb21acc9b7f9872a0/jdk/src/share/classes/java/io/FilterOutputStream.java#L96).
>> 
>> for `MappedByteBuffer slice()` and `MappedByteBuffer slice(int index, int 
>> length)`, the return type changed from `ByteBuffer ` to `MappedByteBuffer`. 
>> And the checker tool differentiates between them because of that.
>> 
>> This is similar to #18032 and #18373 
>> 
>> For context, I am writing tests to check for accurate use of `@since` tags 
>> in documentation comments in source code.
>> We're following these rules for now:
>> 
>> ### Rule 1: Introduction of New Elements
>> 
>> - If an element is new in JDK N, with no equivalent in JDK N-1, it must 
>> include `@since N`.
>>   - Exception: Member elements (fields, methods, nested classes) may omit 
>> `@since` if their version matches the value specified for the enclosing 
>> class or interface.
>> 
>> ### Rule 2: Existing Elements in Subsequent JDK Versions
>> 
>> - If an element exists in JDK N, with an equivalent in JDK N-1, it should 
>> not include `@since N`.
>> 
>> ### Rule 3: Handling Missing `@since` Tags in methods if there is no `@since`
>> 
>> - When inspecting methods, prioritize the `@since` annotation of the 
>> supertype's overridden method.
>> - If unavailable or if the enclosing class's `@since` is newer, use the 
>> enclosing element's `@since`.
>> 
>>   I.e. if A extends B, and we add a method to B in JDK N, and add an 
>> override of the method to A in JDK M (M > N), we will use N as the effective 
>> `@since` for the method.
>
> Nizar Benalla has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   empty commit and merge

src/java.base/share/classes/java/lang/reflect/MalformedParameterizedTypeException.java
 line 56:

> 54:  * @since 10
> 55:  */
> 56: public MalformedParameterizedTypeException(String message) {

Both this and the explicit no-arg constructor were introduced in this class 
through https://bugs.openjdk.org/browse/JDK-8183175 in Java 10. Since an 
(implicit) no-arg constructor was always available even before that change, it 
makes sense to add a `@since 10` to only this explicit constructor. This change 
looks good.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18055#discussion_r1625848542

Re: RFR: 8326951: since-checker - missing @ since tags [v9]

[Jaikiran Pai]

On Mon, 13 May 2024 20:39:14 GMT, Nizar Benalla  wrote:

>> I added `@since` tags for methods/constructors that do not match the 
>> `@since` of the enclosing class.
>> 
>> The `write` method already existed in `PrintStream` in earlier versions and 
>> instances of it could always call this method, since it extends 
>> `FilterOutputStream` [which has the 
>> method](https://github.com/openjdk/jdk6/blob/3e49aa876353eaa215cde71eb21acc9b7f9872a0/jdk/src/share/classes/java/io/FilterOutputStream.java#L96).
>> 
>> for `MappedByteBuffer slice()` and `MappedByteBuffer slice(int index, int 
>> length)`, the return type changed from `ByteBuffer ` to `MappedByteBuffer`. 
>> And the checker tool differentiates between them because of that.
>> 
>> This is similar to #18032 and #18373 
>> 
>> For context, I am writing tests to check for accurate use of `@since` tags 
>> in documentation comments in source code.
>> We're following these rules for now:
>> 
>> ### Rule 1: Introduction of New Elements
>> 
>> - If an element is new in JDK N, with no equivalent in JDK N-1, it must 
>> include `@since N`.
>>   - Exception: Member elements (fields, methods, nested classes) may omit 
>> `@since` if their version matches the value specified for the enclosing 
>> class or interface.
>> 
>> ### Rule 2: Existing Elements in Subsequent JDK Versions
>> 
>> - If an element exists in JDK N, with an equivalent in JDK N-1, it should 
>> not include `@since N`.
>> 
>> ### Rule 3: Handling Missing `@since` Tags in methods if there is no `@since`
>> 
>> - When inspecting methods, prioritize the `@since` annotation of the 
>> supertype's overridden method.
>> - If unavailable or if the enclosing class's `@since` is newer, use the 
>> enclosing element's `@since`.
>> 
>>   I.e. if A extends B, and we add a method to B in JDK N, and add an 
>> override of the method to A in JDK M (M > N), we will use N as the effective 
>> `@since` for the method.
>
> Nizar Benalla has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   empty commit and merge

src/java.base/share/classes/java/io/PrintStream.java line 683:

> 681:  * @see #write(byte[],int,int)
> 682:  *
> 683:  * @since 14

The `@since 14` was added here as part of 
https://bugs.openjdk.org/browse/JDK-8187898. The CSR explains what the changes 
were https://bugs.openjdk.org/browse/JDK-8230625. As noted in that CSR's 
specification (and attached images), the change for this method in that issue 
ended up being just API clarification (through a `@apiNote`) and no other 
change to this specific method. It continued to have the same signature 
including the throws clause that was previously available on this class through 
the super `FilterOutputStream#write(byte[])` method.

So removing this `@since 14` looks correct to me.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18055#discussion_r1625840895

Integrated: 8333398: Uncomment the commented test in test/jdk/java/util/jar/JarFile/mrjar/MultiReleaseJarAPI.java

[Jaikiran Pai]

On Mon, 3 Jun 2024 04:25:38 GMT, Jaikiran Pai  wrote:

> Can I please get a review of this test-only change which uncomments an 
> additional test that was commented out in 
> `test/jdk/java/util/jar/JarFile/mrjar/MultiReleaseJarAPI.java`?
> 
> As noted in https://bugs.openjdk.org/browse/JDK-898, the original issue 
> due to which this line was commented out in the test, has been fixed several 
> releases back and thus this line can now be uncommented.
> 
> I've verified that this test continues to pass with these changes.

This pull request has now been integrated.

Changeset: d230b303
Author:Jaikiran Pai 
URL:   
https://git.openjdk.org/jdk/commit/d230b30353f59135287436b09949b80e9fd73a93
Stats: 5 lines in 1 file changed: 0 ins; 3 del; 2 mod

898: Uncomment the commented test in 
test/jdk/java/util/jar/JarFile/mrjar/MultiReleaseJarAPI.java

Reviewed-by: iris, lancea

-

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

Re: RFR: 8333398: Uncomment the commented test in test/jdk/java/util/jar/JarFile/mrjar/MultiReleaseJarAPI.java

[Jaikiran Pai]

On Mon, 3 Jun 2024 04:25:38 GMT, Jaikiran Pai  wrote:

> Can I please get a review of this test-only change which uncomments an 
> additional test that was commented out in 
> `test/jdk/java/util/jar/JarFile/mrjar/MultiReleaseJarAPI.java`?
> 
> As noted in https://bugs.openjdk.org/browse/JDK-898, the original issue 
> due to which this line was commented out in the test, has been fixed several 
> releases back and thus this line can now be uncommented.
> 
> I've verified that this test continues to pass with these changes.

Thank you Iris and Lance for the reviews.

> We should look at at a later date to convert this to junit and when we do, 
> look to rework isMultiReleaseJar() as it actually runs multiple tests within 
> the test itself and if one fails, the rest will not be run

I have added it to my TODO to update this test in future.

-

PR Comment: https://git.openjdk.org/jdk/pull/19514#issuecomment-2146416470

Re: RFR: 8206447: InflaterInputStream.skip receives long but it's limited to Integer.MAX_VALUE

[Jaikiran Pai]

On Mon, 3 Jun 2024 05:06:00 GMT, Jaikiran Pai  wrote:

> Can I please get a review of this change which updates the API specification 
> of `java.util.zip.InflaterInputStream.skip()` method to match its current 
> implementation?
> 
> `InflaterInputStream.skip()`, just like `DeflaterInputStream.skip()`, takes a 
> `long` value `n` representing the number of bytes to skip. When a value 
> greater than `Integer.MAX_VALUE` is passed to this 
> `InflaterInputStream.skip()` method, the current implementation in that 
> method only skips at most `Integer.MAX_VALUE` bytes. 
> `DeflaterInputStream.skip()` too has the same behaviour. However, in the case 
> of `DeflaterInputStream.skip()`, this semantic is clearly noted in that 
> method's API documentation. `InflaterInputStream.skip()` currently doesn't 
> specify this behaviour.
> 
> The commit in this PR updates the `InflaterInputStream.skip()` to specify the 
> current implementation.
> 
> I'll open a CSR for this change.

The CSR is available for review at https://bugs.openjdk.org/browse/JDK-8333400

-

PR Comment: https://git.openjdk.org/jdk/pull/19515#issuecomment-2144323834

Re: RFR: 8206447: InflaterInputStream.skip receives long but it's limited to Integer.MAX_VALUE [v2]

[Jaikiran Pai]

> Can I please get a review of this change which updates the API specification 
> of `java.util.zip.InflaterInputStream.skip()` method to match its current 
> implementation?
> 
> `InflaterInputStream.skip()`, just like `DeflaterInputStream.skip()`, takes a 
> `long` value `n` representing the number of bytes to skip. When a value 
> greater than `Integer.MAX_VALUE` is passed to this 
> `InflaterInputStream.skip()` method, the current implementation in that 
> method only skips at most `Integer.MAX_VALUE` bytes. 
> `DeflaterInputStream.skip()` too has the same behaviour. However, in the case 
> of `DeflaterInputStream.skip()`, this semantic is clearly noted in that 
> method's API documentation. `InflaterInputStream.skip()` currently doesn't 
> specify this behaviour.
> 
> The commit in this PR updates the `InflaterInputStream.skip()` to specify the 
> current implementation.
> 
> I'll open a CSR for this change.

Jaikiran Pai has updated the pull request incrementally with one additional 
commit since the last revision:

  also note that the method may block, just like DeflaterInputStream specifies 
it

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19515/files
  - new: https://git.openjdk.org/jdk/pull/19515/files/2a116fe4..e13e65d2

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=19515=01
 - incr: https://webrevs.openjdk.org/?repo=jdk=19515=00-01

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

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

RFR: 8206447: InflaterInputStream.skip receives long but it's limited to Integer.MAX_VALUE

[Jaikiran Pai]

Can I please get a review of this change which updates the API specification of 
`java.util.zip.InflaterInputStream.skip()` method to match its current 
implementation?

`InflaterInputStream.skip()`, just like `DeflaterInputStream.skip()`, takes a 
`long` value `n` representing the number of bytes to skip. When a value greater 
than `Integer.MAX_VALUE` is passed to this `InflaterInputStream.skip()` method, 
the current implementation in that method only skips at most 
`Integer.MAX_VALUE` bytes. `DeflaterInputStream.skip()` too has the same 
behaviour. However, in the case of `DeflaterInputStream.skip()`, this semantic 
is clearly noted in that method's API documentation. 
`InflaterInputStream.skip()` currently doesn't specify this behaviour.

The commit in this PR updates the `InflaterInputStream.skip()` to specify the 
current implementation.

I'll open a CSR for this change.

-

Commit messages:
 - 8206447: InflaterInputStream.skip receives long but it's limited to 
Integer.MAX_VALUE

Changes: https://git.openjdk.org/jdk/pull/19515/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk=19515=00
  Issue: https://bugs.openjdk.org/browse/JDK-8206447
  Stats: 10 lines in 2 files changed: 7 ins; 0 del; 3 mod
  Patch: https://git.openjdk.org/jdk/pull/19515.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19515/head:pull/19515

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

RFR: 8333398: Uncomment the commented test in test/jdk/java/util/jar/JarFile/mrjar/MultiReleaseJarAPI.java

[Jaikiran Pai]

Can I please get a review of this test-only change which uncomments an 
additional test that was commented out in 
`test/jdk/java/util/jar/JarFile/mrjar/MultiReleaseJarAPI.java`?

As noted in https://bugs.openjdk.org/browse/JDK-898, the original issue due 
to which this line was commented out in the test, has been fixed several 
releases back and thus this line can now be uncommented.

I've verified that this test continues to pass with these changes.

-

Commit messages:
 - 898: Uncomment the commented test in 
test/jdk/java/util/jar/JarFile/mrjar/MultiReleaseJarAPI.java

Changes: https://git.openjdk.org/jdk/pull/19514/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk=19514=00
  Issue: https://bugs.openjdk.org/browse/JDK-898
  Stats: 5 lines in 1 file changed: 0 ins; 3 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/19514.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19514/head:pull/19514

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

Integrated: 7022325: TEST_BUG: test/java/util/zip/ZipFile/ReadLongZipFileName.java leaks files if it fails

[Jaikiran Pai]

On Fri, 31 May 2024 00:57:18 GMT, Jaikiran Pai  wrote:

> Can I please get a review of this test-only change which updates a couple of 
> places in the test to use `try-with-resource`?
> 
> As noted in https://bugs.openjdk.org/browse/JDK-7022325 this change should 
> prevent leaking of resources in case there's any failure in the test. The 
> test continues to pass with this change.

This pull request has now been integrated.

Changeset: 4785461f
Author:Jaikiran Pai 
URL:   
https://git.openjdk.org/jdk/commit/4785461f61d8f5c7444d2e6fd90f1e083dbc6fe4
Stats: 128 lines in 1 file changed: 34 ins; 68 del; 26 mod

7022325: TEST_BUG: test/java/util/zip/ZipFile/ReadLongZipFileName.java leaks 
files if it fails

Reviewed-by: lancea

-

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

Re: RFR: 7022325: TEST_BUG: test/java/util/zip/ZipFile/ReadLongZipFileName.java leaks files if it fails [v2]

[Jaikiran Pai]

On Sat, 1 Jun 2024 05:18:17 GMT, Jaikiran Pai  wrote:

>> Can I please get a review of this test-only change which updates a couple of 
>> places in the test to use `try-with-resource`?
>> 
>> As noted in https://bugs.openjdk.org/browse/JDK-7022325 this change should 
>> prevent leaking of resources in case there's any failure in the test. The 
>> test continues to pass with this change.
>
> Jaikiran Pai has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   convert the test to junit

Thank you Lance for the review.

-

PR Comment: https://git.openjdk.org/jdk/pull/19492#issuecomment-2143653405

Re: RFR: 7022325: TEST_BUG: test/java/util/zip/ZipFile/ReadLongZipFileName.java leaks files if it fails [v2]

[Jaikiran Pai]

On Sat, 1 Jun 2024 05:18:17 GMT, Jaikiran Pai  wrote:

>> Can I please get a review of this test-only change which updates a couple of 
>> places in the test to use `try-with-resource`?
>> 
>> As noted in https://bugs.openjdk.org/browse/JDK-7022325 this change should 
>> prevent leaking of resources in case there's any failure in the test. The 
>> test continues to pass with this change.
>
> Jaikiran Pai has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   convert the test to junit

Hello Lance, I have now updated the PR to convert this test to junit test. 
While at it, I have also cleaned up the code to use `java.nio.file.Path` and 
use the jtreg scratch directory for the directories and the jar file this test 
creates. That way, we don't have to manually delete the directories when done. 
I have verified that the changes don't change the semantics of what was being 
tested in this test, which was a regression test for 
https://bugs.openjdk.org/browse/JDK-6374379.

The test continues to pass with these change in our CI against all platforms.

-

PR Comment: https://git.openjdk.org/jdk/pull/19492#issuecomment-2143307371

Re: RFR: 8333270: HandlersOnComplexResetUpdate and HandlersOnComplexUpdate tests fail with "Unexpected reference" if timeoutFactor is less than 1/3

[Jaikiran Pai]

On Fri, 31 May 2024 14:55:57 GMT, Daniel Fuchs  wrote:

> HandlersOnComplexResetUpdate and HandlersOnComplexUpdate tests verify that 
> loggers are GC'ed (or not GC'ed) after a reset or an update. For that they 
> poll a ReferenceQueue in a loop. The number of iteration is adjusted 
> according to the jtreg timeout factor. However, the logic in the test did not 
> expect that the timeout might be less than 1.
> 
> This fix does two things:
> 
> 1. fix the adjustCount logic - so that the number of iteration can only be 
> increased
> 2. use remove(timeout) instead of poll() in order to minimize the waiting 
> time.

test/jdk/java/util/logging/LogManager/Configuration/updateConfiguration/HandlersOnComplexResetUpdate.java
 line 219:

> 217: }
> 218: WeakReference barRef = new 
> WeakReference<>(Logger.getLogger("com.bar"), queue);
> 219: if (!barRef.refersTo(barChild.getParent())) {

Hello Daniel, since `refersTo()` is the preferred API in cases like this, 
should this same change be done in the other `HandlersOnComplexUpdate` test 
that's being updated in this PR?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19503#discussion_r1623151595

Re: RFR: 7022325: TEST_BUG: test/java/util/zip/ZipFile/ReadLongZipFileName.java leaks files if it fails [v2]

[Jaikiran Pai]

> Can I please get a review of this test-only change which updates a couple of 
> places in the test to use `try-with-resource`?
> 
> As noted in https://bugs.openjdk.org/browse/JDK-7022325 this change should 
> prevent leaking of resources in case there's any failure in the test. The 
> test continues to pass with this change.

Jaikiran Pai has updated the pull request incrementally with one additional 
commit since the last revision:

  convert the test to junit

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19492/files
  - new: https://git.openjdk.org/jdk/pull/19492/files/3ce9ca81..b9d56006

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=19492=01
 - incr: https://webrevs.openjdk.org/?repo=jdk=19492=00-01

  Stats: 122 lines in 1 file changed: 28 ins; 60 del; 34 mod
  Patch: https://git.openjdk.org/jdk/pull/19492.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19492/head:pull/19492

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

RFR: 8026127: Deflater/Inflater documentation incomplete/misleading

[Jaikiran Pai]

Can I please get a review of this doc-only change which proposes to improve the 
code snippet that's in `java.util.zip.Deflater` and `java.util.zip.Inflater` to 
better explain the usage of those classes? This addresses 
https://bugs.openjdk.org/browse/JDK-8026127.

The commit in the PR cleans up the snippet to correctly compress/decompress 
till the `Deflater` and `Inflater` are `finished()`.  Additionally, the snippet 
also shows that the `Deflater` and `Inflater` are expected to be closed when 
their usage it done, to release the resources held by those instances.

I've run `make docs-image` locally to verify that the generated snippet content 
as well as the link from `Inflater` work fine in the rendered javadoc HTML.

-

Commit messages:
 - 8026127: Deflater/Inflater documentation incomplete/misleading

Changes: https://git.openjdk.org/jdk/pull/19507/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk=19507=00
  Issue: https://bugs.openjdk.org/browse/JDK-8026127
  Stats: 88 lines in 2 files changed: 31 ins; 31 del; 26 mod
  Patch: https://git.openjdk.org/jdk/pull/19507.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19507/head:pull/19507

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

Integrated: 8210471: GZIPInputStream constructor could leak an un-end()ed Inflater

[Jaikiran Pai]

On Thu, 30 May 2024 12:14:53 GMT, Jaikiran Pai  wrote:

> Can I please get a review of this change which proposes to address the issue 
> noted in https://bugs.openjdk.org/browse/JDK-8210471?
> 
> `java.util.zip.Inflater` when instantiated will hold on the native resources 
> which are freed only when `Inflater.end()` is called. The constructor of 
> `java.util.zip.GZIPInputStream` instantiates an `Inflater` for its internal 
> use. After instantiating the `Inflater`, the `GZIPInputStream` constructor 
> before returning from the constructor, can run into either a 
> `IllegalArgumentException` (because the `size` is `<=0`) or an `IOException` 
> when trying to read a GZIP header from the underlying `InputStream`. In 
> either of these cases, the `Inflater` that the `GZIPInputStream` created 
> internally will end up leaking and the caller has no way to `end()` that 
> `Inflater` or even knowledge of that `Inflater`.
> 
> The commit in this PR catches the `IOException` when reading the GZIP header 
> and `end()`s the `Inflater`. Furthermore, to prevent instantiation of an 
> `Inflater`, the `GZIPInputStream` constructor before calling `super(...)` 
> will now check the `InputStream` to be non-null and `size` to be `>0`, both 
> of which were being done by the `super` constructor.
> 
> Given the nature of the change, no new test has been added. Existing tests in 
> `test/jdk/java/util/zip` continue to pass and tier1, tier2 and tier3 testing 
> is in progress.

This pull request has now been integrated.

Changeset: d9e7b7e7
Author:Jaikiran Pai 
URL:   
https://git.openjdk.org/jdk/commit/d9e7b7e7da98a0170d26301a4bbd61aad0127c6e
Stats: 118 lines in 2 files changed: 116 ins; 0 del; 2 mod

8210471: GZIPInputStream constructor could leak an un-end()ed Inflater

Reviewed-by: lancea

-

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

Re: RFR: 8210471: GZIPInputStream constructor could leak an un-end()ed Inflater [v4]

[Jaikiran Pai]

On Fri, 31 May 2024 11:03:18 GMT, Jaikiran Pai  wrote:

>> Can I please get a review of this change which proposes to address the issue 
>> noted in https://bugs.openjdk.org/browse/JDK-8210471?
>> 
>> `java.util.zip.Inflater` when instantiated will hold on the native resources 
>> which are freed only when `Inflater.end()` is called. The constructor of 
>> `java.util.zip.GZIPInputStream` instantiates an `Inflater` for its internal 
>> use. After instantiating the `Inflater`, the `GZIPInputStream` constructor 
>> before returning from the constructor, can run into either a 
>> `IllegalArgumentException` (because the `size` is `<=0`) or an `IOException` 
>> when trying to read a GZIP header from the underlying `InputStream`. In 
>> either of these cases, the `Inflater` that the `GZIPInputStream` created 
>> internally will end up leaking and the caller has no way to `end()` that 
>> `Inflater` or even knowledge of that `Inflater`.
>> 
>> The commit in this PR catches the `IOException` when reading the GZIP header 
>> and `end()`s the `Inflater`. Furthermore, to prevent instantiation of an 
>> `Inflater`, the `GZIPInputStream` constructor before calling `super(...)` 
>> will now check the `InputStream` to be non-null and `size` to be `>0`, both 
>> of which were being done by the `super` constructor.
>> 
>> Given the nature of the change, no new test has been added. Existing tests 
>> in `test/jdk/java/util/zip` continue to pass and tier1, tier2 and tier3 
>> testing is in progress.
>
> Jaikiran Pai has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   rework the test to make it parameterized

Thank you Lance for the review. tier testing went fine with the latest state in 
this PR. I'll go ahead and integrate this now.

-

PR Comment: https://git.openjdk.org/jdk/pull/19475#issuecomment-2142193675

Re: RFR: 8210471: GZIPInputStream constructor could leak an un-end()ed Inflater [v3]

[Jaikiran Pai]

On Fri, 31 May 2024 10:35:11 GMT, Lance Andersen  wrote:

> Personally, I would have had each assertThrows as its own test, as if one 
> fails(which should not in this case) then the rest of the assertions are 
> never executed until the first failure is addressed.

I have now updated the PR to redo this new test to use ParameterizedTest. That 
way, each one will be run independent of the other. The test continues to pass 
after this change.

-

PR Comment: https://git.openjdk.org/jdk/pull/19475#issuecomment-2141776021

Re: RFR: 8210471: GZIPInputStream constructor could leak an un-end()ed Inflater [v4]

[Jaikiran Pai]

> Can I please get a review of this change which proposes to address the issue 
> noted in https://bugs.openjdk.org/browse/JDK-8210471?
> 
> `java.util.zip.Inflater` when instantiated will hold on the native resources 
> which are freed only when `Inflater.end()` is called. The constructor of 
> `java.util.zip.GZIPInputStream` instantiates an `Inflater` for its internal 
> use. After instantiating the `Inflater`, the `GZIPInputStream` constructor 
> before returning from the constructor, can run into either a 
> `IllegalArgumentException` (because the `size` is `<=0`) or an `IOException` 
> when trying to read a GZIP header from the underlying `InputStream`. In 
> either of these cases, the `Inflater` that the `GZIPInputStream` created 
> internally will end up leaking and the caller has no way to `end()` that 
> `Inflater` or even knowledge of that `Inflater`.
> 
> The commit in this PR catches the `IOException` when reading the GZIP header 
> and `end()`s the `Inflater`. Furthermore, to prevent instantiation of an 
> `Inflater`, the `GZIPInputStream` constructor before calling `super(...)` 
> will now check the `InputStream` to be non-null and `size` to be `>0`, both 
> of which were being done by the `super` constructor.
> 
> Given the nature of the change, no new test has been added. Existing tests in 
> `test/jdk/java/util/zip` continue to pass and tier1, tier2 and tier3 testing 
> is in progress.

Jaikiran Pai has updated the pull request incrementally with one additional 
commit since the last revision:

  rework the test to make it parameterized

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19475/files
  - new: https://git.openjdk.org/jdk/pull/19475/files/a70ddc79..41f5736b

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=19475=03
 - incr: https://webrevs.openjdk.org/?repo=jdk=19475=02-03

  Stats: 62 lines in 1 file changed: 38 ins; 9 del; 15 mod
  Patch: https://git.openjdk.org/jdk/pull/19475.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19475/head:pull/19475

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

Re: RFR: 8210471: GZIPInputStream constructor could leak an un-end()ed Inflater [v3]

[Jaikiran Pai]

> Can I please get a review of this change which proposes to address the issue 
> noted in https://bugs.openjdk.org/browse/JDK-8210471?
> 
> `java.util.zip.Inflater` when instantiated will hold on the native resources 
> which are freed only when `Inflater.end()` is called. The constructor of 
> `java.util.zip.GZIPInputStream` instantiates an `Inflater` for its internal 
> use. After instantiating the `Inflater`, the `GZIPInputStream` constructor 
> before returning from the constructor, can run into either a 
> `IllegalArgumentException` (because the `size` is `<=0`) or an `IOException` 
> when trying to read a GZIP header from the underlying `InputStream`. In 
> either of these cases, the `Inflater` that the `GZIPInputStream` created 
> internally will end up leaking and the caller has no way to `end()` that 
> `Inflater` or even knowledge of that `Inflater`.
> 
> The commit in this PR catches the `IOException` when reading the GZIP header 
> and `end()`s the `Inflater`. Furthermore, to prevent instantiation of an 
> `Inflater`, the `GZIPInputStream` constructor before calling `super(...)` 
> will now check the `InputStream` to be non-null and `size` to be `>0`, both 
> of which were being done by the `super` constructor.
> 
> Given the nature of the change, no new test has been added. Existing tests in 
> `test/jdk/java/util/zip` continue to pass and tier1, tier2 and tier3 testing 
> is in progress.

Jaikiran Pai has updated the pull request incrementally with one additional 
commit since the last revision:

  enhance the new test to even verify IOException from the constructor

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19475/files
  - new: https://git.openjdk.org/jdk/pull/19475/files/3d079802..a70ddc79

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=19475=02
 - incr: https://webrevs.openjdk.org/?repo=jdk=19475=01-02

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

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

RFR: 8333130: MakeJAR2.sh uses hard-coded JDK version

[Jaikiran Pai]

Can I please get a review of this test-only change which addresses 
https://bugs.openjdk.org/browse/JDK-8333130?

There are a couple of tests `NativeMethodPrefixApp` and `RetransformApp` under 
`test/jdk/java/lang/instrument/` which launch  the app/test with a 
`-javaagent:` pointing to a test specific jar. The test, launched with 
that `javaagent`, then verifies the test specific details.

In their current form, in order to construct the agent `.jar` and make it 
available to the jtreg test that's launched, they `@run` a  jtreg `shell` test. 
This `shell` test uses the `MakeJAR2.sh` script to generate the jar file. The 
contents of the script is relatively straightforward - it compiles (using 
`javac`) some boot classpath classes, some agent specific classes and then 
generates a jar file with the agent classes and a `MANIFEST.MF` which points to 
the boot classpath classes along with test specific manifest attributes. In a 
recent PR the `MakeJAR2.sh` script was updated to pass `--enable-preview 
--release 23` since one of the existing agent class was updated to use the 
ClassFile API PreviewFeature (https://github.com/openjdk/jdk/pull/13009).

This generated agent jar then is passed as `-javaagent:` to the subsequent 
`@run main` test which does the actual testing.

So essentially the `shell` test which uses the `MakeJAR2.sh` is merely there to 
create a jar file that's needed by the actual test. Within the JDK we have 
several tests which compile other classes and generate jar files using 
in-process test libraries and the `java.util.spi.ToolProvider` implementations. 
These 2 tests too can be updated to use a similar technique, to completely get 
rid of the `MakeJAR2.sh`. Removing the `MakeJAR2.sh` will simplify the ability 
to pass around value for `--release` when using `--enable-preview`.

The commit in this PR updates these 2 tests to use `@run driver` which then 
compiles relevant classes (maintaining the semantics of `MakeJAR2.sh`) and 
generates the agent jar file and then finally launching the test process. As 
part of the update, I did not retain the `@author` tag from the 2 test 
definitions, since it's my understanding that we no longer use those. I will 
add them back if we should retain them.

The tests continue to pass locally with this change. I've also triggered tier 
testing in our CI for this change.

-

Commit messages:
 - 8333130: MakeJAR2.sh uses hard-coded JDK version

Changes: https://git.openjdk.org/jdk/pull/19495/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk=19495=00
  Issue: https://bugs.openjdk.org/browse/JDK-8333130
  Stats: 409 lines in 5 files changed: 265 ins; 131 del; 13 mod
  Patch: https://git.openjdk.org/jdk/pull/19495.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19495/head:pull/19495

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

Re: RFR: 8329581: Java launcher no longer prints a stack trace [v10]

[Jaikiran Pai]

On Thu, 9 May 2024 19:52:12 GMT, Sonia Zaldana Calles  
wrote:

>> Hi folks, 
>> 
>> This PR aims to fix 
>> [JDK-8329581](https://bugs.openjdk.org/browse/JDK-8329581). 
>> 
>> I think the regression got introduced in 
>> [JDK-8315458](https://bugs.openjdk.org/browse/JDK-8315458). 
>> 
>> In the issue linked above, 
>> [LauncherHelper#getMainType](https://github.com/openjdk/jdk/pull/16461/files#diff-108a3a3e3c2d108c8c7f19ea498f641413b7c9239ecd2975a6c27d904c2ba226)
>>  got removed to simplify launcher code.
>> 
>> Previously, we used ```getMainType``` to do the appropriate main method 
>> invocation in ```JavaMain```. However, we currently attempt to do all types 
>> of main method invocations at the same time 
>> [here](https://github.com/openjdk/jdk/blob/master/src/java.base/share/native/libjli/java.c#L623).
>>  
>> 
>> Note how all of these invocations clear the exception reported with 
>> [CHECK_EXCEPTION_FAIL](https://github.com/openjdk/jdk/blob/140f56718bbbfc31bb0c39255c68568fad285a1f/src/java.base/share/native/libjli/java.c#L390).
>>  
>> 
>> Therefore, if a legitimate exception comes up during one of these 
>> invocations, it does not get reported. 
>> 
>> I propose reintroducing ```LauncherHelper#getMainType``` but I'm looking 
>> forward to your suggestions. 
>> 
>> Cheers, 
>> Sonia
>
> Sonia Zaldana Calles has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Decreasing diff size addressing unnecessary changes

Hello Sonia, I happened to be working on something else and ended up hitting an 
issue where the java launch was no longer throwing the exception from the main 
application and realized that this PR hasn't been integrated.

I see Alan has suggested a change but I'm unsure if you plan to implement that 
in this PR or if that's something that's suggested for a subsequent cleanup. I 
think it would be good to have the review suggestions resolved soon and have 
this integrated.

-

PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2141428133

Re: RFR: 8210471: GZIPInputStream constructor could leak an un-end()ed Inflater [v2]

[Jaikiran Pai]

Now that you mentioned it was added in Java 23, I went back to check 
what change brought that in. Turns out it was me who introduced it, that 
too as recently as a few months back. I too had forgotten about it :)


-Jaikiran

On 31/05/24 6:40 am, Lance Andersen wrote:

I was looking at jdk 22 and now see the npe was added to the constructor 
specification earlier this year and I reviewed it
Sent from my iPad


On May 30, 2024, at 8:56 PM, Jaikiran Pai  wrote:

On Thu, 30 May 2024 14:56:00 GMT, Lance Andersen  wrote:


Jaikiran Pai has updated the pull request incrementally with one additional 
commit since the last revision:

  introduce a basic test for GZIPInputStream

src/java.base/share/classes/java/util/zip/GZIPInputStream.java line 97:


95:  */
96: private static Inflater createInflater(InputStream in, int size) {
97: Objects.requireNonNull(in);

I don't believe we currently indicate at at NPE will be thrown if the 
InputStream is null so this would require a CSR if we need to document it

Hello Lance, both the constructors of `GZIPInputStream` already state that they 
throw a NullPointerException when the input stream is null:


 * @throwsNullPointerException if {@code in} is null

It was already being thrown from within the super constructor. To prevent 
creation of a `Inflater` when `in` is null, I had to add this check here.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19475#discussion_r1621554479

RFR: 7022325: TEST_BUG: test/java/util/zip/ZipFile/ReadLongZipFileName.java leaks files if it fails

[Jaikiran Pai]

Can I please get a review of this test-only change which updates a couple of 
places in the test to use `try-with-resource`?

As noted in https://bugs.openjdk.org/browse/JDK-7022325 this change should 
prevent leaking of resources in case there's any failure in the test. The test 
continues to pass with this change.

-

Commit messages:
 - 7022325: TEST_BUG: test/java/util/zip/ZipFile/ReadLongZipFileName.java leaks 
files if it fails

Changes: https://git.openjdk.org/jdk/pull/19492/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk=19492=00
  Issue: https://bugs.openjdk.org/browse/JDK-7022325
  Stats: 28 lines in 1 file changed: 2 ins; 4 del; 22 mod
  Patch: https://git.openjdk.org/jdk/pull/19492.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19492/head:pull/19492

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

Re: RFR: 8210471: GZIPInputStream constructor could leak an un-end()ed Inflater [v2]

[Jaikiran Pai]

On Thu, 30 May 2024 14:56:00 GMT, Lance Andersen  wrote:

>> Jaikiran Pai has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   introduce a basic test for GZIPInputStream
>
> src/java.base/share/classes/java/util/zip/GZIPInputStream.java line 97:
> 
>> 95:  */
>> 96: private static Inflater createInflater(InputStream in, int size) {
>> 97: Objects.requireNonNull(in);
> 
> I don't believe we currently indicate at at NPE will be thrown if the 
> InputStream is null so this would require a CSR if we need to document it

Hello Lance, both the constructors of `GZIPInputStream` already state that they 
throw a NullPointerException when the input stream is null:


 * @throwsNullPointerException if {@code in} is null

It was already being thrown from within the super constructor. To prevent 
creation of a `Inflater` when `in` is null, I had to add this check here.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19475#discussion_r1621554479

Re: RFR: 8210471: GZIPInputStream constructor could leak an un-end()ed Inflater [v2]

[Jaikiran Pai]

On Thu, 30 May 2024 12:32:22 GMT, Jaikiran Pai  wrote:

>> Can I please get a review of this change which proposes to address the issue 
>> noted in https://bugs.openjdk.org/browse/JDK-8210471?
>> 
>> `java.util.zip.Inflater` when instantiated will hold on the native resources 
>> which are freed only when `Inflater.end()` is called. The constructor of 
>> `java.util.zip.GZIPInputStream` instantiates an `Inflater` for its internal 
>> use. After instantiating the `Inflater`, the `GZIPInputStream` constructor 
>> before returning from the constructor, can run into either a 
>> `IllegalArgumentException` (because the `size` is `<=0`) or an `IOException` 
>> when trying to read a GZIP header from the underlying `InputStream`. In 
>> either of these cases, the `Inflater` that the `GZIPInputStream` created 
>> internally will end up leaking and the caller has no way to `end()` that 
>> `Inflater` or even knowledge of that `Inflater`.
>> 
>> The commit in this PR catches the `IOException` when reading the GZIP header 
>> and `end()`s the `Inflater`. Furthermore, to prevent instantiation of an 
>> `Inflater`, the `GZIPInputStream` constructor before calling `super(...)` 
>> will now check the `InputStream` to be non-null and `size` to be `>0`, both 
>> of which were being done by the `super` constructor.
>> 
>> Given the nature of the change, no new test has been added. Existing tests 
>> in `test/jdk/java/util/zip` continue to pass and tier1, tier2 and tier3 
>> testing is in progress.
>
> Jaikiran Pai has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   introduce a basic test for GZIPInputStream

> Given the nature of the change, no new test has been added.

I noticed that the `GZIPInputStream` doesn't have any basic tests (at least) 
for its constructors, so I went ahead and added one which verifies the expected 
exceptions are thrown from the constructor.

-

PR Comment: https://git.openjdk.org/jdk/pull/19475#issuecomment-2139463615

Re: RFR: 8210471: GZIPInputStream constructor could leak an un-end()ed Inflater [v2]

[Jaikiran Pai]

> Can I please get a review of this change which proposes to address the issue 
> noted in https://bugs.openjdk.org/browse/JDK-8210471?
> 
> `java.util.zip.Inflater` when instantiated will hold on the native resources 
> which are freed only when `Inflater.end()` is called. The constructor of 
> `java.util.zip.GZIPInputStream` instantiates an `Inflater` for its internal 
> use. After instantiating the `Inflater`, the `GZIPInputStream` constructor 
> before returning from the constructor, can run into either a 
> `IllegalArgumentException` (because the `size` is `<=0`) or an `IOException` 
> when trying to read a GZIP header from the underlying `InputStream`. In 
> either of these cases, the `Inflater` that the `GZIPInputStream` created 
> internally will end up leaking and the caller has no way to `end()` that 
> `Inflater` or even knowledge of that `Inflater`.
> 
> The commit in this PR catches the `IOException` when reading the GZIP header 
> and `end()`s the `Inflater`. Furthermore, to prevent instantiation of an 
> `Inflater`, the `GZIPInputStream` constructor before calling `super(...)` 
> will now check the `InputStream` to be non-null and `size` to be `>0`, both 
> of which were being done by the `super` constructor.
> 
> Given the nature of the change, no new test has been added. Existing tests in 
> `test/jdk/java/util/zip` continue to pass and tier1, tier2 and tier3 testing 
> is in progress.

Jaikiran Pai has updated the pull request incrementally with one additional 
commit since the last revision:

  introduce a basic test for GZIPInputStream

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19475/files
  - new: https://git.openjdk.org/jdk/pull/19475/files/8d565e11..3d079802

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=19475=01
 - incr: https://webrevs.openjdk.org/?repo=jdk=19475=00-01

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

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

RFR: 8210471: GZIPInputStream constructor could leak an un-end()ed Inflater

[Jaikiran Pai]

Can I please get a review of this change which proposes to address the issue 
noted in https://bugs.openjdk.org/browse/JDK-8210471?

`java.util.zip.Inflater` when instantiated will hold on the native resources 
which are freed only when `Inflater.end()` is called. The constructor of 
`java.util.zip.GZIPInputStream` instantiates an `Inflater` for its internal 
use. After instantiating the `Inflater`, the `GZIPInputStream` constructor 
before returning from the constructor, can run into either a 
`IllegalArgumentException` (because the `size` is `<=0`) or an `IOException` 
when trying to read a GZIP header from the underlying `InputStream`. In either 
of these cases, the `Inflater` that the `GZIPInputStream` created internally 
will end up leaking and the caller has no way to `end()` that `Inflater` or 
even knowledge of that `Inflater`.

The commit in this PR catches the `IOException` when reading the GZIP header 
and `end()`s the `Inflater`. Furthermore, to prevent instantiation of an 
`Inflater`, the `GZIPInputStream` constructor before calling `super(...)` will 
now check the `InputStream` to be non-null and `size` to be `>0`, both of which 
were being done by the `super` constructor.

Given the nature of the change, no new test has been added. Existing tests in 
`test/jdk/java/util/zip` continue to pass and tier1, tier2 and tier3 testing is 
in progress.

-

Commit messages:
 - 8210471: GZIPInputStream constructor could leak an un-end()ed Inflater

Changes: https://git.openjdk.org/jdk/pull/19475/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk=19475=00
  Issue: https://bugs.openjdk.org/browse/JDK-8210471
  Stats: 23 lines in 1 file changed: 21 ins; 0 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/19475.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19475/head:pull/19475

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

Re: RFR: 8333103: Re-examine the console provider loading

[Jaikiran Pai]

On Wed, 29 May 2024 19:51:36 GMT, Naoto Sato  wrote:

> There is an initialization code in `Console` class that searches for the 
> Console implementations. Refactoring the init code not to use lambda/stream 
> would reduce the (initial) number of loaded classes by about 100 for 
> java.base implementations. This would become relevant when the java.io.IO 
> (JEP 477) uses Console as the underlying framework.

The change to replace lambda with anonymous class looks OK to me.

-

Marked as reviewed by jpai (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19467#pullrequestreview-2087164330

Re: RFR: 8331670: Deprecate the Memory-Access Methods in sun.misc.Unsafe for Removal [v3]

[Jaikiran Pai]

On Tue, 21 May 2024 07:26:17 GMT, Alan Bateman  wrote:

>> This is the implementation changes for JEP 471.
>> 
>> The methods in sun.misc.Unsafe for on-heap and off-heap access are 
>> deprecated for removal. This means a removal warning at compile time. No 
>> methods have been removed. A deprecated message is added to each of the 
>> methods but unlikely to be seen as the JDK does not generate or publish the 
>> API docs for this class.
>> 
>> A new command line option --sun-misc-unsafe-memory-access=$value is 
>> introduced to allow or deny access to these methods. The default proposed 
>> for JDK 23 is "allow" so no change in behavior compared to JDK 22 or 
>> previous releases.
>> 
>> A new test is added to test the command line option settings. The existing 
>> micros for FFM that use Unsafe are updated to suppress the removal warning 
>> at compile time. A new micro is introduced with a small sample of methods to 
>> ensure the changes doesn't cause any perf regressions ([sample 
>> results](https://cr.openjdk.org/~alanb/8331670-results.txt)).
>> 
>> For now, the changes include the update to the man page for the "java" 
>> command. It might be that this has to be separated out so that it goes with 
>> other updates in the release.
>
> Alan Bateman has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 10 commits:
> 
>  - Merge
>  - Add removal to DISABLED_WARNINGS
>Fail at startup if bad value specified to option
>  - Merge
>  - Update man page
>  - Update man page
>  - Fix comment
>  - Merge
>  - Merge
>  - Whitespace
>  - Initial commit

Marked as reviewed by jpai (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19174#pullrequestreview-2072956262

Re: RFR: 8332490: JMH org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead OOM [v2]

[Jaikiran Pai]

On Wed, 22 May 2024 07:49:27 GMT, Jaikiran Pai  wrote:

>> Can I please get a review of this test-only change for addressing 
>> https://bugs.openjdk.org/browse/JDK-8332490?
>> 
>> The jmh test opens a `InflaterInputStream`, reads the stream contents, but 
>> then doesn't close the stream. This can lead to resource leak which can then 
>> cause OOM as noted in that JBS issue.
>> 
>> The commit in this PR closes the `InflaterInputStream` when the reads 
>> complete.
>> 
>> 
>> make test TEST=micro:org.openjdk.bench.java.util.zip.InflaterInputStreams
>> 
>> and
>> 
>> 
>> ./build/macosx-aarch64/images/jdk/bin/java -jar 
>> ./build/macosx-aarch64/images/test/micro/benchmarks.jar 
>> org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead 
>> -t max -f 1 -wi 2 -foe true
>> 
>> pass after this change.
>
> Jaikiran Pai has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   add a comment

Thank you Alan and Claes for the reviews.

-

PR Comment: https://git.openjdk.org/jdk/pull/19340#issuecomment-2126009374

Integrated: 8332490: JMH org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead OOM

[Jaikiran Pai]

On Wed, 22 May 2024 05:16:42 GMT, Jaikiran Pai  wrote:

> Can I please get a review of this test-only change for addressing 
> https://bugs.openjdk.org/browse/JDK-8332490?
> 
> The jmh test opens a `InflaterInputStream`, reads the stream contents, but 
> then doesn't close the stream. This can lead to resource leak which can then 
> cause OOM as noted in that JBS issue.
> 
> The commit in this PR closes the `InflaterInputStream` when the reads 
> complete.
> 
> 
> make test TEST=micro:org.openjdk.bench.java.util.zip.InflaterInputStreams
> 
> and
> 
> 
> ./build/macosx-aarch64/images/jdk/bin/java -jar 
> ./build/macosx-aarch64/images/test/micro/benchmarks.jar 
> org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead 
> -t max -f 1 -wi 2 -foe true
> 
> pass after this change.

This pull request has now been integrated.

Changeset: 98f6a808
Author:Jaikiran Pai 
URL:   
https://git.openjdk.org/jdk/commit/98f6a80852383dcbdad7292b7d269a8547d54d45
Stats: 5 lines in 1 file changed: 3 ins; 0 del; 2 mod

8332490: JMH 
org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead OOM

Reviewed-by: aturbanov, redestad

-

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

Re: RFR: 8332490: JMH org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead OOM [v2]

[Jaikiran Pai]

On Wed, 22 May 2024 07:28:04 GMT, Claes Redestad  wrote:

> LGTM - feel free to add a comment that closing the InflaterInputStream has no 
> effect on the underlying stream deflated.

Hello Claes, I've updated the PR to include a comment.

-

PR Comment: https://git.openjdk.org/jdk/pull/19340#issuecomment-2124098078

Re: RFR: 8332490: JMH org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead OOM [v2]

[Jaikiran Pai]

On Wed, 22 May 2024 07:27:14 GMT, Claes Redestad  wrote:

>> test/micro/org/openjdk/bench/java/util/zip/InflaterInputStreams.java line 
>> 113:
>> 
>>> 111: try (InflaterInputStream iis = new 
>>> InflaterInputStream(deflated)) {
>>> 112: while (iis.read(inflated, 0, inflated.length) != -1);
>>> 113: }
>> 
>> Presumably this only works because closing the underlying stream (a BAIS in 
>> this case) is a no-op.
>
> Yes, the underlying BAIS can be repeatedly closed without affecting the 
> benchmark.

Hello Alan, before this change, the memory gets occupied by too many of these:


   1:  10871820  521847360  
jdk.internal.ref.CleanerImpl$PhantomCleanableRef (java.base@23-internal)
   2:  10871758  260922192  java.util.zip.Inflater$InflaterZStreamRef 
(java.base@23-internal)



Closing the `InflaterInputStream` releases those underlying 
`InflaterZStreamRef`s.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19340#discussion_r1609461029

Re: RFR: 8332490: JMH org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead OOM [v2]

[Jaikiran Pai]

> Can I please get a review of this test-only change for addressing 
> https://bugs.openjdk.org/browse/JDK-8332490?
> 
> The jmh test opens a `InflaterInputStream`, reads the stream contents, but 
> then doesn't close the stream. This can lead to resource leak which can then 
> cause OOM as noted in that JBS issue.
> 
> The commit in this PR closes the `InflaterInputStream` when the reads 
> complete.
> 
> 
> make test TEST=micro:org.openjdk.bench.java.util.zip.InflaterInputStreams
> 
> and
> 
> 
> ./build/macosx-aarch64/images/jdk/bin/java -jar 
> ./build/macosx-aarch64/images/test/micro/benchmarks.jar 
> org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead 
> -t max -f 1 -wi 2 -foe true
> 
> pass after this change.

Jaikiran Pai has updated the pull request incrementally with one additional 
commit since the last revision:

  add a comment

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19340/files
  - new: https://git.openjdk.org/jdk/pull/19340/files/f8687c3f..42a2d32a

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=19340=01
 - incr: https://webrevs.openjdk.org/?repo=jdk=19340=00-01

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

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

RFR: 8332490: JMH org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead OOM

[Jaikiran Pai]

Can I please get a review of this test-only change for addressing 
https://bugs.openjdk.org/browse/JDK-8332490?

The jmh test opens a `InflaterInputStream`, reads the stream contents, but then 
doesn't close the stream. This can lead to resource leak which can then cause 
OOM as noted in that JBS issue.

The commit in this PR closes the `InflaterInputStream` when the reads complete.


make test TEST=micro:org.openjdk.bench.java.util.zip.InflaterInputStreams

and


./build/macosx-aarch64/images/jdk/bin/java -jar 
./build/macosx-aarch64/images/test/micro/benchmarks.jar 
org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead -t 
max -f 1 -wi 2 -foe true

pass after this change.

-

Commit messages:
 - 8332490: JMH 
org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead OOM

Changes: https://git.openjdk.org/jdk/pull/19340/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk=19340=00
  Issue: https://bugs.openjdk.org/browse/JDK-8332490
  Stats: 3 lines in 1 file changed: 1 ins; 0 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/19340.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19340/head:pull/19340

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

Re: RFR: 8331189: Implementation of Scoped Values (Third Preview)

[Jaikiran Pai]

On Wed, 8 May 2024 09:40:38 GMT, Alan Bateman  wrote:

> JEP 481 proposes Scoped Values to continue to preview in JDK 23 with one 
> change. The type of the operation parameter of the callWhere method is 
> changed to a new functional interface to avoid having the API throw 
> Exception. With that change, the getWhere (and the corresponding method on 
> Carrier) are no longer needed. The functional interface is not proposed for 
> j.u.function at this time.

The changes look OK to me.

-

Marked as reviewed by jpai (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19136#pullrequestreview-2068941921

Re: RFR: 8331189: Implementation of Scoped Values (Third Preview)

[Jaikiran Pai]

On Wed, 8 May 2024 09:40:38 GMT, Alan Bateman  wrote:

> JEP 481 proposes Scoped Values to continue to preview in JDK 23 with one 
> change. The type of the operation parameter of the callWhere method is 
> changed to a new functional interface to avoid having the API throw 
> Exception. With that change, the getWhere (and the corresponding method on 
> Carrier) are no longer needed. The functional interface is not proposed for 
> j.u.function at this time.

src/java.base/share/classes/java/lang/ScopedValue.java line 494:

> 492: 
> 493: /**
> 494:  * An operation that returns a result and may throw an exception.

Nit - should it be "or may throw an exception"?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19136#discussion_r1608511614

Re: RFR: 8332086: Remove the usage of ServiceLoader in j.u.r.RandomGeneratorFactory [v8]

[Jaikiran Pai]

On Tue, 21 May 2024 09:06:16 GMT, Raffaello Giulietti  
wrote:

>> All random number generator algorithms are implemented in module 
>> `java.base`. The usage of `ServiceLoader` in `j.u.r.RandomGeneratorFactory` 
>> is no longer needed.
>
> Raffaello Giulietti has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Prefer 'linkplain' to 'a href'.

Marked as reviewed by jpai (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19212#pullrequestreview-2068098589

Re: RFR: 8332495: java/util/logging/LoggingDeadlock2.java fails with AssertionError: Some tests failed [v2]

[Jaikiran Pai]

On Mon, 20 May 2024 07:25:12 GMT, Axel Boldt-Christmas  
wrote:

>> Improve ` java/util/logging/LoggingDeadlock2.java` stderr parsing by 
>> ignoring deprecated warnings. Testing non-generational ZGC requires the use 
>> of a deprecated option.
>
> Axel Boldt-Christmas has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Update copyright year

Marked as reviewed by jpai (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19298#pullrequestreview-2065553921

Re: RFR: 8332494: java/util/zip/EntryCount64k.java failing with java.lang.RuntimeException: '\\A\\Z' missing from stderr [v2]

[Jaikiran Pai]

On Mon, 20 May 2024 07:27:19 GMT, Axel Boldt-Christmas  
wrote:

>> Improve `java/util/zip/EntryCount64k.java` stderr parsing by ignoring 
>> deprecated warnings. Testing non-generational ZGC requires the use of a 
>> deprecated option.
>
> Axel Boldt-Christmas has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Update copyright year

Marked as reviewed by jpai (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19297#pullrequestreview-2065548780

Re: RFR: 8332494: java/util/zip/EntryCount64k.java failing with java.lang.RuntimeException: '\\A\\Z' missing from stderr

[Jaikiran Pai]

On Mon, 20 May 2024 06:41:45 GMT, Axel Boldt-Christmas  
wrote:

> Improve `java/util/zip/EntryCount64k.java` stderr parsing by ignoring 
> deprecated warnings. Testing non-generational ZGC requires the use of a 
> deprecated option.

Hello Axel, the change looks OK to me. I was going to suggest 
`stderrShouldMatch(pattern, boolean ignoreDeprecatedWarning)` as the new method 
instead of `stderrShouldMatchIgnoreDeprecatedWarnings`, but then I see that 
there are also methods with names like this in `OutputAnalyzer`. So this change 
is consistent with those.

`OutputAnalyzer.java` will need a copyright year update before integrating.

-

Marked as reviewed by jpai (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19297#pullrequestreview-2065518056

Re: RFR: 8332495: java/util/logging/LoggingDeadlock2.java fails with AssertionError: Some tests failed

[Jaikiran Pai]

On Mon, 20 May 2024 06:42:19 GMT, Axel Boldt-Christmas  
wrote:

> Improve ` java/util/logging/LoggingDeadlock2.java` stderr parsing by ignoring 
> deprecated warnings. Testing non-generational ZGC requires the use of a 
> deprecated option.

The change looks OK to me. The copyright year on the file will need an update.

-

Marked as reviewed by jpai (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19298#pullrequestreview-2065506460

Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v8]

[Jaikiran Pai]

On Fri, 17 May 2024 13:38:25 GMT, Maurizio Cimadamore  
wrote:

>> This PR implements [JEP 472](https://openjdk.org/jeps/472), by restricting 
>> the use of JNI in the following ways:
>> 
>> * `System::load` and `System::loadLibrary` are now restricted methods
>> * `Runtime::load` and `Runtime::loadLibrary` are now restricted methods
>> * binding a JNI `native` method declaration to a native implementation is 
>> now considered a restricted operation
>> 
>> This PR slightly changes the way in which the JDK deals with restricted 
>> methods, even for FFM API calls. In Java 22, the single 
>> `--enable-native-access` was used both to specify a set of modules for which 
>> native access should be allowed *and* to specify whether illegal native 
>> access (that is, native access occurring from a module not specified by 
>> `--enable-native-access`) should be treated as an error or a warning. More 
>> specifically, an error is only issued if the `--enable-native-access flag` 
>> is used at least once.
>> 
>> Here, a new flag is introduced, namely 
>> `illegal-native-access=allow/warn/deny`, which is used to specify what 
>> should happen when access to a restricted method and/or functionality is 
>> found outside the set of modules specified with `--enable-native-access`. 
>> The default policy is `warn`, but users can select `allow` to suppress the 
>> warnings, or `deny` to cause `IllegalCallerException` to be thrown. This 
>> aligns the treatment of restricted methods with other mechanisms, such as 
>> `--illegal-access` and the more recent `--sun-misc-unsafe-memory-access`.
>> 
>> Some changes were required in the package-info javadoc for 
>> `java.lang.foreign`, to reflect the changes in the command line flags 
>> described above.
>
> Maurizio Cimadamore has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Address review comments

Marked as reviewed by jpai (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19213#pullrequestreview-2064736036

Re: RFR: 8332086: Remove the usage of ServiceLoader in j.u.r.RandomGeneratorFactory [v7]

[Jaikiran Pai]

On Fri, 17 May 2024 08:28:15 GMT, Raffaello Giulietti  
wrote:

>> All random number generator algorithms are implemented in module 
>> `java.base`. The usage of `ServiceLoader` in `j.u.r.RandomGeneratorFactory` 
>> is no longer needed.
>
> Raffaello Giulietti has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   SecureRandom added to the table in package documentation.

Thank you Raffaello for fixing this as well as considering the review 
suggestions. The latest state of this PR in `880138d7` looks good to me. I've 
also reviewed the linked CSR and that too looks fine to me.

-

Marked as reviewed by jpai (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19212#pullrequestreview-2063525156

Re: RFR: 8332086: Remove the usage of ServiceLoader in j.u.r.RandomGeneratorFactory [v5]

[Jaikiran Pai]

On Thu, 16 May 2024 13:23:14 GMT, Raffaello Giulietti  
wrote:

>> Yes, I thought about this the other day but decided for a bit more 
>> conservative approach, relying on the annotation.
>> 
>> But I agree that, since the meta-information now resides in 
>> `RandomGeneratorProperties`, we might "migrate" the deprecation status here 
>> as well.
>
> Since the legacy generators are public classes, they can be publicly 
> deprecated, so in the last commit the `DEPRECATED` bit for them still relies 
> on the annotation, which IMO is the authoritative "source of truth".
> 
> For the 10 other algorithms, which are accessible only via 
> `RandomFactoryGenerator`, we can rely on the info in `RandomProperties`.

The deprecation process of algorithms is unclear - whether it should be tied 
with the `@Deprecation` of a class that provides that algorithm. We will have 
more clarity if/when we do deprecate these algorithms. The specification in its 
current form doesn't tie it to the `@Deprecation` annotation, which is a good 
thing. So your proposed change in this PR looks fine to me, since it will still 
allow us to move away from the `@Deprecated` annotation check if we wanted to, 
in a subsequent release/change.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19212#discussion_r1605058285

Re: RFR: 8173970: jar tool should have a way to extract to a directory [v6]

[Jaikiran Pai]

> Can I please get a review for this patch which proposes to implement the 
> enhancement request noted in https://bugs.openjdk.java.net/browse/JDK-8173970?
> 
> The commit in this PR introduces the `-o` and `--output-dir` option to the 
> `jar` command. The option takes a path to a destination directory as a value 
> and extracts the contents of the jar into that directory. This is an optional 
> option and the changes in the commit continue to maintain backward 
> compatibility where the jar is extracted into current directory, if no `-o` 
> or `--output-dir` option has been specified.
> 
> As far as I know, there hasn't been any discussion on what the name of this 
> new option should be. I was initially thinking of using `-d` but that is 
> currently used by the `jar` command for a different purpose. So I decided to 
> use `-o` and `--output-dir`. This is of course open for change depending on 
> any suggestions in this PR.
> 
> The commit in this PR also updates the `jar.properties` file which contains 
> the English version of the jar command's `--help` output. However, no changes 
> have been done to the internationalization files corresponding to this one 
> (for example: `jar_de.properties`), because I don't know what process needs 
> to be followed to have those files updated (if at all they need to be 
> updated).
> 
> The commit also includes a jtreg testcase which verifies the usage of this 
> new option.

Jaikiran Pai has updated the pull request with a new target base due to a merge 
or a rebase. The pull request now contains 16 commits:

 - merge latest from master branch
 - cleanup testExtractNoDestDirWithPFlag() test
 - merge latest from master branch
 - merge latest from master branch
 - convert the new test to junit
 - merge latest from master branch
 - Lance's review - include tests for --extract long form option
 - cleanup after each test
 - Adjust test for new error messages
 - Lance's review - add a code comment for xdestDir
 - ... and 6 more: https://git.openjdk.org/jdk/compare/9160ef8b...a8aeff60

-

Changes: https://git.openjdk.org/jdk/pull/2752/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk=2752=05
  Stats: 569 lines in 4 files changed: 558 ins; 0 del; 11 mod
  Patch: https://git.openjdk.org/jdk/pull/2752.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/2752/head:pull/2752

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

Re: RFR: 8173970: jar tool should have a way to extract to a directory [v5]

[Jaikiran Pai]

On Wed, 8 May 2024 05:37:24 GMT, Jaikiran Pai  wrote:

>> Can I please get a review for this patch which proposes to implement the 
>> enhancement request noted in 
>> https://bugs.openjdk.java.net/browse/JDK-8173970?
>> 
>> The commit in this PR introduces the `-o` and `--output-dir` option to the 
>> `jar` command. The option takes a path to a destination directory as a value 
>> and extracts the contents of the jar into that directory. This is an 
>> optional option and the changes in the commit continue to maintain backward 
>> compatibility where the jar is extracted into current directory, if no `-o` 
>> or `--output-dir` option has been specified.
>> 
>> As far as I know, there hasn't been any discussion on what the name of this 
>> new option should be. I was initially thinking of using `-d` but that is 
>> currently used by the `jar` command for a different purpose. So I decided to 
>> use `-o` and `--output-dir`. This is of course open for change depending on 
>> any suggestions in this PR.
>> 
>> The commit in this PR also updates the `jar.properties` file which contains 
>> the English version of the jar command's `--help` output. However, no 
>> changes have been done to the internationalization files corresponding to 
>> this one (for example: `jar_de.properties`), because I don't know what 
>> process needs to be followed to have those files updated (if at all they 
>> need to be updated).
>> 
>> The commit also includes a jtreg testcase which verifies the usage of this 
>> new option.
>
> Jaikiran Pai has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 15 commits:
> 
>  - cleanup testExtractNoDestDirWithPFlag() test
>  - merge latest from master branch
>  - merge latest from master branch
>  - convert the new test to junit
>  - merge latest from master branch
>  - Lance's review - include tests for --extract long form option
>  - cleanup after each test
>  - Adjust test for new error messages
>  - Lance's review - add a code comment for xdestDir
>  - Lance's review - updates to the help messages in jar.properties
>  - ... and 5 more: https://git.openjdk.org/jdk/compare/3b8227ba...46fb5a8e

I've also updated the CSR https://bugs.openjdk.org/browse/JDK-8264510 for 
review. That CSR would be the best place to get up to speed with this proposed 
enhancement and the semantics of the new option.

-

PR Comment: https://git.openjdk.org/jdk/pull/2752#issuecomment-2116836829

Re: RFR: 8173970: jar tool should have a way to extract to a directory [v5]

[Jaikiran Pai]

On Wed, 8 May 2024 05:37:24 GMT, Jaikiran Pai  wrote:

>> Can I please get a review for this patch which proposes to implement the 
>> enhancement request noted in 
>> https://bugs.openjdk.java.net/browse/JDK-8173970?
>> 
>> The commit in this PR introduces the `-o` and `--output-dir` option to the 
>> `jar` command. The option takes a path to a destination directory as a value 
>> and extracts the contents of the jar into that directory. This is an 
>> optional option and the changes in the commit continue to maintain backward 
>> compatibility where the jar is extracted into current directory, if no `-o` 
>> or `--output-dir` option has been specified.
>> 
>> As far as I know, there hasn't been any discussion on what the name of this 
>> new option should be. I was initially thinking of using `-d` but that is 
>> currently used by the `jar` command for a different purpose. So I decided to 
>> use `-o` and `--output-dir`. This is of course open for change depending on 
>> any suggestions in this PR.
>> 
>> The commit in this PR also updates the `jar.properties` file which contains 
>> the English version of the jar command's `--help` output. However, no 
>> changes have been done to the internationalization files corresponding to 
>> this one (for example: `jar_de.properties`), because I don't know what 
>> process needs to be followed to have those files updated (if at all they 
>> need to be updated).
>> 
>> The commit also includes a jtreg testcase which verifies the usage of this 
>> new option.
>
> Jaikiran Pai has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 15 commits:
> 
>  - cleanup testExtractNoDestDirWithPFlag() test
>  - merge latest from master branch
>  - merge latest from master branch
>  - convert the new test to junit
>  - merge latest from master branch
>  - Lance's review - include tests for --extract long form option
>  - cleanup after each test
>  - Adjust test for new error messages
>  - Lance's review - add a code comment for xdestDir
>  - Lance's review - updates to the help messages in jar.properties
>  - ... and 5 more: https://git.openjdk.org/jdk/compare/3b8227ba...46fb5a8e

Hello everyone, this was a PR that I had opened some years back to introduce an 
option for the `jar` command to extract the contents to a directory of choice. 
After evaluating the option name (discussion within this PR), we had settled on 
`--dir` as the option. For some reason (I don't recollect) I never took this PR 
to completion. I've now reopened this PR and merged with the latest master 
branch and run all the tests including the new ones introduced in this PR. I 
would like to take this to completion, so I request reviews on this.

-

PR Comment: https://git.openjdk.org/jdk/pull/2752#issuecomment-2116762815

Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v6]

[Jaikiran Pai]

On Thu, 16 May 2024 11:47:13 GMT, Maurizio Cimadamore  
wrote:

>> src/java.base/share/classes/sun/launcher/resources/launcher.properties line 
>> 72:
>> 
>>> 70: \  by code in modules for which native access is not 
>>> explicitly enabled.\n\
>>> 71: \   is one of "deny", "warn" or "allow".\n\
>>> 72: \  This option will be removed in a future release.\n\
>> 
>> Should this specify the current default value for this option if it isn't 
>> set?
>
> We already do, see
> https://github.com/openjdk/jdk/pull/19213/files/1c45e5d56c429205ab8185481bc1044a86ab3bc6#diff-d05029afe6aed86f860a10901114402f1f6af4fe1e4b46d883141ab1d2a527b8R582

This is slightly different from what we do in the other PR for unsafe memory 
access where we specify the default in the launcher's help text too 
https://github.com/openjdk/jdk/pull/19174/files#diff-799093930b698e97b23ead98c6496261af1e2e33ec7aa9261584870cbee8a5eaR219.

I don't have a strong opinion on this, I think either one is fine.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1603205279

Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v6]

[Jaikiran Pai]

On Wed, 15 May 2024 16:08:17 GMT, Maurizio Cimadamore  
wrote:

>> This PR implements [JEP 472](https://openjdk.org/jeps/472), by restricting 
>> the use of JNI in the following ways:
>> 
>> * `System::load` and `System::loadLibrary` are now restricted methods
>> * `Runtime::load` and `Runtime::loadLibrary` are now restricted methods
>> * binding a JNI `native` method declaration to a native implementation is 
>> now considered a restricted operation
>> 
>> This PR slightly changes the way in which the JDK deals with restricted 
>> methods, even for FFM API calls. In Java 22, the single 
>> `--enable-native-access` was used both to specify a set of modules for which 
>> native access should be allowed *and* to specify whether illegal native 
>> access (that is, native access occurring from a module not specified by 
>> `--enable-native-access`) should be treated as an error or a warning. More 
>> specifically, an error is only issued if the `--enable-native-access flag` 
>> is used at least once.
>> 
>> Here, a new flag is introduced, namely 
>> `illegal-native-access=allow/warn/deny`, which is used to specify what 
>> should happen when access to a restricted method and/or functionality is 
>> found outside the set of modules specified with `--enable-native-access`. 
>> The default policy is `warn`, but users can select `allow` to suppress the 
>> warnings, or `deny` to cause `IllegalCallerException` to be thrown. This 
>> aligns the treatment of restricted methods with other mechanisms, such as 
>> `--illegal-access` and the more recent `--sun-misc-unsafe-memory-access`.
>> 
>> Some changes were required in the package-info javadoc for 
>> `java.lang.foreign`, to reflect the changes in the command line flags 
>> described above.
>
> Maurizio Cimadamore has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Address review comment

Hello Maurizio, in the current mainline, we have code in `LauncherHelper` 
https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/sun/launcher/LauncherHelper.java#L636
 where we enable native access to all unnamed modules if an executable jar with 
`Enable-Native-Access: ALL-UNNAMED` manifest is being launched. For such 
executable jars, what is the expected semantics when the launch also explicitly 
has a `--enable-native-access=M1,M2` option. Something like:


java --enable-native-access=M1,M2 -jar foo.jar

where `foo.jar` has `Enable-Native-Access: ALL-UNNAMED` in its manifest.

-

PR Comment: https://git.openjdk.org/jdk/pull/19213#issuecomment-2115005638

Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v6]

[Jaikiran Pai]

On Wed, 15 May 2024 16:08:17 GMT, Maurizio Cimadamore  
wrote:

>> This PR implements [JEP 472](https://openjdk.org/jeps/472), by restricting 
>> the use of JNI in the following ways:
>> 
>> * `System::load` and `System::loadLibrary` are now restricted methods
>> * `Runtime::load` and `Runtime::loadLibrary` are now restricted methods
>> * binding a JNI `native` method declaration to a native implementation is 
>> now considered a restricted operation
>> 
>> This PR slightly changes the way in which the JDK deals with restricted 
>> methods, even for FFM API calls. In Java 22, the single 
>> `--enable-native-access` was used both to specify a set of modules for which 
>> native access should be allowed *and* to specify whether illegal native 
>> access (that is, native access occurring from a module not specified by 
>> `--enable-native-access`) should be treated as an error or a warning. More 
>> specifically, an error is only issued if the `--enable-native-access flag` 
>> is used at least once.
>> 
>> Here, a new flag is introduced, namely 
>> `illegal-native-access=allow/warn/deny`, which is used to specify what 
>> should happen when access to a restricted method and/or functionality is 
>> found outside the set of modules specified with `--enable-native-access`. 
>> The default policy is `warn`, but users can select `allow` to suppress the 
>> warnings, or `deny` to cause `IllegalCallerException` to be thrown. This 
>> aligns the treatment of restricted methods with other mechanisms, such as 
>> `--illegal-access` and the more recent `--sun-misc-unsafe-memory-access`.
>> 
>> Some changes were required in the package-info javadoc for 
>> `java.lang.foreign`, to reflect the changes in the command line flags 
>> described above.
>
> Maurizio Cimadamore has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Address review comment

src/java.base/share/classes/sun/launcher/resources/launcher.properties line 72:

> 70: \  by code in modules for which native access is not 
> explicitly enabled.\n\
> 71: \   is one of "deny", "warn" or "allow".\n\
> 72: \  This option will be removed in a future release.\n\

Should this specify the current default value for this option if it isn't set?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1603157916

Re: RFR: 8332086: Remove the usage of ServiceLoader in j.u.r.RandomGeneratorFactory [v5]

[Jaikiran Pai]

On Wed, 15 May 2024 09:51:23 GMT, Raffaello Giulietti  
wrote:

>> All random number generator algorithms are implemented in module 
>> `java.base`. The usage of `ServiceLoader` in `j.u.r.RandomGeneratorFactory` 
>> is no longer needed.
>
> Raffaello Giulietti has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Small documentation changes.

test/jdk/java/util/Random/RandomTestCoverage.java line 195:

> 193: }
> 194: 
> 195: static void coverDefaults() {

This test method appears to test the calls to `getDefault()` methods on 
`RandomGeneratorFactory` and `RandomGenerator` classes, but it isn't being 
called in the test. We should call this method from `main()` to have test 
coverage for those methods.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19212#discussion_r1603131062

Re: RFR: 8332086: Remove the usage of ServiceLoader in j.u.r.RandomGeneratorFactory [v5]

[Jaikiran Pai]

On Wed, 15 May 2024 09:51:23 GMT, Raffaello Giulietti  
wrote:

>> All random number generator algorithms are implemented in module 
>> `java.base`. The usage of `ServiceLoader` in `j.u.r.RandomGeneratorFactory` 
>> is no longer needed.
>
> Raffaello Giulietti has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Small documentation changes.

test/jdk/java/util/Random/RandomTestCoverage.java line 179:

> 177: try {
> 178: rng = factory.create(12345L);
> 179: } catch (UnsupportedOperationException ignore) {

I think now that we know for sure which algorithm instances don't support which 
type of seed value and since throwing `UnsupportedOperationException` is now a 
specification of the `create(...)` methods, we should probably do something 
like this:


diff --git a/test/jdk/java/util/Random/RandomTestCoverage.java 
b/test/jdk/java/util/Random/RandomTestCoverage.java
index be12d188198..6e5c36e13c3 100644
--- a/test/jdk/java/util/Random/RandomTestCoverage.java
+++ b/test/jdk/java/util/Random/RandomTestCoverage.java
@@ -171,8 +171,37 @@ static void coverFactory(RandomGeneratorFactory factory) {
 boolean isSplittable = factory.isSplittable();
 
 coverRandomGenerator(factory.create());
-coverRandomGenerator(factory.create(12345L));
-coverRandomGenerator(factory.create(new byte[] {1, 2, 3, 4, 5, 6, 7, 
8}));
+
+String algo = factory.name();
+// test create(long)
+switch (algo) {
+// SecureRandom doesn't have long constructors so we expect
+// UnsupportedOperationException
+case "SecureRandom" -> {
+try {
+factory.create(12345L);
+throw new 
AssertionError("RandomGeneratorFactory.create(long) was expected" +
+"to throw UnsupportedOperationException for " + 
algo + " but didn't");
+} catch (UnsupportedOperationException uoe) {
+// expected
+}
+}
+default -> coverRandomGenerator(factory.create(12345L));
+}
+// test create(byte[])
+switch (algo) {
+// these don't have byte[] constructors so we expect 
UnsupportedOperationException
+case "Random", "SplittableRandom" -> {
+try {
+factory.create(new byte[] {1, 2, 3, 4, 5, 6, 7, 8});
+throw new 
AssertionError("RandomGeneratorFactory.create(byte[]) was expected" +
+"to throw UnsupportedOperationException for " + 
algo + " but didn't");
+} catch (UnsupportedOperationException uoe) {
+// expected
+}
+}
+default -> coverRandomGenerator(factory.create(new byte[] {1, 2, 
3, 4, 5, 6, 7, 8}));
+}
 }

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19212#discussion_r1603123836

Re: RFR: 8332086: Remove the usage of ServiceLoader in j.u.r.RandomGeneratorFactory [v5]

[Jaikiran Pai]

On Wed, 15 May 2024 09:51:23 GMT, Raffaello Giulietti  
wrote:

>> All random number generator algorithms are implemented in module 
>> `java.base`. The usage of `ServiceLoader` in `j.u.r.RandomGeneratorFactory` 
>> is no longer needed.
>
> Raffaello Giulietti has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Small documentation changes.

src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line 
204:

> 202: new RandomGeneratorProperties(rgClass, name, group,
> 203: i, j, k, equidistribution,
> 204: flags | 
> (rgClass.isAnnotationPresent(Deprecated.class) ? DEPRECATED : 0)));

Hello Raffaello, this is the final remaining reflection usage and even this I 
think isn't required now that all the random generator implementations reside 
within java.base as an implementation detail.

I think we should just skip this annotation check here and set `DEPRECATED` bit 
on the `flags` to `0` for all implementations. When/if we do deprecate any of 
the random generators, we can just come here and switch that bit to on for the 
specific random generator when instantiating this `RandomGeneratorProperties` 
record. I had a brief look at the code and the documentation in 
`package-info.java` of `java/util/random` and we don't mention that we rely on 
the `@Deprecated`  annotation to determine whether an algorithm is deprecated. 
I think that's a good thing.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19212#discussion_r1603101449

Re: RFR: 8332101: Add an `@since` to `StandardOperation:REMOVE` in `jdk.dynalink`

[Jaikiran Pai]

On Sat, 11 May 2024 15:39:04 GMT, Nizar Benalla  wrote:

> Code cleanup, please review this simple change. I split my changes into 1 PR 
> per module to make reviewing simpler.
> To help with the review, this was added back in 
> https://bugs.openjdk.org/browse/JDK-8191905
> TIA

The change looks good to me. `StandardOperation.REMOVE` was introduced through 
https://bugs.openjdk.org/browse/JDK-8191905 which was a JDK 10 change, so 
matches with the proposed change in this PR.

-

Marked as reviewed by jpai (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19189#pullrequestreview-2060312959

Re: RFR: 8331670: Deprecate the Memory-Access Methods in sun.misc.Unsafe for Removal [v2]

[Jaikiran Pai]

On Thu, 16 May 2024 07:14:53 GMT, Alan Bateman  wrote:

>> This is the implementation changes for JEP 471.
>> 
>> The methods in sun.misc.Unsafe for on-heap and off-heap access are 
>> deprecated for removal. This means a removal warning at compile time. No 
>> methods have been removed. A deprecated message is added to each of the 
>> methods but unlikely to be seen as the JDK does not generate or publish the 
>> API docs for this class.
>> 
>> A new command line option --sun-misc-unsafe-memory-access=$value is 
>> introduced to allow or deny access to these methods. The default proposed 
>> for JDK 23 is "allow" so no change in behavior compared to JDK 22 or 
>> previous releases.
>> 
>> A new test is added to test the command line option settings. The existing 
>> micros for FFM that use Unsafe are updated to suppress the removal warning 
>> at compile time. A new micro is introduced with a small sample of methods to 
>> ensure the changes doesn't cause any perf regressions.
>> 
>> For now, the changes include the update to the man page for the "java" 
>> command. It might be that this has to be separated out so that it goes with 
>> other updates in the release.
>
> Alan Bateman has updated the pull request with a new target base due to a 
> merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains ten additional 
> commits since the last revision:
> 
>  - Add removal to DISABLED_WARNINGS
>Fail at startup if bad value specified to option
>  - Merge
>  - Update man page
>  - Update man page
>  - Fix comment
>  - Merge
>  - Merge
>  - Whitespace
>  - Initial commit

The changes in `bd74a21d` look good to me. I haven't paid close attention to 
the new benchmark.

-

Marked as reviewed by jpai (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19174#pullrequestreview-2059867530

Re: RFR: 8321622: ClassFile.verify(byte[] bytes) throws unexpected ConstantPoolException, IAE

[Jaikiran Pai]

On Thu, 16 May 2024 06:44:29 GMT, Adam Sotona  wrote:

> Class-File API `VerifierImpl` contains debugging stack trace print causing 
> unexpected output and confusion in specific verification cases.
> 
> This patch removes the stack trace print.
> 
> Please review.
> 
> Thanks,
> Adam

Hello Adam, the change looks OK to me. The file needs a copyright year update 
before integrating.

On an unrelated note, the class level javadoc has some `@see` usages which use 
URLs pointing to `https://raw.githubusercontent.com/openjdk/jdk...`. Are those 
intentional? Of course, this is an internal class so the javadoc won't be 
published.

-

Marked as reviewed by jpai (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19258#pullrequestreview-2059775369

Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base`

[Jaikiran Pai]

On Fri, 10 May 2024 01:25:45 GMT, xiaotaonan  wrote:

>> Clean up non-standard use of /// comments in `java.base`
>
> @mdinacci @hns  @landonf

Hello @xiaotaonan, like Jon noted in his comment, there's already another PR 
addressing this change. So I think this current PR can be closed.

-

PR Comment: https://git.openjdk.org/jdk/pull/19151#issuecomment-2113730374

Re: RFR: 8332086: Remove the usage of ServiceLoader in j.u.r.RandomGeneratorFactory [v4]

[Jaikiran Pai]

On Tue, 14 May 2024 16:08:22 GMT, Raffaello Giulietti  
wrote:

>> All random number generator algorithms are implemented in module 
>> `java.base`. The usage of `ServiceLoader` in `j.u.r.RandomGeneratorFactory` 
>> is no longer needed.
>
> Raffaello Giulietti has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Replace SimpleImmutableEntry constructor with Map.entry() factory method.

src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line 
626:

> 624:  * and providing a starting long seed.
> 625:  * If a long seed is not supported by the algorithm,
> 626:  * an {@link UnsupportedOperationException} is thrown.

Perhaps reword this and the other `create(byte[])` method to something like:


* Create an instance of {@link RandomGenerator} based on the
* algorithm chosen,
* and the provided {@code seed}.
* If the {@code RandomGenerator} doesn't support instantiation through
* a {@code seed} of type {@code long} then this method throws an
* an {@link UnsupportedOperationException}.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19212#discussion_r1601030273

Re: RFR: 8332086: Remove the usage of ServiceLoader in j.u.r.RandomGeneratorFactory [v4]

[Jaikiran Pai]

On Wed, 15 May 2024 06:30:35 GMT, Jaikiran Pai  wrote:

>> Raffaello Giulietti has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Replace SimpleImmutableEntry constructor with Map.entry() factory method.
>
> src/java.base/share/classes/java/util/random/package-info.java line 85:
> 
>> 83:  * Then one can choose a specific implementation by giving the name of a 
>> generator
>> 84:  * algorithm to the static method {@link RandomGenerator#of}, in which 
>> case no
>> 85:  * seed is specified by the caller:
> 
> Perhaps reword this to:
> 
> 
>> ... in which case a {@code RandomGenerator} is constructed without any seed 
>> value:

An already existing issue in the specification in this file is that, a few 
lines below, we note that:

>  There are three groups of random number generator algorithm provided in 
> Java: the Legacy group, the LXM group, and the Xoroshiro/Xoshiro group.

The "three" groups is misleading I think, since both later in the table as well 
as the `group()` method implementation on `RandomGeneratorFactory`, we return 
four distinct values "Legacy", "LXM", "Xoroshiro" and "Xoshiro". Should we 
reword this part of the documentation to remove the mention of "three"?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19212#discussion_r1601022606

Re: RFR: 8332086: Remove the usage of ServiceLoader in j.u.r.RandomGeneratorFactory [v4]

[Jaikiran Pai]

On Tue, 14 May 2024 16:08:22 GMT, Raffaello Giulietti  
wrote:

>> All random number generator algorithms are implemented in module 
>> `java.base`. The usage of `ServiceLoader` in `j.u.r.RandomGeneratorFactory` 
>> is no longer needed.
>
> Raffaello Giulietti has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Replace SimpleImmutableEntry constructor with Map.entry() factory method.

Thank you for these updates Raffaello. I think the implementation is looking 
much more simpler and cleaner now. I've a few documentation related review 
comments which I've added inline. I haven't reviewed the test change yet.

src/java.base/share/classes/java/util/random/package-info.java line 85:

> 83:  * Then one can choose a specific implementation by giving the name of a 
> generator
> 84:  * algorithm to the static method {@link RandomGenerator#of}, in which 
> case no
> 85:  * seed is specified by the caller:

Perhaps reword this to:


> ... in which case a {@code RandomGenerator} is constructed without any seed 
> value:

-

PR Comment: https://git.openjdk.org/jdk/pull/19212#issuecomment-2111679665
PR Review Comment: https://git.openjdk.org/jdk/pull/19212#discussion_r1601015731

Re: RFR: 8332260: Mark tools/jlink/JLinkReproducibleTest.java as intermittent failure

[Jaikiran Pai]

On Wed, 15 May 2024 03:28:29 GMT, SendaoYan  wrote:

> Hi all,
>   The `tools/jlink/JLinkReproducibleTest.java` intermittent fails on linux 
> aarch64. Should we mark this testcase as `@key intermittent`. No risk.
> 
> Thanks.
> -sendao

Hello @sendaoYan, the linked issue https://bugs.openjdk.org/browse/JDK-8327181 
which talks about a JVM crash looks very generic and not specific to this test. 
Before updating this test, I think that issue needs to investigated and 
addressed or at least we need to ascertain that this test is somehow the only 
test causing it.

-

PR Comment: https://git.openjdk.org/jdk/pull/19241#issuecomment-2111522183

Re: RFR: 8332086: Remove the usage of ServiceLoader in j.u.r.RandomGeneratorFactory

[Jaikiran Pai]

On Mon, 13 May 2024 13:54:18 GMT, Raffaello Giulietti  
wrote:

> Then I would even remove the double-checking idiom, the volatile on ctor and 
> properties, and declare methods getProperties() and ensureConstructors() as 
> synchronized.
> I'm not sure that the double-checking optimization brings much value on 
> contemporary JVMs.

Making the methods synchronized would bring in a penalty that there will always 
be a monitor entry at every call site, even after the `properites` and 
`ctor`(s) are initialized. Ideally, we should just do all of this intialization 
in the constructor of the `RandomGeneratorFactory`, the one which takes the 
`Class<>` type of the `RandomGenerator`. We can then make the `properties` and 
the `ctor`(s) all `final` and not have to worry about any synchronization or 
volatile semantics. You would of course have to rework the ensureConstructors 
to not throw an exception at that time.

> But I feel that the followup PR discussed before wouldn't need synchronized 
> at all.

Correct. The more I think about it, I think cleaning up all this in this PR 
itself might make both reviewing and the implementation a bit more simpler. 
What's your thoughts?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19212#discussion_r1598540564

Re: RFR: 8332086: Remove the usage of ServiceLoader in j.u.r.RandomGeneratorFactory

[Jaikiran Pai]

On Mon, 13 May 2024 08:47:50 GMT, Raffaello Giulietti  
wrote:

> All random number generator algorithms are implemented in module `java.base`. 
> The usage of `ServiceLoader` in `j.u.r.RandomGeneratorFactory` is no longer 
> needed.

src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line 
299:

> 297: private void ensureConstructors() {
> 298: if (ctor == null) {  // volatile load
> 299: synchronized (rgClass) {

Here too I think we should synchronize on `this` - we would want to allow 
multiple different instances of a `RandomGeneratorFactory` for the same 
`RandomGenerator` class type to be able to concurrently instantiate their 
individual instance fields (like the `ctor`(s) and `properties`).

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19212#discussion_r1598500425

Re: RFR: 8332086: Remove the usage of ServiceLoader in j.u.r.RandomGeneratorFactory

[Jaikiran Pai]

On Mon, 13 May 2024 08:47:50 GMT, Raffaello Giulietti  
wrote:

> All random number generator algorithms are implemented in module `java.base`. 
> The usage of `ServiceLoader` in `j.u.r.RandomGeneratorFactory` is no longer 
> needed.

src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line 
187:

> 185:  private RandomGeneratorProperties getProperties() {
> 186: if (properties == null) {  // volatile load
> 187: synchronized (rgClass) {

The synchronization on `rgClass` to intialize an instance field `properties` 
appears odd here. I think this should be synchronized on `this`.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19212#discussion_r1598485190

Re: RFR: 8332086: Remove the usage of ServiceLoader in j.u.r.RandomGeneratorFactory

[Jaikiran Pai]

On Mon, 13 May 2024 13:14:57 GMT, Raffaello Giulietti  
wrote:

>> Thinking a bit more, I think we can even get rid of the reflection used in 
>> `create()` methods implementation, if we wanted to, by doing something like 
>> this:
>> 
>> 
>> private record RandomGenEntry(Class 
>> randomGenClass, int i, int j,
>>   int k, int equiDistribution, boolean 
>> stochastic,
>>   boolean hardware) {
>> 
>> RandomGenerator create() {
>> String algo = randomGenClass.getSimpleName();
>> return switch (algo) {
>> case "Random" -> new Random();
>> case "L128X1024MixRandom" -> new L128X1024MixRandom();
>> case "Xoshiro256PlusPlus" -> new Xoshiro256PlusPlus();
>> // ... so on for the rest
>> default -> throw new InternalError("should not happen");
>> };
>> }
>> 
>> RandomGenerator create(long seed) {
>> String algo = randomGenClass.getSimpleName();
>> return switch (algo) {
>> case "Random", "SplittableRandom", "SecureRandom" -> {
>> throw new UnsupportedOperationException("cannot 
>> construct with a long seed");
>> }
>> case "L128X1024MixRandom" -> new L128X1024MixRandom(seed);
>> case "Xoshiro256PlusPlus" -> new Xoshiro256PlusPlus(seed);
>> // ... so on for the rest
>> default -> throw new InternalError("should not happen");
>> };
>> }
>> 
>> RandomGenerator create(byte[] seed) {
>> String algo = randomGenClass.getSimpleName();
>> return switch (algo) {
>> case "Random", "SplittableRandom", "SecureRandom" -> {
>> throw new UnsupportedOperationException("cannot 
>> construct with a byte[] seed");
>> }
>> case "L128X1024MixRandom" -> new L128X1024MixRandom(seed);
>> case "Xoshiro256PlusPlus" -> new Xoshiro256PlusPlus(seed);
>> // ... so on for the rest
>> default -> throw new InternalError("should not happen");
>> };
>> }
>> }
>> 
>> 
>> private static final Map FACTORY_MAP = ... // 
>> construct the map
>
> @jaikiran I agree that we can simplify even more. I just wanted to change as 
> little as possible in this PR to facilitate reviews.
> Shall I push your proposed changes in this PR or is a followup PR preferable?

A followup PR is fine. I think once this PR which addresses the API aspects 
(like removal of ServiceLoader and then updates to the create() method javadoc) 
is integrated, then the subsequent PR can just be all internal implementation 
changes like the proposed removal of reflection.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19212#discussion_r1598464086

Re: RFR: 8332086: Remove the usage of ServiceLoader in j.u.r.RandomGeneratorFactory

[Jaikiran Pai]

On Mon, 13 May 2024 12:45:59 GMT, Jaikiran Pai  wrote:

>> All random number generator algorithms are implemented in module 
>> `java.base`. The usage of `ServiceLoader` in `j.u.r.RandomGeneratorFactory` 
>> is no longer needed.
>
> src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line 
> 190:
> 
>> 188: if (properties == null) {  // double-checking idiom
>> 189: RandomGeneratorProperties props = 
>> rgClass.getDeclaredAnnotation(RandomGeneratorProperties.class);
>> 190: Objects.requireNonNull(props, rgClass + " missing 
>> annotation");
> 
> Hello Raffaello, with the `RandomGenerator` implementations now all residing 
> within the java.base module, I think an additional advantage of that is that, 
> it will now allow us to remove this internal `RandomGeneratorProperties` 
> annotation and thus this reflection code.
> 
> I think one way to do that would be something like this within this 
> `RandomGeneratorFactory` class itself: 
> 
> 
> private record RandomGenEntry(Class randomGenClass, int i, int j,
>   int k, int equiDistribution, boolean stochastic,
>   boolean hardware) {
> 
> }
> 
> private static final Map FACTORY_MAP = ... // 
> construct the map
> 
> 
> where the `FACTORY_MAP` will be keyed to the alogrithm and the value will be 
> a record which holds these additional details about the `RandomGenerator`. 
> This current PR is about getting rid of ServiceLoader usage. So if you want 
> to remove the usage of this annotation and reflection is a separate PR that's 
> fine with me. Furthermore, although I don't see the necessity of an 
> annotation for what we are doing here, if you think that the removal of the 
> annotation and reflection isn't worth doing, that is OK too.

Thinking a bit more, I think we can even get rid of the reflection used in 
`create()` methods implementation, if we wanted to, by doing something like 
this:


private record RandomGenEntry(Class randomGenClass, 
int i, int j,
  int k, int equiDistribution, boolean 
stochastic,
  boolean hardware) {

RandomGenerator create() {
String algo = randomGenClass.getSimpleName();
return switch (algo) {
case "Random" -> new Random();
case "L128X1024MixRandom" -> new L128X1024MixRandom();
case "Xoshiro256PlusPlus" -> new Xoshiro256PlusPlus();
// ... so on for the rest
default -> throw new InternalError("should not happen");
};
}

RandomGenerator create(long seed) {
String algo = randomGenClass.getSimpleName();
return switch (algo) {
case "Random", "SplittableRandom", "SecureRandom" -> {
throw new UnsupportedOperationException("cannot construct 
with a long seed");
}
case "L128X1024MixRandom" -> new L128X1024MixRandom(seed);
case "Xoshiro256PlusPlus" -> new Xoshiro256PlusPlus(seed);
// ... so on for the rest
default -> throw new InternalError("should not happen");
};
}

RandomGenerator create(byte[] seed) {
String algo = randomGenClass.getSimpleName();
return switch (algo) {
case "Random", "SplittableRandom", "SecureRandom" -> {
throw new UnsupportedOperationException("cannot construct 
with a byte[] seed");
}
case "L128X1024MixRandom" -> new L128X1024MixRandom(seed);
case "Xoshiro256PlusPlus" -> new Xoshiro256PlusPlus(seed);
// ... so on for the rest
default -> throw new InternalError("should not happen");
};
}
}


private static final Map FACTORY_MAP = ... // 
construct the map

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19212#discussion_r1598446741

Re: RFR: 8332086: Remove the usage of ServiceLoader in j.u.r.RandomGeneratorFactory

[Jaikiran Pai]

On Mon, 13 May 2024 08:47:50 GMT, Raffaello Giulietti  
wrote:

> All random number generator algorithms are implemented in module `java.base`. 
> The usage of `ServiceLoader` in `j.u.r.RandomGeneratorFactory` is no longer 
> needed.

src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line 
190:

> 188: if (properties == null) {  // double-checking idiom
> 189: RandomGeneratorProperties props = 
> rgClass.getDeclaredAnnotation(RandomGeneratorProperties.class);
> 190: Objects.requireNonNull(props, rgClass + " missing 
> annotation");

Hello Raffaello, with the `RandomGenerator` implementations now all residing 
within the java.base module, I think an additional advantage of that is that, 
it will now allow us to remove this internal `RandomGeneratorProperties` 
annotation and thus this reflection code.

I think one way to do that would be something like this within this 
`RandomGeneratorFactory` class itself: 


private record RandomGenEntry(Class randomGenClass, int i, int j,
  int k, int equiDistribution, boolean stochastic,
  boolean hardware) {

}

private static final Map FACTORY_MAP = ... // construct 
the map


where the `FACTORY_MAP` will be keyed to the alogrithm and the value will be a 
record which holds these additional details about the `RandomGenerator`. 
This current PR is about getting rid of ServiceLoader usage. So if you want to 
remove the usage of this annotation and reflection is a separate PR that's fine 
with me. Furthermore, although I don't see the necessity of an annotation for 
what we are doing here, if you think that the removal of the annotation and 
reflection isn't worth doing, that is OK too.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19212#discussion_r1598417990

Re: RFR: 8305457: Implement java.io.IO [v7]

[Jaikiran Pai]

On Fri, 10 May 2024 16:41:28 GMT, Naoto Sato  wrote:

>> When implementing, I asked myself if I must use any of those monitors and 
>> decided that I don't have to. My reasoning is below.
>> 
>> `readLock`:
>> 
>> - is used inside the object that `Reader reader` is initialised with, and
>> 
>> - it also guards fields such as `char[] rcb`, `boolean restoreEcho` and 
>> `boolean shutdownHookInstalled`.
>> 
>> Since `println` and `print` don't call methods on `reader` or access the 
>> above fields, they don't require `readLock`.
>> 
>> `writeLock`:
>> 
>> - is used inside objects that `Writer out` and `PrintWriter pw` are 
>> initialised with, and
>> - also in compound operations that first write and then immediately read. (I 
>> assume, it's so that no concurrent write could sneak in between writing and 
>> reading parts of such a compound.)
>> 
>> `println` or `print` don't call methods on `out` and certainly don't do any 
>> write-read compounds. That said, they call methods on `pw`. But `pw` uses 
>> `writeLock` internally. So in that sense we're covered. 
>> 
>> One potential concern is a write-write compound in `print`:
>> 
>> 
>> @Override
>> public JdkConsole print(Object obj) {
>> pw.print(obj);
>> pw.flush(); // automatic flushing does not cover print
>> return this;
>> }
>> 
>> 
>> I'm saying write-_write_, not write-_flush_, because as far as 
>> synchronisation is concerned,  `pw.flush()` should behave the same as 
>> `pw.print("")`.
>> 
>> While not using `writeLock` is not strictly correct, I believe the potential 
>> execution interleavings with other writes are benign. What's the worst that 
>> could happen? You flush more than you expected? Big deal!
>> 
>> Since we exhausted all the reasons to use `writeLock`, I don't think we need 
>> one.
>> 
>> 
>> 
>> Naoto has already reviewed this PR with only minor comments. While that 
>> increases my confidence in that the implementation is correct, it doesn't 
>> hurt to request re-review of this specific part: @naotoj, do you think I 
>> should use any of those monitors?
>
> I think your investigation is correct. As to the write-write case, there 
> already is the same pattern in (`formatter` basically utilizes `pw` 
> underneath) 
> 
> public JdkConsole format(String fmt, Object ... args) {
> formatter.format(fmt, args).flush();
> return this;
> }
> 
> So I think it is acceptable.

Thank you for that explanation, Pavel. I think the crucial detail happens to be:

> But pw uses writeLock internally. So in that sense we're covered.

As you note, the same instance of `writeLock` will get used internally by the 
`PrintWriter`, so I think the current version of this code is good and won't 
require additionally locking in the outer code.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1598268588

Re: RFR: 8305457: Implement java.io.IO [v9]

[Jaikiran Pai]

On Mon, 13 May 2024 09:56:35 GMT, Pavel Rappo  wrote:

>> Please review this PR which introduces the `java.io.IO` top-level class and 
>> three methods to `java.io.Console` for [Implicitly Declared Classes and 
>> Instance Main Methods (Third Preview)].
>> 
>> This PR has been obtained as `git merge --squash` of a now obsolete [draft 
>> PR].
>> 
>> [Implicitly Declared Classes and Instance Main Methods (Third Preview)]: 
>> https://bugs.openjdk.org/browse/JDK-8323335
>> [draft PR]: https://github.com/openjdk/jdk/pull/18921
>
> Pavel Rappo has updated the pull request with a new target base due to a 
> merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains 17 additional 
> commits since the last revision:
> 
>  - Escape prompt
>  - Merge branch 'master' into 8305457-Implement-java.io.IO
>  - Clarify input charset
>  - Make IO final
>  - Fix System.console().readln(null) in jshell
>
>Without it, jshell hangs on me. Will think of a test.
>  - Fix typo
>  - Merge branch 'master' into 8305457-Implement-java.io.IO
>  - Simplify output.exp
>  - Cover null prompt in input tests
>  - Make input test parametric
>  - ... and 7 more: https://git.openjdk.org/jdk/compare/ea3ed865...17100ab8

The latest updated state of this PR looks good to me.

-

Marked as reviewed by jpai (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19112#pullrequestreview-2052345767

Re: Enhance proxy support in java.net core libraries

[Jaikiran Pai]

Hello Alessandro,

I've moved this discussion to net-dev mailing list which is more 
relevant for this discussion (and Bcced core-libs-dev). If you haven't 
already subscribed to net-dev, then you can do it here 
https://mail.openjdk.org/mailman/listinfo/net-dev.


-Jaikiran

On 13/05/24 3:28 am, Alessandro Autiero wrote:
Hello, my name is Alessandro Autiero and I'd like to propose three 
enhancements for the java core libraries to better support proxies in 
network components of the JDK.


There are three classes in the java.net  package that 
have proxy support:


  * java.net.Socket
Introduced in Java 1.0, supports HTTP(S)/SOCKS proxies modelled by
java.net.Proxy through the java.net.Socket(java.net.Proxy) constructor
  * java.net.HttpURLConnection
Introduced in Java 1.1, supports HTTP(S)/SOCKS proxies modelled by
java.net.Proxy through the
java.net.URL#openConnection(java.net.Proxy) public method
  * java.net.HttpClient (Introduced in Java 11)
Introduced in Java 11, supports HTTP(S) proxies modelled by
java.net.ProxySelector through the public
proxy(java.net.ProxySelector) method in its builder or the default
java.net.ProxySelector, which can be set by calling
java.net.ProxySelector#setDefault(java.net.ProxySelector)

While most proxies provide support for both the HTTP and SOCKS scheme, 
considering that the older HTTP client API had support for both, 
developers might choose to use the older api, or to use an external 
one, if they need or want to provide support for this feature. A quick 
Google search for a recommendation on which Http Client to use on a 
modern Java version yields many results that list SOCKS support as a 
feature to keep in mind when making a choice. While this is not 
necessarily indicative of the average Java developer sentiment about 
the feature, I think that it should be considered, alongside a couple 
of issues that were opened on StackOverFlow 
 
asking about support for this feature. Accordingly, I propose adding 
support for SOCKS proxies in java.net.HttpClient. If the change is 
allowed, consider that the default java.net.ProxySelector is an 
instance of sun.net.spi.DefaultProxySelector, which supports SOCKS 
proxies, but this implementation cannot be initialized by the user as 
it's not exposed by the module system. Starting from Java 9, 
ProxySelector#of(InetSocketAddress) was introduced, which returns an 
instance of java.net.ProxySelector$StaticProxySelector 
, 
a static inner class of ProxySelector introduced in Java 9 which only 
implements support for HTTP(S) proxies. StaticProxySelector's 
constructor could be modified from 
StaticProxySelector(java.net.InetSocketAddress) to 
StaticProxySelector(java.net.Proxy$Type, java.net.InetSocketAddress) 
to initialize the java.net.Proxy instance with a specified proxy type 
instead of hard coding HTTP. Then we could overload the method 
ProxySelector#of(InetSocketAddress) with 
ProxySelector#of(java.net.Proxy$Type, InetSocketAddress) method which 
would invoke the constructor we defined earlier. This change would not 
be breaking as StaticProxySelector is package private, no public 
methods would be deleted and the default scheme would still be HTTP. 
jdk.internal.net.http.HttpRequestImpl uses the ProxySelector in its 
retrieveProxy method, but checks if the proxy is an HTTP proxy: this 
would need to be changed as well. Finally, considering that unlike 
HttpURLConnection, HttpClient doesn't delegate the underlying 
connection to java.net.Socket, the java.net.http module would need to 
be enhanced to support SOCKS authentication, which could take more effort.


Another observation that I've made is about authentication. If a proxy 
requires basic authentication, that is authentication through a 
username and optionally a password, a developer can implement the 
java.net.Authenticator class and override the 
getPasswordAuthentication method. While basic authentication is still 
the norm for most proxies, it's disabled by default in the JDK since 
Java 8. Though, it's possible to enable it by overriding the net 
properties jdk.http.auth.proxying.disabledSchemes and 
jdk.http.auth.tunneling.disabledSchemes using System.setProperty. I 
couldn't find an explanation about why this change was implemented, so 
I can only speculate that it was done to incentivize Java developers 
to use an IP whitelist instead of basic auth to improve security, 
assuming that the connection isn't secure(HTTP). The problem though is 
that the net properties that need to be changed to allow basic proxy 
authentication are only read only one time in the static initializer 
of sun.net.www.protocol.http.HttpURLConnection class, the underlying 
implementation of 

Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base`

[Jaikiran Pai]

On Fri, 10 May 2024 01:25:45 GMT, xiaotaonan  wrote:

>> Clean up non-standard use of /// comments in `java.base`
>
> @mdinacci @hns  @landonf

Hello @xiaotaonan, I see that you have several PRs of similar nature that have 
been opened in the past day or two. I would recommend taking a look at the 
OpenJDK developer's guide https://openjdk.org/guide/ (if you haven't already) 
to get familiar with the contribution guidelines. Additionally, if an issue has 
been assigned to someone, it's recommended that you ask the person if you can 
take over the issue before starting any work on it.
Finally, although not applicable to this specific PR, some of the other PRs you 
have open involve code changes that require jtreg test cases to be included and 
also making sure existing tests are run and continue to pass. The testing 
guidelines are available in the same guide 
https://openjdk.org/guide/#testing-the-jdk.

-

PR Comment: https://git.openjdk.org/jdk/pull/19151#issuecomment-2105551862

Re: RFR: 8332029: Provide access to initial value of stderr via JavaLangAccess

[Jaikiran Pai]

On Fri, 10 May 2024 07:57:40 GMT, Alan Bateman  wrote:

> In preparation for JEP 471 and JEP 472, provide access to the initial value 
> of System.err from JavaLangAccess. The initial value of System.in is already 
> exposed to code in java.base with this shared secret.

The change looks OK to me.

-

Marked as reviewed by jpai (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19171#pullrequestreview-2050182792

Re: RFR: 8332064: Implementation of Structured Concurrency (Third Preview)

[Jaikiran Pai]

On Fri, 10 May 2024 10:58:42 GMT, Alan Bateman  wrote:

> There aren't any API or implementations changes in third preview but the JEP 
> number/title needs to be bumped for the javadoc page.

Marked as reviewed by jpai (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19175#pullrequestreview-2050165151

Re: RFR: 8305457: Implement java.io.IO [v7]

[Jaikiran Pai]

On Thu, 9 May 2024 18:27:08 GMT, Pavel Rappo  wrote:

>> Please review this PR which introduces the `java.io.IO` top-level class and 
>> three methods to `java.io.Console` for [Implicitly Declared Classes and 
>> Instance Main Methods (Third Preview)].
>> 
>> This PR has been obtained as `git merge --squash` of a now obsolete [draft 
>> PR].
>> 
>> [Implicitly Declared Classes and Instance Main Methods (Third Preview)]: 
>> https://bugs.openjdk.org/browse/JDK-8323335
>> [draft PR]: https://github.com/openjdk/jdk/pull/18921
>
> Pavel Rappo has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fix System.console().readln(null) in jshell
>   
>   Without it, jshell hangs on me. Will think of a test.

src/java.base/share/classes/jdk/internal/io/JdkConsoleImpl.java line 61:

> 59: @Override
> 60: public JdkConsole println(Object obj) {
> 61: pw.println(obj);

Are these `println(...)` and `print(...)` methods intentionally not using a 
`writeLock` unlike the `readln(...)` and `readLine(...)` methods which do use 
(read and write) locks?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1596392802

Re: RFR: 8305457: Implement java.io.IO [v4]

[Jaikiran Pai]

On Wed, 8 May 2024 18:43:41 GMT, Pavel Rappo  wrote:

>> Please review this PR which introduces the `java.io.IO` top-level class and 
>> three methods to `java.io.Console` for [Implicitly Declared Classes and 
>> Instance Main Methods (Third Preview)].
>> 
>> This PR has been obtained as `git merge --squash` of a now obsolete [draft 
>> PR].
>> 
>> [Implicitly Declared Classes and Instance Main Methods (Third Preview)]: 
>> https://bugs.openjdk.org/browse/JDK-8323335
>> [draft PR]: https://github.com/openjdk/jdk/pull/18921
>
> Pavel Rappo has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Specify charset of java.io.IO
>  - Use system-dependent line separator for Console.println

src/java.base/share/classes/java/io/IO.java line 38:

> 36:  * had been called on that console.
> 37:  *
> 38:  *  Output from methods in this class uses the character set of the 
> system

Hello Pavel, given that `java.io.IO` exposes both input and output methods and 
the Charset will be used by both input and output, I think we might have to 
reword this sentence to make a note of input too.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1596380347

Re: Status of ZIP64 InputStream bug JDK-8298530?

[Jaikiran Pai]

I think this might have been addressed in mainline through 
https://bugs.openjdk.org/browse/JDK-8303866


-Jaikiran

On 09/05/24 11:39 pm, Archie Cobbs wrote:
On Thu, May 9, 2024 at 12:43 PM Bernd Eckenfels 
 wrote:


But this one here is still open:

* [JDK-8298530] ZipInputStream.readEnd fails for certain ZIP64
archives with small files - Java Bug System (openjdk.org)


FWIW when I run the test case in that bug on the latest JDK 23 
(jdk-23+21-110-g0a4eeeaa3c6), it does not report any exception.


So this bug may have gotten fixed by recent changes and just needs to 
be closed... ?


-Archie

--
Archie L. Cobbs

Re: RFR: 8321274: Rename ZipEntry.extraAttributes to ZipEntry.externalFileAttributes [v4]

[Jaikiran Pai]

On Fri, 8 Mar 2024 09:34:05 GMT, Eirik Bjørsnøs  wrote:

>> Please consider this PR which suggests we rename `ZipEntry.extraAttributes` 
>> to `ZipEntry.externalFileAttributes`.
>> 
>> This field was introduced in 
>> [JDK-8218021](https://bugs.openjdk.org/browse/JDK-8218021), originally under 
>> the name `ZipEntry.posixPerms`. 
>> [JDK-8250968](https://bugs.openjdk.org/browse/JDK-8250968) later renamed the 
>> field to `ZipEntry.extraAttributes` and extended its semantics to hold the 
>> full two-byte value of the `external file attributes` field, as defined by 
>> `APPNOTE.TXT`
>> 
>> The name `extraAttributes` is misleading. It has nothing to do with the 
>> `extra field` (an unrelated structure defined in `APPNOTE.TXT`), although 
>> the name indicates it does.
>> 
>> To prevent confusion and make life easier for future maintainers, I suggest 
>> we rename this field to `ZipEntry.externalFileAttributes` and update related 
>> methods, parameters and comments accordingly.
>> 
>> While this change is a straightforward renaming, reviewers should consider 
>> whether it carries its weight, especially considering it might complicate 
>> future backports. 
>> 
>> As a note to reviewers, this PR includes the following intended updates:
>> 
>> - Rename `ZipEntry.extraAttributes` and any references to this field to 
>> `ZipEntry.externalFileAttributes`
>> - Update `JavaUtilZipFileAccess` to similarly rename methods to 
>> `setExternalFileAttributes` and `getExternalFileAttributes` 
>> - Rename the field `JarSigner.extraAttrsDetected` to 
>> `JarSigner.externalFileAttrsDetected`
>> - Rename a local variable in `JarSigner.writeEntry` to 
>> `externalFileAttributes`
>> - Rename `s.s.t.jarsigner.Main.extraAttrsDetected` to 
>> `externalFileAttributesDetected`
>> - Rename resource string key names in `s.s.t.jarsigner.Resources` from 
>> `extra.attributes.detected` to `external.file.attributes.detected`
>> - Rename method `SymlinkTest.verifyExtraAttrs` to 
>> `verifyExternalFileAttributes`, also updated two references to 'extra 
>> attributes' in this method
>> - Updated copyright in all affected files
>> 
>> If the resource file changes should be dropped and instead handled via 
>> `msgdop` updates, let me know and I can revert the non-default files.
>> 
>> I did a search across the code base to find 'extraAttrs', 'extra.attr' after 
>> these updates, and found nothing related to zip/jar. The `zip` and `jar` 
>> tests run clean:
>> 
>> 
>> make test TEST="test/jdk/java/util/jar"
>> make test TEST="test/jdk/java/util/zip"
>
> Eirik Bjørsnøs has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Rename SymlinkTest.verifyExternalAttrs to verifyExternalFileAttributes

Hello Eirik, it will be better to merge with the latest master branch and run 
the tier tests before integrating.

-

PR Comment: https://git.openjdk.org/jdk/pull/16952#issuecomment-2102527651

Re: RFR: 8305457: Implement java.io.IO [v3]

[Jaikiran Pai]

On Wed, 8 May 2024 08:59:18 GMT, Pavel Rappo  wrote:

>> src/java.base/share/classes/java/io/Console.java line 192:
>> 
>>> 190:  *
>>> 191:  * @param  prompt
>>> 192:  * A prompt string.
>> 
>> Hello Pavel, should this specify whether `prompt`  can be null?
>
> If we specify that, it would be very much unlike all other `Console` methods 
> that are covered by this:
> 
> * Unless otherwise specified, passing a {@code null} argument to any 
> method
> * in this class will cause a {@link NullPointerException} to be thrown.

I haven't given it a try, but a brief look at the code suggests that if the 
console implementation backed by JLine gets used, then a `null` prompt may not 
generate a `NullPointerException` (since JLine appears to allow `null`) whereas 
if the internal JDK console implementation gets used then a 
`NullPointerException` will get thrown. If the expectation is to disallow a 
`null` prompt, then perhaps `Objects.requireNonNull(prompt)` before we hand off 
to the underlying console implementations might be required.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1593809280