8352693: Use a simpler console reader instead of JLine for System.console() #24242

lahodaj · 2025-03-26T07:54:48Z

The java.io.Console has several backends: a simple on in java.base, a more convenient one in jdk.internal.le (with line-reading based on JLine) and one for JShell.

The backend based on JLine is proving to be a somewhat problematic - JLine is very powerful, possibly too powerful and complex for the simple task of editing a line with no completion, no history, no variables, no commands, etc. As a consequence, there are inevitable sharp edges in this backend.

The idea in this PR is to replace the use of JLine in the jdk.internal.le backend with a simple escape code interpreter, that only handles a handful of keys/codes (left/right arrow, home, end, delete, backspace, enter), and ignores the rest. The goal is to have something simple with less surprising behavior.

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

Warnings

⚠️ Patch contains a binary file (src/java.desktop/share/classes/java/awt/doc-files/BorderLayout-1.gif)
⚠️ Patch contains a binary file (src/java.desktop/share/classes/java/awt/doc-files/FlowLayout-1.gif)
⚠️ Patch contains a binary file (src/java.desktop/share/classes/java/awt/doc-files/GridBagLayout-1.gif)
⚠️ Patch contains a binary file (src/java.desktop/share/classes/java/awt/doc-files/GridBagLayout-2.gif)
⚠️ Patch contains a binary file (src/java.desktop/share/classes/java/awt/doc-files/GridLayout-1.gif)
⚠️ Patch contains a binary file (src/java.desktop/share/classes/java/awt/doc-files/GridLayout-2.gif)
⚠️ Patch contains a binary file (test/hotspot/jtreg/runtime/ClassFile/JsrRewritingTestCase.jar)
⚠️ Patch contains a binary file (test/hotspot/jtreg/runtime/ClassFile/testcase.jar)

Issue

JDK-8352693: Use a simpler console reader instead of JLine for System.console() (Task - P4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24242

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

Using diff file

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

Using Webrev

Link to Webrev Comment

…sole()

bridgekeeper · 2025-03-26T07:55:43Z

👋 Welcome back jlahoda! 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 · 2025-03-26T07:56:28Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2025-03-26T07:57:05Z

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

build
core-libs
kulla

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 · 2025-03-26T08:28:12Z

Webrevs

AlanBateman · 2025-03-26T08:55:09Z

Can you coordinate with Naoto on this? There is a CSR in progress to switch Console back to using the java.base implementation by default.

AlanBateman · 2025-03-26T08:58:03Z

src/jdk.internal.le/share/classes/jdk/internal/console/SimpleConsoleReader.java

+        }
+    }
+
+    private static void doReadImpl(Reader reader, Writer out, boolean password, int firstLineOffset, IntSupplier terminalWidthSupplier, CleanableBuffer result) throws IOException {


These overly long lines make it really hard to see the changes going forward so I think trim back some of these to make it easy to look at diffs in the future.

AlanBateman · 2025-03-26T08:59:39Z

src/java.base/share/classes/module-info.java

        java.management,
        java.rmi,
        jdk.charsets,
+        jdk.internal.le,


What code in JLine is using shared secrets? I wonder what we need to change to avoid this.

Oops, sorry. I forgot to remove these (qualified export of jdk.internal.access and sun.nio.cs) after a refactoring. Will remove them in a next update.

AlanBateman · 2025-03-26T09:00:30Z

src/java.base/share/classes/module-info.java

    exports sun.nio.cs to
-        jdk.charsets;
+        jdk.charsets,
+        jdk.internal.le;


Same thing here.

lahodaj · 2025-03-26T10:26:00Z

Can you coordinate with Naoto on this? There is a CSR in progress to switch Console back to using the java.base implementation by default.

Right - but that still keeps the existing JLine-based provider, and so the problems it has still exist (albeit with a lower severity). This proposal is not the only sole possibility, of course. Other possibilities include removing the JLine-based provider, or fixing the problems in the JLine provider one-by-one. I don't object to any of these possibilities, although I didn't want to offer them in the first step.

naotoj

I am not very familiar with those escape sequences/modes in the native console, but looks good to me. Sharing the common part as the base looks reasonable.

naotoj · 2025-03-27T18:28:17Z

src/jdk.internal.le/share/classes/jdk/internal/console/SimpleConsoleReader.java

+                case 127:
+                    //backspace:


Is it delete?

I don't think so. Delete is an escape sequence (\033[3~).

Hmm, that's interesting. In ASCII control codes, 127 is defined as Delete and 08 is defined as Backspace

naotoj · 2025-03-27T19:29:29Z

src/jdk.internal.le/share/classes/jdk/internal/console/SimpleConsoleReader.java

+            }
+
+            if (it.hasNext()) {
+                out.append("\n\r");


I understand this is a simple console, but do we want to CR/LF/CRLF based on the platform?

line.seprator system property can be used here

When the terminal is in the raw/unprocessed mode, I believe \n is only moving the cursor to the next line (without changing the column), and \r is doing carriage return. So, to move the cursor to the first character of the next line, both are needed.

magicus · 2025-03-31T12:37:47Z

src/jdk.internal.le/windows/native/lible/WindowsTerminal.cpp

+
+#include <stdlib.h>
+#include <Windows.h>
+//#include <wincon.h>


Should you really check in new files with commented-out code?

magicus · 2025-03-31T12:43:26Z

make/modules/jdk.internal.le/Lib.gmk

+      NAME := le, \
+      TOOLCHAIN := TOOLCHAIN_LINK_CXX, \
+      OPTIMIZATION := LOW, \
+      JDK_LIBS := java.base:libjava, \


A quick glance through the native code revealed no obvious calls to libjava functions. Are you sure this dependency is needed?

CHECK_NULL is used by the native code, originating in jni_util.h. But, looking at other libraries, seems that

EXTRA_HEADER_DIRS := java.base:libjava,

should be enough (i.e. replacing JDK_LIBS with EXTRA_HEADER_DIRS). Is there some even better way?

magicus · 2025-03-31T12:43:45Z

make/modules/jdk.internal.le/Lib.gmk

+      LIBS_unix := $(JDKLIB_LIBS) $(LIBCXX), \
+      LIBS_windows := $(JDKLIB_LIBS) user32.lib, \


JDKLIB_LIBS is not used anymore.

magicus · 2025-03-31T12:44:04Z

make/modules/jdk.internal.le/Lib.gmk

+      TOOLCHAIN := TOOLCHAIN_LINK_CXX, \
+      OPTIMIZATION := LOW, \
+      JDK_LIBS := java.base:libjava, \
+      LIBS_unix := $(JDKLIB_LIBS) $(LIBCXX), \


LIBCXX is added automatically nowadays, so you can remove this entire line.

magicus · 2025-03-31T12:45:48Z

make/modules/jdk.internal.le/Lib.gmk

+  $(eval $(call SetupJdkLibrary, BUILD_LIBLE, \
+      NAME := le, \
+      TOOLCHAIN := TOOLCHAIN_LINK_CXX, \
+      OPTIMIZATION := LOW, \


We are trying to move away from using LOW as a default optimization level. If you believe this library is not performance critical, please use SIZE as the new default. Or, use HIGHEST if that gives performance benefits with no significant impact on library size.

magicus · 2025-03-31T12:48:08Z

make/modules/jdk.internal.le/Lib.gmk

+
+################################################################################
+
+ifeq ($(call isTargetOs, linux macosx windows), true)


You might want to sync with the AIX guys to see if this should be enabled there as well. The unix version seems quite portable, so I assume it will work with very few adjustments, if any. @MBaesken

Otherwise, what will happen if you try to run this on a platform without the corresponding native library?

…vider.

dmlloyd · 2025-03-31T17:22:45Z

src/jdk.internal.le/share/classes/jdk/internal/console/SimpleConsoleReader.java

+                        caret--;
+                    }
+                    continue READ;
+                case '\033':


If this is meant to be platform-agnostic, is it really safe to make these assumptions about the ability to produce or interpret escape codes and to make assumptions about their behavior on the user's terminal? I don't think it would even be safe to assume a single terminal type or interpretation on POSIX-type OSes; that's what things like terminfo/termcap are supposed to be for, right?

In theory, yes, that's what terminfo is supposed to do. But terminfo has its own set of problems, and what we get from a terminal and what we send to the terminal is so simple and generally supported, that it should be possible to make it work on all realistic terminals(*). Or is there a particular terminal you have in mind?

(*) I admit I've forgot that \E[7~/\E[8~ is Home/End as well. I'll fix that.

I suppose I don't have a particular terminal in mind. But at the very least, maybe some documentation somewhere should reference e.g. ECMA-48 as the basis for the behaviors of the console?

openjdk · 2025-04-03T17:56:10Z

@lahodaj this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8352693
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

lahodaj · 2025-04-03T18:09:08Z

I have updated the PR based on most of the review comments so far:

removed the unnecessary qualified exports (91fa1b2)
reduced line lengths (91fa1b2)
cleaned comments in the C code (91fa1b2)
cleaned up the makefiles (91fa1b2)
if the native library (for UNIX) is not available, fall back to the simple/default provider (although support for AIX would be great, of course) (c6256ee)
I realized the backspace value can (and should) be read from the terminal as a control character, so I did that (d5176d6)

magicus · 2025-04-04T14:25:22Z

make/modules/jdk.internal.le/Lib.gmk

+################################################################################
+
+ifeq ($(call isTargetOs, linux macosx windows), true)
+


Can you please insert the standardized header we use for native libraries? (These are extremely helpful to find/locate where certain libraries are built, since there is no existing proper IDE support for makefiles.)

Suggested change

##############################################################################

## Build lible

##############################################################################

liach · 2025-04-23T03:09:04Z

src/java.base/share/classes/jdk/internal/io/BaseJdkConsoleImpl.java

+
+    protected final Charset charset;
+    protected final Object readLock;
+    protected final Object writeLock;


Recommend mentioning that when both locks must be acquired, the writeLock must be acquired first. Unfortunately we cannot just use a ReentrantReadWriteLock.

liach · 2025-04-23T03:09:17Z

src/java.base/share/classes/jdk/internal/io/BaseJdkConsoleImpl.java

+    protected abstract char[] readline(boolean password) throws IOException;
+
+    @SuppressWarnings("this-escape")
+    public BaseJdkConsoleImpl(Charset charset) {


Suggested change

public BaseJdkConsoleImpl(Charset charset) {

protected BaseJdkConsoleImpl(Charset charset) {

liach · 2025-04-23T03:12:03Z

src/java.base/share/classes/jdk/internal/io/BaseJdkConsoleImpl.java

+    protected final PrintWriter pw;
+    protected final Formatter formatter;
+
+    protected abstract char[] readline(boolean password) throws IOException;


I recommend a more distinct method name, like nextLine or implReadLine; using readline to distinguish from readLine is weird.

liach · 2025-04-23T03:16:04Z

src/jdk.internal.le/share/classes/jdk/internal/console/JdkConsoleImpl.java

+        return charset;
+    }
+
+    protected char[] readline(boolean password) throws IOException {


@Override

liach · 2025-04-23T03:16:44Z

src/jdk.internal.le/share/classes/jdk/internal/console/LastErrorException.java

+package jdk.internal.console;
+
+@SuppressWarnings("serial")
+public class LastErrorException extends RuntimeException{


Suggested change

public class LastErrorException extends RuntimeException{

public class LastErrorException extends RuntimeException {

liach · 2025-04-23T03:19:50Z

src/jdk.internal.le/share/classes/jdk/internal/console/JdkConsoleImpl.java

+/**
+ * JdkConsole implementation based on the platform's TTY, with basic keyboard navigation.
+ */
+public final class JdkConsoleImpl extends BaseJdkConsoleImpl {


We should choose a different simple name for this class, especially when we are importing classes from jdk.internal.io package.

bridgekeeper · 2025-05-21T08:22:18Z

@lahodaj This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper · 2025-06-18T09:38:25Z

@lahodaj This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

8352693: Use a simpler console reader instead of JLine for System.con…

2fb2f00

…sole()

openjdk bot added build build-dev@openjdk.org core-libs core-libs-dev@openjdk.org kulla kulla-dev@openjdk.org labels Mar 26, 2025

Removing trailing whitespace

77c41e4

openjdk bot added the rfr Pull request is ready for review label Mar 26, 2025

AlanBateman reviewed Mar 26, 2025

View reviewed changes

naotoj reviewed Mar 27, 2025

View reviewed changes

magicus reviewed Mar 31, 2025

View reviewed changes

lahodaj added 2 commits March 31, 2025 16:34

Reflecting review feedback.

91fa1b2

If there's no native library available, fall back to the standard pro…

c6256ee

…vider.

dmlloyd reviewed Mar 31, 2025

View reviewed changes

Using control characters to get backspace control character.

d5176d6

openjdk bot added merge-conflict Pull request has merge conflict with target branch and removed rfr Pull request is ready for review labels Apr 3, 2025

Merge branch 'master' into JDK-8352693

daa2cc3

openjdk bot added rfr Pull request is ready for review and removed merge-conflict Pull request has merge conflict with target branch labels Apr 3, 2025

magicus reviewed Apr 4, 2025

View reviewed changes

Reflecting review feedback: Adding makefile comment as suggested

d75641e

liach reviewed Apr 23, 2025

View reviewed changes

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Jun 5, 2025

bridgekeeper bot closed this Jun 18, 2025

		LIBS_unix := $(JDKLIB_LIBS) $(LIBCXX), \
		LIBS_windows := $(JDKLIB_LIBS) user32.lib, \


		################################################################################

		ifeq ($(call isTargetOs, linux macosx windows), true)

	public BaseJdkConsoleImpl(Charset charset) {
	protected BaseJdkConsoleImpl(Charset charset) {

	public class LastErrorException extends RuntimeException{
	public class LastErrorException extends RuntimeException {

8352693: Use a simpler console reader instead of JLine for System.console() #24242

8352693: Use a simpler console reader instead of JLine for System.console() #24242

Uh oh!

Conversation

lahodaj commented Mar 26, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Warnings

Issue

Reviewing

Uh oh!

bridgekeeper bot commented Mar 26, 2025

Uh oh!

openjdk bot commented Mar 26, 2025

Uh oh!

openjdk bot commented Mar 26, 2025

Uh oh!

mlbridge bot commented Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

AlanBateman commented Mar 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lahodaj commented Mar 26, 2025

Uh oh!

naotoj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

naotoj Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Apr 3, 2025

Uh oh!

lahodaj commented Apr 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lahodaj commented Mar 26, 2025 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 26, 2025 •

edited

Loading

naotoj Mar 27, 2025 •

edited

Loading