-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8361613: System.console() should only be available for interactive terminal #26273
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
8361613: System.console() should only be available for interactive terminal #26273
Conversation
👋 Welcome back naoto! A progress list of the required criteria for merging this PR into |
❗ This change is not yet ready to be integrated. |
Webrevs
|
Co-authored-by: Andrey Turbanov <[email protected]>
@ValueSource(strings = {"UTF-8", "ISO-8859-1", "US-ASCII", "foo", ""}) | ||
void testDefaultCharset(String stdoutEncoding) throws Exception { | ||
// check "expect" command availability | ||
var expect = Paths.get("/usr/bin/expect"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only need to check "expect" availability once, so we should move this check to a @BeforeAll
static method. It's also more clear that this check is a precondition, and not part of the actual test. Applies to the other locations, but primarily the other parameterized tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Modified to use @BeforeAll
. For ModuleSelectionTest
, one of the test is not using expect
, so I left it as it is. In addition to that, I removed the @requires
condition to allow that test to run on windows.
|
||
@ParameterizedTest | ||
@MethodSource("options") | ||
void testWithoutExpect(String opts, String expected) throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expected
param looks unused.
|
||
@ParameterizedTest | ||
@MethodSource("options") | ||
void testWithExpect(String opts, String expected) throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be more clear if these tests were renamed to expectConsoleTest
and noConsoleTest
or something along those lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, using expect
command with junit's expect is confusing 🙂. Just renamed those methods using TTY/NonTTY, as"expect/noConsoleTest" reads somewhat odd as it includes the expected results in the test name.
* @requires (os.family == "linux") | (os.family == "mac") | ||
* @library /test/lib | ||
* @build jdk.test.lib.Utils | ||
* jdk.test.lib.JDKToolFinder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is jdk.test.lib.JDKToolFinder
needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, although additional review from core libs might be desirable. Thanks!
* with the current Java virtual machine, if any. | ||
* | ||
* @return The system console, if any, otherwise {@code null}. | ||
* @see Console |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method declaration already links to Console so I don't think we need another link in the "See also" section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I missed it, but do we have anything to make it clear that it returns null if either stdin or stdout are redirected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we do have wordings like " If the virtual machine is started from an interactive command line without redirecting the standard input AND output streams then its console will exist ..." and "If no console device is
available then an invocation of that method will return null" from the very beginning. not very "straightforward" but i think it's clear enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think adding @see
tag would be more helpful.
As to the spec wording wrt stdin/out, there is another issue filed to make it clearer: JDK-8361972. This PR addresses the implementation part only so that it can be backported to prior LTSes without spec change.
In prior JDK releases,
System.console()
could return aConsole
instance even when the JVM was not attached to an interactive terminal. This could lead to confusion, particularly when input was not from a keyboard or output was redirected, such as to or from a file or pipe, especially when using methods likereadPassword()
. Starting with JDK 25, the default behavior has changed:System.console()
now returnsnull
if standard input and/or output is redirected. However, if a JLine-based Console implementation is explicitly specified via the system property-Djdk.console=jdk.internal.le
, the previous behavior may still occur.This PR aims to align the behavior of the JLine-based
Console
implementation with the defaultSystem.console()
behavior. The actual code change is a one-liner inJdkConsoleProviderImpl.java
; the rest of the changes are adjustments to test cases to reflect the updated behavior. A corresponding CSR has also been drafted.Progress
Issues
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26273/head:pull/26273
$ git checkout pull/26273
Update a local copy of the PR:
$ git checkout pull/26273
$ git pull https://git.openjdk.org/jdk.git pull/26273/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 26273
View PR using the GUI difftool:
$ git pr show -t 26273
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26273.diff
Using Webrev
Link to Webrev Comment