(groovy) 18/20: GROOVY-12410: groovysh `/!` can lose a shell command's output

[pa...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

paulk-asert pushed a commit to branch GROOVY_6_0_X
in repository https://gitbox.apache.org/repos/asf/groovy.git

commit c20e8d4c1f98515c1fdd540417eff1276683b02a
Author: Paul King <pa...@asert.com.au>
AuthorDate: Mon Sep 14 19:08:30 2026 +1000

    GROOVY-12410: groovysh `/!` can lose a shell command's output
    
    executeCommand started the two stream-reader threads and then waited for
    the process, but nothing waited for the threads:
    
        proc.consumeProcessOutput(sout, serr)   // starts the readers
        int exitCode = proc.waitFor()           // waits for the process only
        if (sout.size()) print sout             // may run before they append
    
    consumeProcessOutput starts the readers and returns; waitFor waits for
    the process to exit, which can happen while output is still sitting in
    the pipe unread. The command could therefore print nothing for a shell
    command that produced output. waitForProcessOutput joins both readers
    and then waits for the process, which is the guarantee this code needs;
    its javadoc makes the distinction explicit. exitValue is safe after it,
    since the process has finished.
    
    This is the only occurrence of the shape in product code. The two in
    `WorkingWithIOSpecTest` are the documented drain-to-avoid-blocking
    idiom, where the output is discarded, so they are correct as they stand.
    
    The failure seen in CI is `ExtraConsoleCommandsTest.bangRunsAPortableEcho`
    asserting on an empty buffer, intermittently and on a different JDK each
    time. That is consistent with this race, but it did not reproduce
    locally, so the fix is made on the strength of the code being wrong
    rather than on a demonstrated repro.
---
 .../groovy/org/apache/groovy/groovysh/ExtraConsoleCommands.groovy  | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/subprojects/groovy-groovysh/src/main/groovy/org/apache/groovy/groovysh/ExtraConsoleCommands.groovy b/subprojects/groovy-groovysh/src/main/groovy/org/apache/groovy/groovysh/ExtraConsoleCommands.groovy
index f0acc3eead..56d1cc555a 100644
--- a/subprojects/groovy-groovysh/src/main/groovy/org/apache/groovy/groovysh/ExtraConsoleCommands.groovy
+++ b/subprojects/groovy-groovysh/src/main/groovy/org/apache/groovy/groovysh/ExtraConsoleCommands.groovy
@@ -226,8 +226,11 @@ class ExtraConsoleCommands extends JlineCommandRegistry implements CommandRegist
         def sout = new StringBuilder(), serr = new StringBuilder()
         def command = OSUtils.IS_WINDOWS ? ['cmd.exe', '/c'] : ['sh', '-c']
         def proc = new ProcessBuilder().command(command + args.join(' ')).start()
-        proc.consumeProcessOutput(sout, serr)
-        int exitCode = proc.waitFor()
+        // waitForProcessOutput joins the two reader threads before returning;
+        // consumeProcessOutput only starts them, and waitFor waits for the
+        // process rather than for them, so the output could still be unread
+        proc.waitForProcessOutput(sout, serr)
+        int exitCode = proc.exitValue()
         if (sout.size()) print sout
         if (exitCode != 0) {
             if (serr.size()) print serr