Close process streams after exit to work around JDK-4311711 hang

src/main/java/org/apache/maven/shared/utils/cli/AbstractStreamHandler.java

@@ -18,6 +18,8 @@
  */
 package org.apache.maven.shared.utils.cli;
 
+import java.util.concurrent.TimeUnit;
+
 /**
  * @author <a href="mailto:kristian.rosenvold@gmail.com">Kristian Rosenvold</a>
  */
@@ -36,6 +38,33 @@ public synchronized void waitUntilDone() throws InterruptedException {
         }
     }
 
+    /**
+     * Waits until this handler is done or the given timeout elapses.
+     *
+     * @param timeoutInMillis timeout in milliseconds; a value less than or equal to zero waits indefinitely
+     * @return {@code true} if the handler finished within the timeout, {@code false} otherwise
+     * @throws InterruptedException if the current thread is interrupted while waiting
+     */
+    synchronized boolean waitUntilDone(long timeoutInMillis) throws InterruptedException {
+        if (timeoutInMillis <= 0) {
+            waitUntilDone();
+            return true;
+        }
+
+        long deadline = System.nanoTime() + TimeUnit.MILLISECONDS.toNanos(timeoutInMillis);
+
+        while (!isDone()) {
+            long remainingNanos = deadline - System.nanoTime();
+            if (remainingNanos <= 0) {
+                return false;
+            }
+
+            wait(TimeUnit.NANOSECONDS.toMillis(remainingNanos), (int) (remainingNanos % 1000000L));
+        }
+
+        return true;
+    }
+
     boolean isDisabled() {
         return disabled;
     }

src/main/java/org/apache/maven/shared/utils/cli/CommandLineUtils.java

@@ -18,6 +18,7 @@
  */
 package org.apache.maven.shared.utils.cli;
 
+import java.io.IOException;
 import java.io.InputStream;
 import java.nio.charset.Charset;
 import java.util.ArrayList;
@@ -28,7 +29,7 @@
 import java.util.StringTokenizer;
 import java.util.concurrent.TimeUnit;
 
 import org.apache.maven.shared.utils.Os;
 import org.apache.maven.shared.utils.StringUtils;
 import org.jspecify.annotations.NonNull;
 import org.jspecify.annotations.Nullable;
@@ -38,6 +39,13 @@
  */
 public abstract class CommandLineUtils {
 
+    /**
+     * Grace period in milliseconds to wait for the stream pumpers to drain the remaining process output and reach EOF
+     * on their own after the process has exited, before forcing EOF by closing the process streams. See
+     * <a href="https://bugs.java.com/bugdatabase/view_bug.do?bug_id=4311711">JDK-4311711</a>.
+     */
+    private static final long STREAM_EOF_GRACE_PERIOD_MS = 5000;
+
     /**
      * A {@code StreamConsumer} providing consumed lines as a {@code String}.
      *
@@ -274,10 +282,29 @@
 
                     int returnValue = p.waitFor();
 
+                    // After the process has terminated its output streams may, on some
+                    // JVMs, fail to deliver EOF (JDK-4311711), leaving the pumpers
+                    // blocked in readLine() forever. Normally the pumpers drain the
+                    // remaining buffered output and reach EOF on their own, so first
+                    // wait for them with a grace period; only if they are still stuck
+                    // force EOF by closing the streams. The pumpers are disabled first
+                    // so the IOException caused by our close is treated as EOF instead
+                    // of being reported as a stream failure.
                     try {
                         if (inputFeeder != null) {
                             inputFeeder.waitUntilDone();
                         }
+
+                        if (!outputPumper.waitUntilDone(STREAM_EOF_GRACE_PERIOD_MS)
+                                || !errorPumper.waitUntilDone(STREAM_EOF_GRACE_PERIOD_MS)) {
+                            outputPumper.disable();
+                            errorPumper.disable();
+
+                            closeProcessStreams(p);
+
+                            outputPumper.waitUntilDone();
+                            errorPumper.waitUntilDone();
+                        }
                     } finally {
                         try {
                             outputPumper.waitUntilDone();
@@ -324,6 +351,24 @@
         };
     }
 
+    private static void closeProcessStreams(Process p) {
+        try {
+            p.getOutputStream().close();
+        } catch (IOException e) {
+            // ignore
+        }
+        try {
+            p.getInputStream().close();
+        } catch (IOException e) {
+            // ignore
+        }
+        try {
+            p.getErrorStream().close();
+        } catch (IOException e) {
+            // ignore
+        }
+    }
+
     /**
      * Gets the shell environment variables for this process. Note that the returned mapping from variable names to
      * values will always be case-sensitive regardless of the platform, i.e. <code>getSystemEnvVars().get("path")</code>

src/main/java/org/apache/maven/shared/utils/cli/StreamPumper.java

@@ -82,12 +82,14 @@ public void run() {
                 }
             }
         } catch (IOException e) {
-            exception = e;
+            if (!isDisabled()) {
+                exception = e;
+            }
         } finally {
             try {
                 in.close();
             } catch (final IOException e2) {
-                if (this.exception == null) {
+                if (!isDisabled() && this.exception == null) {
                     this.exception = e2;
                 }
             }

src/test/java/org/apache/maven/shared/utils/cli/AbstractStreamHandlerTest.java

@@ -52,4 +52,39 @@ void setDoneNotifiesWaitUntilDone() throws InterruptedException {
         waiter.join(500);
         assertFalse(waiter.isAlive());
     }
+
+    @Test
+    void waitUntilDoneWithTimeoutReturnsFalseWhenNotDone() throws InterruptedException {
+        AbstractStreamHandler handler = new AbstractStreamHandler() {};
+
+        assertFalse(handler.waitUntilDone(100), "must time out while the handler is not done");
+    }
+
+    @Test
+    void waitUntilDoneWithTimeoutReturnsTrueWhenDone() throws InterruptedException {
+        AbstractStreamHandler handler = new AbstractStreamHandler() {};
+        handler.setDone();
+
+        assertTrue(handler.waitUntilDone(1000), "must return once the handler is done");
+    }
+
+    @Test
+    void waitUntilDoneWithTimeoutIsInterruptible() throws InterruptedException {
+        AbstractStreamHandler handler = new AbstractStreamHandler() {};
+
+        Thread waiter = new Thread(() -> {
+            try {
+                handler.waitUntilDone(5000);
+            } catch (InterruptedException e) {
+                Thread.currentThread().interrupt();
+            }
+        });
+        waiter.start();
+
+        Thread.sleep(50);
+        waiter.interrupt();
+
+        waiter.join(500);
+        assertFalse(waiter.isAlive(), "waitUntilDone must be interruptible");
+    }
 }

src/test/java/org/apache/maven/shared/utils/cli/CommandLineUtilsTest.java

@@ -107,6 +107,42 @@ public void givenASingleQuoteMarkInArgumentWhenExecutingCodeThenExitCode0Returne
         assertEquals(0, p.exitValue());
     }
 
+    /**
+     * A process that writes more output than fits in the OS pipe buffer is still being drained by the stream pumpers
+     * when {@code waitFor()} returns. Closing the process streams unconditionally at that point (as was attempted to
+     * work around <a href="https://bugs.java.com/bugdatabase/view_bug.do?bug_id=4311711">JDK-4311711</a>) makes an
+     * in-flight {@code readLine()} throw and surfaces as a spurious {@link CommandLineException} on a process that
+     * exited successfully with complete output. The close must therefore only be forced as a fallback after the pumpers
+     * had a grace period to reach EOF on their own. Repeats because the race is timing dependent. Unix-only because it
+     * relies on {@code seq}.
+     */
+    @Test
+    public void executeCommandLineWithLargeStdoutCompletesWithoutFailure() throws Exception {
+        if (!Os.isFamily(Os.FAMILY_UNIX)) {
+            return;
+        }
+
+        int lines = 50000;
+
+        StringBuilder expected = new StringBuilder();
+        for (int i = 1; i <= lines; i++) {
+            expected.append(i).append(System.lineSeparator());
+        }
+
+        for (int i = 0; i < 10; i++) {
+            Commandline cl = new Commandline("seq 1 " + lines);
+
+            CommandLineUtils.StringStreamConsumer stdout = new CommandLineUtils.StringStreamConsumer();
+            CommandLineUtils.StringStreamConsumer stderr = new CommandLineUtils.StringStreamConsumer();
+
+            int exitCode = CommandLineUtils.executeCommandLine(cl, stdout, stderr);
+
+            assertEquals(0, exitCode, "unexpected exit code in iteration " + i);
+            assertEquals(expected.toString(), stdout.getOutput(), "stdout must be complete in iteration " + i);
+            assertEquals("", stderr.getOutput(), "stderr must be empty in iteration " + i);
+        }
+    }
+
     @Test
     public void givenASingleQuoteMarkInArgumentWhenTranslatingToCmdLineArgsThenTheQuotationMarkIsNotEscaped()
             throws Exception {