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

Reply via email to