ctubbsii commented on PR #5221: URL: https://github.com/apache/accumulo/pull/5221#issuecomment-2569945216
> ``` > this.terminal = TerminalBuilder.builder().jansi(false).systemOutput(SystemOutput.SysOut).build(); > ``` This seems like a sensible solution (though I'm not sure I understand why we should disable jansi). However, looking at the code, it looks like you should call `.streams(in, out)` to set up the input/output stream based on what was passed to the Shell (we might pass a different stream to the shell for testing, but the shell's normal execute/main method should use System.in and System.out for these). Trying to trace the code, the systemOutput seems to be a fallback for the system tty when the streams are not set. There's a lot of complex logic in the jline code that reconciles all these options, so I'm not 100% sure what the intent was here. I'm pretty sure that before we upgraded jline, we did explicitly set System.in and System.out when constructing the terminal. Perhaps that got lost in the upgrade? > Jline's documentation states that the default is the system output stream so it's interested that explicitly setting it fixes the issue. https://github.com/jline/jline3/blob/c75301facc8716b59c1d57d3e3c5943358022560/terminal/src/main/java/org/jline/terminal/TerminalBuilder.java#L187-L193 Tracing through the code, it looks like the default isn't actually `SystemOutput.SysOut`, but `SystemOutput.SysOutOrSysErr`, which seems causes one to be selected based on whether the terminal provider supports the output as a tty. I think it's falling back to stderr because stdout is not a tty. I tested by giving it a pseudo tty. This works, and sends it to stdout so I can grep: ```bash socat - EXEC:'accumulo shell -e tables\\ -np',pty,setsid,ctty | grep meta ``` However, neither a pipe nor a redirection works without simulating a tty. So, neither of these work: ```bash accumulo shell -e tables | grep meta accumulo shell -e tables > tables.txt ``` I added `org.jline` to my log4j config and ran the shell with the options to redirect the java-util-logging to log4j: ```bash ACCUMULO_JAVA_OPTS=-Djava.util.logging.manager=org.apache.logging.log4j.jul.LogManager accumulo shell -e tables ``` This gave some useful output about which terminal provider it was loading and which pty implementation it was using (skipping the stack traces for the providers which couldn't be loaded). It seems to be loading both the `jni` and the `exec` terminal providers, but `jni` is first, so I think it's using that: ``` 2025-01-03T18:16:02,963 [org.jline] DEBUG: Available providers: jni, exec 2025-01-03T18:16:02,996 [org.jline] DEBUG: Registering shutdown-hook: Thread[JLine Shutdown Hook,5,main] 2025-01-03T18:16:02,996 [org.jline] DEBUG: Adding shutdown-hook task: org.jline.terminal.impl.PosixSysTerminal$$Lambda$178/0x00007f48531b6940@467cb96f 2025-01-03T18:16:02,997 [org.jline] DEBUG: Using terminal PosixSysTerminal 2025-01-03T18:16:02,997 [org.jline] DEBUG: Using pty LinuxNativePty ``` I believe the problem is that the `JniTerminalProvider.isSystemStream(stream)` method returns false if any stream is not a tty, which it is not if you're redirecting/piping. This means that when it's selecting the output stream for the system type, and you are only redirecting stdout, then `SystemOutput.SysOutOrSysErr`, results in the stdout being skipped over and the stderr being used. You can verify this by redirecting both streams to separate files. When you do that, it is unable to construct a terminal using any output stream, and instead falls back to the DumbTerminal. I think ultimately, the problem is the logic at https://github.com/jline/jline3/blob/c75301facc8716b59c1d57d3e3c5943358022560/terminal/src/main/java/org/jline/terminal/TerminalBuilder.java#L407-L410 This logic sets the system stream based on whether the provider says any of the passed streams are considered system streams. But for the JniTerminalProvider, it's only considered a "system stream" if the stream is a tty, which is false for the stdout, but true for stderr, so JniTerminalProvider ends up being the selected provider using the stderr, which is still a tty. What it should do instead, it should only use that provider if *all* of the provided streams are considered system streams. Otherwise, it should use the DumbTerminalProvider with the system stream. I'm not sure if this worked at all after #3446. I think the original behavior that was implemented in jline/jline3#787 to use the terminal with stderr only still exists after jline/jline3#854, which I don't think fixed the issue. Or, if it did, a subsequent change broke it again. jline/jline3#854 only changed which output stream was used for the DumbTerminal, but the bug we're now seeing is that the DumbTerminal isn't being selected... the JniTerminal is being selected but with only stderr since stdout is not a tty. I think this issue needs to be raised again upstream. In the meantime, I think setting the stream directly to stdout will be the right fix for Accumulo. The property can be mentioned to users who need a workaround in the meantime, but I don't think our code should rely on that property being set in the config for shell redirection to work correctly. What I'd be curious to know, though, is whether pagination is attempted after setting the system output stream correctly. I noticed while testing, when I gave a pseudo tty, if I didn't specify `-np` to disable pagination, it would paginate after every entry. But, it shouldn't do that... it should behave correctly without pagination automatically when there is no tty, but still use the correct output stream for piping/redirection. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
