Adds property for jline to prefer using stdout vs stderr

[ddanielr]

When the shell is being used with an execute command and pipe ./accumulo shell -e '<command>' | grep "<>",
jline does not write the output on stdout and instead writes it to stderr.

This causes frustration when attempting to script actions using the accumulo shell.

Without Property Set:

$ ./accumulo shell -e 'scan -t accumulo.root -np' | grep localhost 
2024-12-31T05:15:59,532 [shell.Shell] INFO : Loading configuration from /workspace/fluo-uno/install/accumulo-2.1.3/conf/accumulo-client.properties
!0;~ file:hdfs://localhost:8020/accumulo/tables/!0/table_info/0_1.rf []	0,0
!0;~ loc:10000501ae30004 []	localhost:9997
!0;~ srv:dir []	table_info
!0;~ srv:flush []	4
!0;~ srv:lock []	tservers/localhost:9997/zlock#d923681f-93ab-498f-b32e-0abf799b8354#0000000000$10000501ae30004
!0;~ srv:time []	L0
!0;~ ~tab:~pr []	\x00
!0< loc:10000501ae30004 []	localhost:9997
!0< srv:dir []	default_tablet
!0< srv:flush []	4
!0< srv:lock []	tservers/localhost:9997/zlock#d923681f-93ab-498f-b32e-0abf799b8354#0000000000$10000501ae30004
!0< srv:time []	L0
!0< ~tab:~pr []	\x01~

With Property Set:

$ ./accumulo shell -e 'scan -t accumulo.root -np' | grep localhost 
!0;~ file:hdfs://localhost:8020/accumulo/tables/!0/table_info/0_1.rf []	0,0
!0;~ loc:10000501ae30004 []	localhost:9997
!0;~ srv:lock []	tservers/localhost:9997/zlock#d923681f-93ab-498f-b32e-0abf799b8354#0000000000$10000501ae30004
!0< loc:10000501ae30004 []	localhost:9997
!0< srv:lock []	tservers/localhost:9997/zlock#d923681f-93ab-498f-b32e-0abf799b8354#0000000000$10000501ae30004

What made this odd was that the shell writes to stdout if not using a pipe | as redirecting the output to a file works
/accumulo shell -e '<command>' > output.txt

Related Jline Code:

https://github.com/jline/jline3/blob/c75301facc8716b59c1d57d3e3c5943358022560/terminal/src/main/java/org/jline/terminal/TerminalBuilder.java#L541

https://github.com/jline/jline3/blob/c75301facc8716b59c1d57d3e3c5943358022560/terminal/src/main/java/org/jline/terminal/TerminalBuilder.java#L588

[ddanielr]

@ctubbsii While testing this I also found that the issue could be fixed by setting the systemOutput in the builder.

accumulo/shell/src/main/java/org/apache/accumulo/shell/Shell.java

Lines 301 to 304 in 4eff735

	public boolean config(String... args) throws IOException {
	if (this.terminal == null) {
	this.terminal = TerminalBuilder.builder().jansi(false).build();
	}

 this.terminal = TerminalBuilder.builder().jansi(false).systemOutput(SystemOutput.SysOut).build();

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

[dlmarion]

@ddanielr - curious if you saw #3446, which I think might be somewhat related to this. Regarding the property that you found, is that a new addition to a newer JLine release?

[ddanielr]

@ddanielr - curious if you saw #3446, which I think might be somewhat related to this. Regarding the property that you found, is that a new addition to a newer JLine release?

I did see #3446 which was originally fixed in jline 3.24.0.
However we are already on jline 3.25.1 in accumulo 2.1 so whatever was fixed is no longer working in the 3.25.1 release.

The property I found was added in jline 3.22.0 so it's not super recent (Jan 2023).
Commit: jline/jline3@d6e84da#diff-8f75c2bcaf44443b94e6a2a293331a90047529aabbc2d0e2bae5811ce22ad692R56

Version 3.22.0: https://github.com/jline/jline3/blob/jline-parent-3.22.0/terminal/src/main/java/org/jline/terminal/TerminalBuilder.java#L56

[dlmarion]

Good find.

[ctubbsii]

 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:

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:

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:

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 (but I think that reverts the intent of jline/jline3#787, so there may be a philosophical disagreement with upstream devs on what the correct behavior should be when one stream is a tty and the other is not). 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.

[ctubbsii]

Prefer the code change, so the fix isn't dependent on user config. The property is a helpful workaround to document for users in the meantime, though.