Examine usage of setAccessible in AnsiConsoleLoader

[ChrisHegarty]

Elasticsearch Version

8.1

Problem Description

AnsiConsoleLoader is using setAccessible(true) to gain access to the internals of JANSI, in order to determine the charset that JANSI has determined is appropriate to use for its print streams. The use of setAccessible(true) is problematic, since it highlights a dependency on an internal implementation detail.

JANSI determines the charset to use by inspecting the sun.stdout.encoding/sun.stderr.encoding system properties, falling back to the Charset::defaultCharset where the relevant property is not set. Since Java 17 System.console().charset() returns the appropriate console charset, see JDK-8264209 . The System.console().charset() method determines the charset in a similar way to that of JANSI. In fact, it would be appropriate for JANSI to use same (if it compiles to a minimum of Java 17).

Note: System.console().charset() only inspects sun.stdout.encoding, not sun.stderr.encoding, but the usage in AnsiConsoleLoader is only interest in stdout (not stderr).

[elasticmachine]

Pinging @elastic/es-core-infra (Team:Core/Infra)

[ChrisHegarty]

One possible way to resolve this:

@@ -44,7 +39,7 @@ public class AnsiConsoleLoader implements Supplier<ConsoleLoader.Console> {
             // option on the console using the Kernel32 API, which JANSI knows to do, but ES currently lacks
             // the testing infra to assert the behavior
             boolean ansiEnabled = Ansi.isEnabled() && out.getType() != AnsiType.VirtualTerminal;
-            return new ConsoleLoader.Console(out, () -> out.getTerminalWidth(), ansiEnabled, tryExtractPrintCharset(out));
+            return new ConsoleLoader.Console(out, () -> out.getTerminalWidth(), ansiEnabled, System.console().charset());
         } else {
             return null;
         }

[ChrisHegarty]

@albertzaharovits @jkakavas, since you have more context and were involved in the PR #82740, which introduced the above code.

[albertzaharovits]

@ChrisHegarty Thanks for looking into this.

I see that the way JANSI detects the console encoding and the new System.console().charset() are similar (at least in v 2.4.0).
Similar, but not identical. Theretically, there would be a chance that we output Unicode-only glyphs (because System.console().charset() says that's OK), to the JANSI Printstream, but which are not encoded correctly because the JANSI Printstream uses something different.
Does this sound plausible to you?

I would say that ideally JANSI should use the new Java API internally, and expose the charset in a public method.

I'm not opposed to using the Java API, but I also don't see a big issue for using reflection here: we might not be able to detect the encoding after a lib upgrade, which would prevent from outputing Unicode-only glyphs. Maybe tests catch it (unlikely, likely we will see it when trying it manually).