diff --git a/src/main/java/org/apache/maven/shared/utils/cli/AbstractStreamHandler.java b/src/main/java/org/apache/maven/shared/utils/cli/AbstractStreamHandler.java index 7bfea7a2..ae12d7f4 100644 --- a/src/main/java/org/apache/maven/shared/utils/cli/AbstractStreamHandler.java +++ b/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 Kristian Rosenvold */ @@ -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; } diff --git a/src/main/java/org/apache/maven/shared/utils/cli/CommandLineUtils.java b/src/main/java/org/apache/maven/shared/utils/cli/CommandLineUtils.java index ee676901..1a3a72a7 100644 --- a/src/main/java/org/apache/maven/shared/utils/cli/CommandLineUtils.java +++ b/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; @@ -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 + * JDK-4311711. + */ + private static final long STREAM_EOF_GRACE_PERIOD_MS = 5000; + /** * A {@code StreamConsumer} providing consumed lines as a {@code String}. * @@ -274,10 +282,29 @@ public Integer call() throws CommandLineException { 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 @@ public Integer call() throws CommandLineException { }; } + 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. getSystemEnvVars().get("path") diff --git a/src/main/java/org/apache/maven/shared/utils/cli/StreamPumper.java b/src/main/java/org/apache/maven/shared/utils/cli/StreamPumper.java index f9a51ae0..9d023591 100644 --- a/src/main/java/org/apache/maven/shared/utils/cli/StreamPumper.java +++ b/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; } } diff --git a/src/test/java/org/apache/maven/shared/utils/cli/AbstractStreamHandlerTest.java b/src/test/java/org/apache/maven/shared/utils/cli/AbstractStreamHandlerTest.java index eebe0a24..a8a7bdc9 100644 --- a/src/test/java/org/apache/maven/shared/utils/cli/AbstractStreamHandlerTest.java +++ b/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"); + } } diff --git a/src/test/java/org/apache/maven/shared/utils/cli/CommandLineUtilsTest.java b/src/test/java/org/apache/maven/shared/utils/cli/CommandLineUtilsTest.java index c4beaf75..aaef68a3 100644 --- a/src/test/java/org/apache/maven/shared/utils/cli/CommandLineUtilsTest.java +++ b/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 JDK-4311711) 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 {