On Tue, 6 Dec 2022 06:19:37 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> Naoto Sato has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fixed the copyright year > > src/java.base/share/classes/java/io/Console.java line 99: > >> 97: */ >> 98: >> 99: public class Console implements Flushable > > Should we perhaps `seal` this class and only `permit` `ProxyingConsole` to > `extend` it? Right. Will address it after this PR. > src/java.base/share/classes/java/io/Console.java line 615: > >> 613: var consModName = System.getProperty("jdk.console", >> 614: >> JdkConsoleProvider.DEFAULT_PROVIDER_MODULE_NAME); >> 615: return >> ServiceLoader.load(JdkConsoleProvider.class).stream() > > Are we intentionally using thread context classloader (which can be different > depending on which caller ends up first accessing/initializing the > `java.io.Console` class) to load these services? > > I initially thought that `java.io.Console` might be used/initialized early in > the bootstrap process of Java so the classloader could perhaps be > deterministic, but running a trivial Java application with `-verbose:class` > shows that `java.io.Console` doesn't get instantiated during the launch, so > that leaves this code to "first access wins" situation and maybe an > "incorrect" context classloader which doesn't have access the configured > `jdk.console` module may end up causing this code to default to > `java.io.Console`? > > > public class Hello { > public static void main(final String[] args) { > } > } > > > java -verbose:class Hello.java > > > Instead, should we perhaps use the ModuleLayer to find this configured module > and then use its classloader to load the `JdkConsoleProvider` service > provider? Something like: > > > final Optional<Module> mod = ModuleLayer.boot().findModule(consModName); > // ... if not present default to java.io.Console else use the module's > classloader to try and load the JdkConsoleProvider > return ServiceLoader.load(JdkConsoleProvider.class, > mod.get().getClassLoader()).stream()...... Changed it to use the boot layer `ServiceLoader.load(ModuleLayer.boot(), JdkConsoleProvider.class)` > src/jdk.internal.le/share/classes/jdk/internal/org/jline/JdkConsoleProviderImpl.java > line 113: > >> 111: public JdkConsoleImpl() { >> 112: try { >> 113: terminal = TerminalBuilder.builder().build(); > > The `java.io.Console` in its static initialization code has some logic to > determine the `Charset` to use. Should that same `Charset` (or logic) be used > here to build the terminal? Something like > `TerminalBuilder.builder().encoding(fooBarCharset).build();`. Initially, I thought of having charset as an argument to the constructor but realized jline would delve into the platform and figure out the same encoding, so I omitted it. However now I realized that the user could specify the standard out encoding via the `stdout/err.encoding` system property. So I revived the charset argument, as in your suggestion. ------------- PR: https://git.openjdk.org/jdk/pull/11421