RFR: JDK-8306983: Do not invoke external programs when switch terminal to raw mode on selected platforms [v2]

Tue May 9 20:59:35 UTC 2023

On Fri, 5 May 2023 12:34:29 GMT, Jan Lahoda <jlahoda at openjdk.org> wrote:

>> To support JShell and other usecases, the JDK uses JLine, which provides line-editing functionality inside a terminal.
>> 
>> JLine has several ways to work with the terminal, based on various additional libraries and tools. Most of them are not directly usable inside the JDK, so currently, on non-Windows platforms, the JLine inside the JDK will use external programs, like `stty`, to inspect the terminal's properties and to switch the terminal to raw mode.
>> 
>> This is slightly problematic, as the external programs might be missing, and while this is not that big problem for JShell, it might be a problem for other potential uses of JLine, like using it inside `System.console()`.
>> 
>> So, the proposal here is to call the corresponding native methods directly, on selected platforms for now (Linux and Mac OS/X), instead of invoking the external programs. On Windows, this has always been the case, we are using a custom implementation of the interface that maps native and Java function for JNA there, and the proposal is to do the same here. We take the appropriate mapping interface for JNA, and provide hand-written implementation for it, using JNI.
>> 
>> The Windows implementation is mostly unchanged, the changes are mostly non-Windows only. The overview of the changes is:
>>  - `LastErrorException` is moved from the Windows-specific code to the platform-neutral code, as it is used by all the native implementations. This is the only change that directly affects the Windows-specific code
>>  -  `unix/classes/jdk/internal/org/jline/terminal/impl/jna/JnaNativePty.java` and `unix/classes/jdk/internal/org/jline/terminal/impl/jna/JnaTerminalProvider.java` are created based on the corresponding files from JLine. In JLine, these files directly call platform-specific implementations, but in this patch, the platform specific code is commented out, and replaced with calls to `JDKNativePty`, which is a new platform-specific class, that delegates to the actual platform-specific implementations. Note the `JnaNativePty.java` and `JnaTerminalProvider.java` already exist in the Windows part, but have different pieces of code commented out, which makes them substantially different.
>>  - for Linux and Mac OS/X, there are platform-specific implementations based on the corresponding code from JLine, with the hand-written JNI-based implementation of the JNA-based existing interfaces. They also have an implementation of `JDKNativePty`, which just delegates to the given platform's `"NativePty"` implementation.
>>  - the `JdkConsoleProviderImpl.java` has been tweaked to not allow the implementation based on the external programs, and gracefully falling back to the standard implementation. JShell is kept unchanged.
>> 
>> The reasons for this organization are: limiting duplication, mostly adhering to the JDK's platform specific build (which is different from the normal JLine's platform-neutral build) and limiting the difference between standard JLine and JLine inside JDK, to help future upgrades.
>
> Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Adjusting build script as suggested on the review.

src/jdk.internal.le/linux/native/lible/CLibrary.cpp line 2:

> 1: /*
> 2:  * Copyright (c) 2015, 2018, Oracle and/or its affiliates. All rights reserved.

Copyright year 2023 only if this is new.

src/jdk.internal.le/linux/native/lible/CLibrary.cpp line 103:

> 101: 
> 102:     if (tcgetattr(fd, &data) != 0) {
> 103:         jobject exc = env->NewObject(lastErrorExceptionClass,

Could lines 103-106 be converted to a utility function which would then be invoked at current lines 103, 141, 163, and 197?

src/jdk.internal.le/linux/native/lible/CLibrary.cpp line 116:

> 114:     env->SetIntField(result, c_line, data.c_line);
> 115:     jbyteArray c_ccValue = (jbyteArray) env->GetObjectField(result, c_cc);
> 116:     env->SetByteArrayRegion(c_ccValue, 0, NCCS, (signed char *) data.c_cc);//TODO: cast?

Cast instead to `(jbyte*)` here and at lines 136 and 204?

src/jdk.internal.le/linux/native/lible/CLibrary.cpp line 183:

> 181: JNIEXPORT jint JNICALL Java_jdk_internal_org_jline_terminal_impl_jna_linux_CLibraryImpl_isatty
> 182:   (JNIEnv *, jobject, jint fd) {
> 183:     return isatty(fd);

Do we care if the native `isatty()` returns zero? Or is this dealt with somewhere else?

src/jdk.internal.le/macosx/native/lible/CLibrary.cpp line 2:

> 1: /*
> 2:  * Copyright (c) 2015, 2018, Oracle and/or its affiliates. All rights reserved.

Copyright year 2023 only?

src/jdk.internal.le/macosx/native/lible/CLibrary.cpp line 59:

> 57: 
> 58: JNIEXPORT void JNICALL Java_jdk_internal_org_jline_terminal_impl_jna_osx_CLibraryImpl_initIDs
> 59:   (JNIEnv *env, jclass) {

Missing parameter for `jclass`? Maybe add `clazz` or `unused` or something?

src/jdk.internal.le/macosx/native/lible/CLibrary.cpp line 109:

> 107: 
> 108:     if (tcgetattr(fd, &data) != 0) {
> 109:         jobject exc = env->NewObject(lastErrorExceptionClass,

Could lines 109-112 be converted to a utility function which would then be invoked at current lines 109, 145, 167, and 197?

src/jdk.internal.le/macosx/native/lible/CLibrary.cpp line 121:

> 119:     env->SetLongField(env->GetObjectField(result, c_lflag), nativelong_value, data.c_lflag);
> 120:     jbyteArray c_ccValue = (jbyteArray) env->GetObjectField(result, c_cc);
> 121:     env->SetByteArrayRegion(c_ccValue, 0, NCCS, (signed char *) data.c_cc);

Cast instead to `(jbyte*)` here and at lines 140 and 208?

src/jdk.internal.le/macosx/native/lible/CLibrary.cpp line 187:

> 185: JNIEXPORT jint JNICALL Java_jdk_internal_org_jline_terminal_impl_jna_osx_CLibraryImpl_isatty
> 186:   (JNIEnv *, jobject, jint fd) {
> 187:     return isatty(fd);

Do we care if the native `isatty()` returns zero? Or is this dealt with somewhere else?

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/13687#discussion_r1189114958
PR Review Comment: https://git.openjdk.org/jdk/pull/13687#discussion_r1189116097
PR Review Comment: https://git.openjdk.org/jdk/pull/13687#discussion_r1189116683
PR Review Comment: https://git.openjdk.org/jdk/pull/13687#discussion_r1189117836
PR Review Comment: https://git.openjdk.org/jdk/pull/13687#discussion_r1189123411
PR Review Comment: https://git.openjdk.org/jdk/pull/13687#discussion_r1189118782
PR Review Comment: https://git.openjdk.org/jdk/pull/13687#discussion_r1189120027
PR Review Comment: https://git.openjdk.org/jdk/pull/13687#discussion_r1189121218
PR Review Comment: https://git.openjdk.org/jdk/pull/13687#discussion_r1189121521