-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8356985: Use "stdin.encoding" in Console's read*() methods #25271
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
Changes from 4 commits
3113ad8
f450bc6
f2eb1ad
ec42065
dd37076
ce1c803
75fa0a8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,18 +49,18 @@ public class JdkConsoleProviderImpl implements JdkConsoleProvider { | |
| * {@inheritDoc} | ||
| */ | ||
| @Override | ||
| public JdkConsole console(boolean isTTY, Charset charset) { | ||
| return new LazyDelegatingJdkConsoleImpl(charset); | ||
| public JdkConsole console(boolean isTTY, Charset inCharset, Charset outCharset) { | ||
| return new LazyDelegatingJdkConsoleImpl(inCharset, outCharset); | ||
| } | ||
|
|
||
| private static class LazyDelegatingJdkConsoleImpl implements JdkConsole { | ||
| private final Charset charset; | ||
| private final Charset outCharset; | ||
| private volatile boolean jlineInitialized; | ||
| private volatile JdkConsole delegate; | ||
|
|
||
| public LazyDelegatingJdkConsoleImpl(Charset charset) { | ||
| this.charset = charset; | ||
| this.delegate = new jdk.internal.io.JdkConsoleImpl(charset); | ||
| public LazyDelegatingJdkConsoleImpl(Charset inCharset, Charset outCharset) { | ||
| this.outCharset = outCharset; | ||
| this.delegate = new jdk.internal.io.JdkConsoleImpl(inCharset, outCharset); | ||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -130,7 +130,7 @@ public void flush() { | |
|
|
||
| @Override | ||
| public Charset charset() { | ||
| return charset; | ||
| return outCharset; | ||
| } | ||
|
|
||
| private void flushOldDelegateIfNeeded(JdkConsole oldDelegate) { | ||
|
|
@@ -157,7 +157,7 @@ private synchronized JdkConsole initializeJLineDelegate() { | |
| } | ||
|
|
||
| try { | ||
| Terminal terminal = TerminalBuilder.builder().encoding(charset) | ||
| Terminal terminal = TerminalBuilder.builder().encoding(outCharset) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't ideally
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also noticed
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. JLine is a 3rd party library. It would be desirable that they change their implementation to separately handle in/out in their terminal, but that is out of scope of this PR
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Created jline/jline3#1282.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks!
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. JLine seems to incorporate the stdin encoding already, which is quick! |
||
| .exec(false) | ||
| .nativeSignals(false) | ||
| .systemOutput(SystemOutput.SysOut) | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Copyright year update is missing. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,94 @@ | ||
| /* | ||
| * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved. | ||
| * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. | ||
| * | ||
| * This code is free software; you can redistribute it and/or modify it | ||
| * under the terms of the GNU General Public License version 2 only, as | ||
| * published by the Free Software Foundation. | ||
| * | ||
| * This code is distributed in the hope that it will be useful, but WITHOUT | ||
| * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or | ||
| * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License | ||
| * version 2 for more details (a copy is included in the LICENSE file that | ||
| * accompanied this code). | ||
| * | ||
| * You should have received a copy of the GNU General Public License version | ||
| * 2 along with this work; if not, write to the Free Software Foundation, | ||
| * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. | ||
| * | ||
| * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA | ||
| * or visit www.oracle.com if you need additional information or have any | ||
| * questions. | ||
| */ | ||
|
|
||
| import jdk.test.lib.process.OutputAnalyzer; | ||
| import jdk.test.lib.process.ProcessTools; | ||
|
|
||
| import java.io.BufferedReader; | ||
| import java.nio.file.Files; | ||
| import java.nio.file.Paths; | ||
|
|
||
| import org.junit.jupiter.api.Assumptions; | ||
| import org.junit.jupiter.api.Test; | ||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
|
|
||
| /** | ||
| * @test | ||
| * @bug 8356985 | ||
| * @summary Tests if "stdin.encoding" is reflected for reading | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would be helpful to include why the test is limited to only linux and mac,
|
||
| * the console. | ||
| * @requires (os.family == "linux" | os.family == "mac") | ||
| * @library /test/lib | ||
| * @build csp/* | ||
| * @run junit StdinEncodingTest | ||
| */ | ||
| public class StdinEncodingTest { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAICT, there is no similar test (e.g., one using a mock
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed there are For the record, AFAICT, there are no tests for 1 There is
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually providing mock charset was a workaround of not having public method for getting the input encoding. I think it would be an overkill to introduce a new public method because it will not be used much, as most cases are suffice with the existing one (Console is used for interactive user enviornment, and I don't believe users would like to see different characters displayed for the input).
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It is addressed in this PR
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the JBS bug ID needs to be added to
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bugid added to the test. Additionally replaced testsrc/jdk/classes with Utils ones |
||
|
|
||
| @Test | ||
| public void testStdinEncoding() throws Throwable { | ||
| // check "expect" command availability | ||
| var expect = Paths.get("/usr/bin/expect"); | ||
| if (!Files.exists(expect) || !Files.isExecutable(expect)) { | ||
|
||
| Assumptions.abort("'" + expect + "' not found"); | ||
| } | ||
|
|
||
| // invoking "expect" command | ||
| var testSrc = System.getProperty("test.src", "."); | ||
|
||
| var testClasses = System.getProperty("test.classes", "."); | ||
| var jdkDir = System.getProperty("test.jdk"); | ||
| OutputAnalyzer output = ProcessTools.executeProcess( | ||
| "expect", | ||
| "-n", | ||
| testSrc + "/stdinEncoding.exp", | ||
| jdkDir + "/bin/java", | ||
| "--module-path", | ||
| testClasses + "/modules", | ||
| "-Dstdin.encoding=Uppercasing", // <- gist of this test | ||
| "StdinEncodingTest"); | ||
| output.reportDiagnosticSummary(); | ||
| var eval = output.getExitValue(); | ||
| assertEquals(0, eval, "Test failed. Exit value from 'expect' command: " + eval); | ||
| } | ||
|
|
||
| public static void main(String... args) throws Throwable { | ||
| // check stdin.encoding | ||
| if (!"Uppercasing".equals(System.getProperty("stdin.encoding"))) { | ||
| throw new RuntimeException("Uppercasing charset was not set in stdin.encoding"); | ||
| } | ||
| var con = System.console(); | ||
|
|
||
| // Console.readLine() | ||
| System.out.print(con.readLine()); | ||
|
|
||
| // Console.readPassword() | ||
| System.out.print(String.valueOf(con.readPassword())); | ||
|
|
||
| // Console.reader() | ||
| try (var br = new BufferedReader(con.reader())) { | ||
| System.out.print(br.readLine()); | ||
| } | ||
|
|
||
| // Wait till the test receives the result | ||
| con.readLine(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| /* | ||
| * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved. | ||
| * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. | ||
| * | ||
| * This code is free software; you can redistribute it and/or modify it | ||
| * under the terms of the GNU General Public License version 2 only, as | ||
| * published by the Free Software Foundation. | ||
| * | ||
| * This code is distributed in the hope that it will be useful, but WITHOUT | ||
| * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or | ||
| * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License | ||
| * version 2 for more details (a copy is included in the LICENSE file that | ||
| * accompanied this code). | ||
| * | ||
| * You should have received a copy of the GNU General Public License version | ||
| * 2 along with this work; if not, write to the Free Software Foundation, | ||
| * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. | ||
| * | ||
| * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA | ||
| * or visit www.oracle.com if you need additional information or have any | ||
| * questions. | ||
| */ | ||
|
|
||
| module csp { | ||
| provides java.nio.charset.spi.CharsetProvider with provider.UppercasingCharsetProvider; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,87 @@ | ||
| /* | ||
| * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved. | ||
| * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. | ||
| * | ||
| * This code is free software; you can redistribute it and/or modify it | ||
| * under the terms of the GNU General Public License version 2 only, as | ||
| * published by the Free Software Foundation. | ||
| * | ||
| * This code is distributed in the hope that it will be useful, but WITHOUT | ||
| * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or | ||
| * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License | ||
| * version 2 for more details (a copy is included in the LICENSE file that | ||
| * accompanied this code). | ||
| * | ||
| * You should have received a copy of the GNU General Public License version | ||
| * 2 along with this work; if not, write to the Free Software Foundation, | ||
| * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. | ||
| * | ||
| * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA | ||
| * or visit www.oracle.com if you need additional information or have any | ||
| * questions. | ||
| */ | ||
| package provider; | ||
|
|
||
| import java.nio.ByteBuffer; | ||
| import java.nio.CharBuffer; | ||
| import java.nio.charset.Charset; | ||
| import java.nio.charset.CharsetDecoder; | ||
| import java.nio.charset.CharsetEncoder; | ||
| import java.nio.charset.CoderResult; | ||
| import java.nio.charset.spi.CharsetProvider; | ||
| import java.util.Collections; | ||
| import java.util.Iterator; | ||
|
|
||
| // A test charset provider that decodes every input byte into its uppercase | ||
| public class UppercasingCharsetProvider extends CharsetProvider { | ||
|
|
||
| @Override | ||
| public Iterator charsets() { | ||
| return Collections.singleton(new UppercasingCharsetProvider.UppercasingCharset()).iterator(); | ||
| } | ||
|
|
||
| @Override | ||
| public Charset charsetForName(String charsetName) { | ||
| if (charsetName.equals("Uppercasing")) { | ||
| return new UppercasingCharsetProvider.UppercasingCharset(); | ||
| } else { | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| public static class UppercasingCharset extends Charset { | ||
|
|
||
| public UppercasingCharset() { | ||
| super("Uppercasing", null); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean contains(Charset cs) { | ||
| return false; | ||
| } | ||
|
|
||
| @Override | ||
| public CharsetDecoder newDecoder() { | ||
| return new UppercasingCharsetDecoder(this, 1, 1); | ||
| } | ||
|
|
||
| @Override | ||
| public CharsetEncoder newEncoder() { | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| private static class UppercasingCharsetDecoder extends CharsetDecoder { | ||
| public UppercasingCharsetDecoder(Charset cs, float averageCharsPerByte, float maxCharsPerByte) { | ||
| super(cs, averageCharsPerByte, maxCharsPerByte); | ||
| } | ||
|
|
||
| @Override | ||
| protected CoderResult decodeLoop(ByteBuffer in, CharBuffer out) { | ||
| while (in.remaining() > 0) { | ||
| out.put(Character.toUpperCase((char)in.get())); | ||
| } | ||
| return CoderResult.UNDERFLOW; | ||
| } | ||
| } | ||
| } |
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.
Console.charset()states "The returned charset is used for interpreting the input and output source (e.g., keyboard and/or display) specified by the host environment or user, which defaults to the one based on stdout.encoding." If stdin.encoding is set otherwise, this is no longer true, so I think this method may need a wording update as well.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. Brought the same wording to the
charset()method description for further clarification.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'm confused by the actual behavior here. What might be helpful is to divide the discussion between a) what charsets get used for input and output, and b) the return value of the
charset()method.I'm not entirely sure, but since
stdin.encodingandstdout.encodingare always set to something -- whether it comes from the platform or the command line -- won't Console just usestdin.encodingfor input andstdout.encodingfor output? If this is true, maybe just say this instead of deferring to thecharset()method.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 I agree with Stuart and it would be better to say that stdin.encoding is used for reading, and stdout.encoding for writing. They are usually the same but if they differ then Console will return the charset for output.
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.
Thanks. I reworded the descriptions of the class and
charset()method.