8357993: Use "stdin.encoding" for reading System.in with InputStreamReader/Scanner [hotspot]

[vy]

Passes the Charset read from the stdin.encoding system property while creating InputStreamReader or Scanner instances for System.in.

stdin.encoding is a recently added property for Java 25 in JDK-8350703. Employing it throughout the entire code base is addressed by the parent ticket JDK-8356893. JDK-8357993 this PR is addressing is a sub-task of JDK-8356893 and is concerned with only areas related to Hotspot.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8357993: Use "stdin.encoding" for reading System.in with InputStreamReader/Scanner [hotspot] (Sub-task - P4)

Reviewers

• Chris Plummer (@plummercj - Reviewer)
• Serguei Spitsyn (@sspitsyn - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25542/head:pull/25542
$ git checkout pull/25542

Update a local copy of the PR:
$ git checkout pull/25542
$ git pull https://git.openjdk.org/jdk.git pull/25542/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25542

View PR using the GUI difftool:
$ git pr show -t 25542

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25542.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back vyazici! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@vy This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8357993: Use "stdin.encoding" for reading System.in with InputStreamReader/Scanner [hotspot]

Reviewed-by: cjplummer, sspitsyn

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 196 new commits pushed to the master branch:

• 62fde68: 8357396: Refactor nmethod::make_not_entrant to use Enum instead of "const char*"
• af87035: 8355746: Start of release updates for JDK 26
• c59e44a: 8357914: TestEmptyBootstrapMethodsAttr.java fails when run with TEST_THREAD_FACTORY=Virtual
• ... and 193 more: https://git.openjdk.org/jdk/compare/1a65719ab336336f94777df85bafeeef2c4d5b56...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[openjdk]

@vy The following labels will be automatically applied to this pull request:

• hotspot-jfr
• serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 01: Full - Incremental (45bdc4fd)
• 00: Full (d7751294)

[plummercj]

Changes look good. Can you clarify your testing?

[vy]

Can you clarify your testing?

@plummercj, green tier1,2 results on 45bdc4f are attached to the ticket. Would they suffice?

[AlanBateman]

/reviewers 2

[openjdk]

@AlanBateman
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

[AlanBateman]

/reviewers 1

[openjdk]

@AlanBateman
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 1 (with at least 1 Reviewer).

[plummercj]

Can you clarify your testing?

@plummercj, green tier1,2 results on 45bdc4f are attached to the ticket. Would they suffice?

That's not going to be sufficient. The nsk tests are not being run until tier5.

[vy]

That's not going to be sufficient. The nsk tests are not being run until tier5.

@plummercj, I've attached the tier3,4,5 run report to the ticket, would you mind reviewing them, please?

[plummercj]

That's not going to be sufficient. The nsk tests are not being run until tier5.

@plummercj, I've attached the tier3,4,5 run report to the ticket, would you mind reviewing them, please?

Looks good.

[sspitsyn]

Looks okay to me.

[vy]

@AlanBateman @plummercj @sspitsyn Thanks so much for taking time to review the changes, much appreciated. 🙇

/integrate

[openjdk]

Going to push as commit bb2611a.
Since your change was applied there have been 203 commits pushed to the master branch:

• e918a59: 8357821: Revert incorrectly named JavaLangAccess::unchecked* methods
• 28acca6: 8358653: [s390] Clean up comments regarding frame manager
• 029e3bf: 8349914: ZipFile::entries and ZipFile::getInputStream not consistent with each other when there are duplicate entries
• ... and 200 more: https://git.openjdk.org/jdk/compare/1a65719ab336336f94777df85bafeeef2c4d5b56...master

Your commit was automatically rebased without conflicts.

[openjdk]

@vy Pushed as commit bb2611a.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

[AlanBateman]

@AlanBateman @plummercj @sspitsyn Thanks so much for taking time to review the changes, much appreciated.

Okay, but I'm still puzzled as to why the tests were changed as they don't ready from the console.

[vy]

Okay, but I'm still puzzled as to why the tests were changed as they don't ready from the console.

@AlanBateman, I only skipped the practice of passing stdin.encoding to InputStreamReader/Scanner ctors whenever it is obvious that the System.in referred there is not the console. Do you imply I was mistaken while assessing BindServer and attach010Agent00? If so, which ones, and [to learn what I missed] why? I can create a follow-up ticket+PR to amend them.

[AlanBateman]

@AlanBateman, I only skipped the practice of passing stdin.encoding to InputStreamReader/Scanner ctors whenever it is obvious that the System.in referred there is not the console. Do you imply I was mistaken while assessing BindServer and attach010Agent00? If so, which ones, and [to learn what I missed] why? I can create a follow-up ticket+PR to amend them.

Tests are automated and very rare to have tests read from an interactive console. A small number of test uses "expect" for testing interactive input. There might be tests that have stdin redirected from a file or something else.

For multi VM tests (very common in the test suite) then you typically see the parent process using Process/ProcessBuilder and starting a child VM with its standard streams connected to the parent, meaning reading from System.in will read from a pipe connected to the parent. Some tests do inherit but I doubt we have any tests that inherit and also read from an interactive console.

So my point is that we probably don't need to jump on all tests that use System.in. The tests that do read from System.in probably need closer examination to see if they are reading from a pipe or something else.

[vy]

For multi VM tests (very common in the test suite) then you typically see the parent process using Process/ProcessBuilder and starting a child VM with its standard streams connected to the parent, meaning reading from System.in will read from a pipe connected to the parent. Some tests do inherit but I doubt we have any tests that inherit and also read from an interactive console.

So my point is that we probably don't need to jump on all tests that use System.in.

I think 😅 I see your point, and I agree with it. I've dropped several of my earlier changes from the abandoned parent #25368 – this applies to all subsequent tickets: #25544 (core), #25541 (tools), and this one. When the subject code is called in a Process, or the stream is obtained from a URL or a ClassLoader, ..., when it is "obvious" that the stream doesn't refer to the console, I've skipped the stdin.encoding refactoring. That said, "obvious" is very subjective and I might have misjudged certain usages.

The tests that do read from System.in probably need closer examination to see if they are reading from a pipe or something else.

In all stdin.encoding PRs – #25544 (core), #25541 (tools), and this one – I've examined usages before introducing the changes. I did my best, but I cannot claim it was exhaustive. That said, if it requires a significant effort to deduce that the console is not involved in a System.in usage, I'd prefer to include the stdin.encoding best-practice. There is a considerable likelihood that a later change could inadvertently violate that subtle assumption.

@AlanBateman, if you think a particular change needs closer examination or needs to be reverted, please say so. I'd be more than happy to carry out that task.

[plummercj]

BindServer.java is not used. It can be removed.

The changes in attach010Agent00.java are unnecessary because System.in has been set to read from a file:

35 private static final String inStreamFileName = "AttachOnDemand.attach010.in";
60 FileInputStream newInputStream = new FileInputStream(inStreamFileName);
61 System.setIn(newInputStream);

[vy]

@plummercj, shall I create a ticket+PR to

1. remove BindServer.java, and
2. revert changes to attach010Agent00.java?

[plummercj]

@plummercj, shall I create a ticket+PR to

1. remove `BindServer.java`, and

2. revert changes to `attach010Agent00.java`?

Yes. It might be better to make them 2 PRs.

[vy]

I've created

• 8359167: Remove unused test/hotspot/jtreg/vmTestbase/nsk/share/jpda/BindServer.java
• 8359168: Revert stdin.encoding usage in test/hotspot/jtreg/vmTestbase/nsk/jvmti/AttachOnDemand/attach010/attach010Agent00.java

Used hotspot and test as Component/s and Subcomponent, respectively – hope that is okay. Feel free to update the tickets as you see fit. I will try to pick them up sometime this week.

@plummercj, @AlanBateman, thank you for your assistance and patience. 🙇