diff --git a/src/main/java/pl/project13/core/NativeGitProvider.java b/src/main/java/pl/project13/core/NativeGitProvider.java index 1e5702a..6f6616d 100644 --- a/src/main/java/pl/project13/core/NativeGitProvider.java +++ b/src/main/java/pl/project13/core/NativeGitProvider.java @@ -25,12 +25,11 @@ import javax.annotation.Nonnull; import java.io.*; -import java.nio.charset.StandardCharsets; import java.text.SimpleDateFormat; import java.util.Optional; import java.util.concurrent.*; import java.util.concurrent.atomic.AtomicBoolean; -import java.util.function.Function; +import java.util.function.Consumer; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -338,7 +337,7 @@ private String getOriginRemote(File directory, long nativeGitTimeoutInMs) throws } /** - * Runs a maven command and returns {@code true} if output was non empty. + * Runs a Git command and returns {@code true} if output was non empty. * Can be used to short cut reading output from command when we know it may be a rather long one. * Return true if the result is empty. **/ @@ -468,12 +467,10 @@ public String run(File directory, long nativeGitTimeoutInMs, String command) thr try { final StringBuilder commandResult = new StringBuilder(); - final Function stdoutConsumer = line -> { + final Consumer stdoutConsumer = line -> { if (line != null) { - commandResult.append(line).append("\n"); + commandResult.append(line).append('\n'); } - // return true to indicate we want to read more content - return true; }; runProcess(directory, nativeGitTimeoutInMs, command, stdoutConsumer); @@ -489,12 +486,9 @@ public boolean runEmpty(File directory, long nativeGitTimeoutInMs, String comman final AtomicBoolean empty = new AtomicBoolean(true); try { - final Function stdoutConsumer = line -> { - if (line != null) { - empty.set(false); - } - // return false to indicate we don't need to read more content - return false; + final Consumer stdoutConsumer = line -> { + empty.set(false); + // Ignore the content of the line }; runProcess(directory, nativeGitTimeoutInMs, command, stdoutConsumer); } catch (final InterruptedException ex) { @@ -507,75 +501,22 @@ private void runProcess( File directory, long nativeGitTimeoutInMs, String command, - final Function stdoutConsumer) throws InterruptedException, IOException, GitCommitIdExecutionException { + final Consumer stdoutLineConsumer) throws InterruptedException, IOException, GitCommitIdExecutionException { final ProcessBuilder builder = new ProcessBuilder(command.split("\\s")); final Process proc = builder.directory(directory).start(); - final ExecutorService executorService = Executors.newFixedThreadPool(2); - final StringBuilder errMsg = new StringBuilder(); - - final Future> stdoutFuture = executorService.submit( - new CallableBufferedStreamReader(proc.getInputStream(), stdoutConsumer)); - final Future> stderrFuture = executorService.submit( - new CallableBufferedStreamReader(proc.getErrorStream(), - line -> { - errMsg.append(line); - // return true to indicate we want to read more content - return true; - })); - - if (!proc.waitFor(nativeGitTimeoutInMs, TimeUnit.MILLISECONDS)) { - proc.destroy(); - executorService.shutdownNow(); - throw new RuntimeException(String.format("GIT-Command '%s' did not finish in %d milliseconds", command, nativeGitTimeoutInMs)); - } - - try { - stdoutFuture.get() - .ifPresent(e -> { - throw e; - }); - stderrFuture.get() - .ifPresent(e -> { - throw e; - }); - } catch (final ExecutionException e) { - throw new RuntimeException(String.format("Executing GIT-Command '%s' threw an '%s' exception.", command, e.getMessage()), e); - } + try (ProcessHandler processHandler = new ProcessHandler(proc, stdoutLineConsumer)) { + int exitValue = processHandler.exitValue(nativeGitTimeoutInMs, TimeUnit.MILLISECONDS); - executorService.shutdown(); - if (proc.exitValue() != 0) { - throw new NativeCommandException(proc.exitValue(), command, directory, "", errMsg.toString()); - } - } - - private static class CallableBufferedStreamReader implements Callable> { - private final InputStream is; - private final Function streamConsumer; - - CallableBufferedStreamReader(final InputStream is, final Function streamConsumer) { - this.is = is; - this.streamConsumer = streamConsumer; - } - - @Override - public Optional call() { - RuntimeException thrownException = null; - try (final BufferedReader br = new BufferedReader( - new InputStreamReader(is, StandardCharsets.UTF_8))) { - for (String line = br.readLine(); - line != null; - line = br.readLine()) { - if (!streamConsumer.apply(line)) { - break; - } - } - } catch (final IOException e) { - thrownException = new RuntimeException(String.format("Executing GIT-Command threw an '%s' exception.", e.getMessage()), e); + if (exitValue != 0) { + throw new NativeCommandException(exitValue, command, directory, "", processHandler.getStderr()); } - return Optional.ofNullable(thrownException); + } catch (TimeoutException e) { + throw new RuntimeException(String.format("GIT-Command '%s' did not finish in %d milliseconds", command, nativeGitTimeoutInMs), e); + } catch (ExecutionException e) { + throw new RuntimeException(String.format("Executing GIT-Command '%s' threw an '%s' exception.", command, e.getMessage()), e); } } } diff --git a/src/main/java/pl/project13/core/ProcessHandler.java b/src/main/java/pl/project13/core/ProcessHandler.java new file mode 100644 index 0000000..faea2fd --- /dev/null +++ b/src/main/java/pl/project13/core/ProcessHandler.java @@ -0,0 +1,160 @@ +/* + * This file is part of git-commit-id-plugin-core by Konrad 'ktoso' Malawski + * + * git-commit-id-plugin-core is free software: you can redistribute it and/or modify + * it under the terms of the GNU Lesser General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * git-commit-id-plugin-core is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with git-commit-id-plugin-core. If not, see . + */ + +package pl.project13.core; + +import java.io.BufferedReader; +import java.io.InputStream; +import java.io.InputStreamReader; +import java.nio.charset.StandardCharsets; +import java.util.Objects; +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.ThreadFactory; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; +import java.util.function.Consumer; +import java.util.function.Supplier; + +/** + * Handles waiting for a {@link Process} and reading its stdout and stderr output. + */ +class ProcessHandler implements AutoCloseable { + private final Process process; + + private final ExecutorService outputReaderExecutor; + private final Future stdoutFuture; + private final Future stderrFuture; + + private String stderrOutput = null; + + /** + * @param process the process which should be handled + * @param stdoutLineConsumer called asynchronously with the lines read from stdout. The consumer + * must either be thread-safe, or the result it is building must only be used after + * {@link #exitValue(long, TimeUnit)} has returned without throwing an exception. The + * consumer must not block, otherwise this could prevent the process from writing any + * output, causing it to get stuck. + */ + public ProcessHandler(Process process, Consumer stdoutLineConsumer) { + this.process = Objects.requireNonNull(process); + Objects.requireNonNull(stdoutLineConsumer); + + // 2 threads, one for stdout, one for stderr + // The process output is consumed concurrently by separate threads because otherwise the process + // could get stuck if the output is not consumed and the output buffer is full + ThreadFactory threadFactory = Executors.defaultThreadFactory(); + outputReaderExecutor = Executors.newFixedThreadPool(2, runnable -> { + Thread t = threadFactory.newThread(runnable); + // Don't prevent JVM exit + t.setDaemon(true); + return t; + }); + + String processInfo; + try { + processInfo = this.process.info().command().orElse("?") + " [" + this.process.pid() + "]"; + } catch (UnsupportedOperationException e) { + processInfo = ""; + } + stdoutFuture = + outputReaderExecutor.submit(new ProcessOutputReader<>("stdout reader (" + processInfo + ")", + this.process.getInputStream(), stdoutLineConsumer, + // Don't create a 'result', `stdoutLineConsumer` will do that itself if needed + () -> null)); + + StringBuilder stderrBuilder = new StringBuilder(); + stderrFuture = + outputReaderExecutor.submit(new ProcessOutputReader<>("stderr reader (" + processInfo + ")", + this.process.getErrorStream(), line -> stderrBuilder.append(line).append('\n'), + stderrBuilder::toString)); + } + + /** + * Waits for the process to finish and returns the exit value. + * + * @throws TimeoutException if waiting for the process to finish times out + */ + public int exitValue(long timeout, TimeUnit timeUnit) throws InterruptedException, TimeoutException, ExecutionException { + boolean finished = process.waitFor(timeout, timeUnit); + if (finished) { + + outputReaderExecutor.shutdown(); + try { + stdoutFuture.get(); + } catch (ExecutionException e) { + throw new ExecutionException("Failed waiting for stdout", e.getCause()); + } + try { + stderrOutput = stderrFuture.get(); + } catch (ExecutionException e) { + throw new ExecutionException("Failed waiting for stderr", e.getCause()); + } + return process.exitValue(); + } + throw new TimeoutException(); + } + + /** + * Gets the stderr output. Must only be called after {@link #exitValue(long, TimeUnit)} has + * returned successfully. + */ + public String getStderr() { + if (stderrOutput == null) { + throw new IllegalStateException("Process has not finished"); + } + return stderrOutput; + } + + @Override + public void close() { + // Perform clean-up; has no effect if process or executor have already been stopped + process.destroy(); + outputReaderExecutor.shutdownNow(); + } + + private static class ProcessOutputReader implements Callable { + private final String threadName; + private final InputStream is; + private final Consumer lineConsumer; + private final Supplier resultCreator; + + ProcessOutputReader(String threadName, InputStream is, Consumer lineConsumer, Supplier resultCreator) { + this.threadName = threadName; + this.is = is; + this.lineConsumer = lineConsumer; + this.resultCreator = resultCreator; + } + + @Override + public T call() throws Exception { + Thread.currentThread().setName(threadName); + + try (BufferedReader br = new BufferedReader(new InputStreamReader(is, StandardCharsets.UTF_8))) { + + String line; + while ((line = br.readLine()) != null) { + lineConsumer.accept(line); + } + } + return resultCreator.get(); + } + } +} diff --git a/src/test/java/pl/project13/core/ProcessHandlerTest.java b/src/test/java/pl/project13/core/ProcessHandlerTest.java new file mode 100644 index 0000000..37bbbde --- /dev/null +++ b/src/test/java/pl/project13/core/ProcessHandlerTest.java @@ -0,0 +1,193 @@ +/* + * This file is part of git-commit-id-plugin-core by Konrad 'ktoso' Malawski + * + * git-commit-id-plugin-core is free software: you can redistribute it and/or modify + * it under the terms of the GNU Lesser General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * git-commit-id-plugin-core is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with git-commit-id-plugin-core. If not, see . + */ + +package pl.project13.core; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; + +import java.io.File; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; +import java.util.concurrent.atomic.AtomicReference; +import java.util.function.Consumer; + +import org.junit.Test; + +public class ProcessHandlerTest { + @FunctionalInterface + private interface TestAction { + void run(Process p) throws Exception; + } + + private static final String STDOUT_LINE = "some text written to the stdout stream; line "; + private static final String STDERR_LINE = "some text written to the stderr stream; line "; + + private static void runJavaProcess(int exitCode, int outRepeatCount, long sleepMillis, TestAction testAction) { + // Creates a small Java program and starts it directly from the source file (JEP 330) + // This is done instead of using shell scripts to be more independent of the OS running the test + String javaBinaryPath = System.getProperty("java.home") + File.separator + "bin" + File.separator + "java"; + String javaCode = "public class Test {\n" + + " public static void main(String... args) throws Exception {\n" + + " int exitCode = Integer.parseInt(args[0]);\n" + + " int outRepeatCount = Integer.parseInt(args[1]);\n" + + " long sleepMillis = Long.parseLong(args[2]);\n" + + "\n" + + " for (int i = 0; i < outRepeatCount; i++) {\n" + + " System.out.println(\"" + STDOUT_LINE + "\" + (i + 1));\n" + + " System.err.println(\"" + STDERR_LINE + "\" + (i + 1));\n" + + " }\n" + + "\n" + + " if (sleepMillis > 0) {\n" + + " Thread.sleep(sleepMillis);\n" + + " }\n" + + "\n" + + " System.exit(exitCode);\n" + + " }\n" + + "}"; + + try { + Path tempJavaFile = Files.createTempFile("GitCommitIdPluginTest", ".java"); + try { + Files.writeString(tempJavaFile, javaCode); + String[] command = + {javaBinaryPath, tempJavaFile.toAbsolutePath().toString(), Integer.toString(exitCode), + Integer.toString(outRepeatCount), Long.toString(sleepMillis)}; + Process process = new ProcessBuilder(command).start(); + testAction.run(process); + } finally { + Files.delete(tempJavaFile); + } + } catch (Exception e) { + throw new RuntimeException("Unexpected exception", e); + } + } + + @Test + public void exitSuccess() { + int exitCode = 0; + int outputRepeatCount = 2; + long sleepMillis = 0; + + runJavaProcess(exitCode, outputRepeatCount, sleepMillis, process -> { + StringBuilder stdoutBuilder = new StringBuilder(); + try (ProcessHandler processHandler = new ProcessHandler(process, outLine -> stdoutBuilder.append(outLine).append('\n'))) { + int actualExitCode = processHandler.exitValue(5, TimeUnit.SECONDS); + String stderr = processHandler.getStderr(); + // For troubleshooting include `stderr` in message, e.g. in case there is a compilation error + assertEquals("Process failed:\n" + stderr, exitCode, actualExitCode); + assertEquals(STDOUT_LINE + "1\n" + STDOUT_LINE + "2\n", stdoutBuilder.toString()); + assertEquals(STDERR_LINE + "1\n" + STDERR_LINE + "2\n", stderr); + } + }); + } + + @Test + public void noOutput() { + int exitCode = 0; + int outputRepeatCount = 0; + long sleepMillis = 0; + + runJavaProcess(exitCode, outputRepeatCount, sleepMillis, process -> { + AtomicReference lastStdoutLine = new AtomicReference<>(); + try (ProcessHandler processHandler = new ProcessHandler(process, outLine -> lastStdoutLine.set(outLine))) { + int actualExitCode = processHandler.exitValue(5, TimeUnit.SECONDS); + String stderr = processHandler.getStderr(); + // For troubleshooting include `stderr` in message, e.g. in case there is a compilation error + assertEquals("Process failed:\n" + stderr, exitCode, actualExitCode); + assertNull(lastStdoutLine.get()); + assertEquals("", stderr); + } + }); + } + + @Test + public void exitError() { + int exitCode = 1; + int outputRepeatCount = 2; + long sleepMillis = 0; + + runJavaProcess(exitCode, outputRepeatCount, sleepMillis, process -> { + StringBuilder stdoutBuilder = new StringBuilder(); + try (ProcessHandler processHandler = new ProcessHandler(process, outLine -> stdoutBuilder.append(outLine).append('\n'))) { + int actualExitCode = processHandler.exitValue(5, TimeUnit.SECONDS); + assertEquals(exitCode, actualExitCode); + assertEquals(STDOUT_LINE + "1\n" + STDOUT_LINE + "2\n", stdoutBuilder.toString()); + assertEquals(STDERR_LINE + "1\n" + STDERR_LINE + "2\n", processHandler.getStderr()); + } + }); + } + + @Test + public void timeout() { + int exitCode = 0; + int outputRepeatCount = 2; + long sleepMillis = TimeUnit.SECONDS.toMillis(3); + + runJavaProcess(exitCode, outputRepeatCount, sleepMillis, process -> { + // Ignore stdout; it is not deterministic how many output has already been read + Consumer stdoutConsumer = line -> {}; + try (ProcessHandler processHandler = new ProcessHandler(process, stdoutConsumer)) { + assertThrows(TimeoutException.class, () -> processHandler.exitValue(1, TimeUnit.MILLISECONDS)); + assertThrows(IllegalStateException.class, processHandler::getStderr); + } + }); + } + + /** + * Tests behavior when the process writes large amounts of output to stdout and stderr. + * + *

+ * If the code handling the {@link Process} does not asynchronously read the output while waiting + * for the process to finish, the process can get stuck because the output buffers are full. This + * test verifies that this does not occur. + */ + @Test + public void largeOutput() { + /* + * Note: Can replicate the process output buffer becoming full, and the process getting stuck by + * either setting a breakpoint in the `stdoutLineConsumer` lambda, or by changing it to perform + * a sleep. + * Unlike the other tests in this class this will then cause a timeout while waiting for the + * process to finish (because the process is blocked writing any further data to stdout). + * + * (might be OS and configuration dependent whether this can be reproduced) + */ + + int exitCode = 0; + int outputRepeatCount = 2_000; + long sleepMillis = 0; + + runJavaProcess(exitCode, outputRepeatCount, sleepMillis, process -> { + AtomicReference lastStdoutLine = new AtomicReference<>(); + try (ProcessHandler processHandler = new ProcessHandler(process, outLine -> lastStdoutLine.set(outLine))) { + int actualExitCode = processHandler.exitValue(5, TimeUnit.SECONDS); + String stderr = processHandler.getStderr(); + // For troubleshooting include `stderr` in message, e.g. in case there is a compilation error + assertEquals("Process failed:\n" + stderr, exitCode, actualExitCode); + assertEquals(STDOUT_LINE + outputRepeatCount, lastStdoutLine.get()); + + assertTrue(stderr.startsWith(STDERR_LINE + 1)); + assertTrue(stderr.endsWith(STDERR_LINE + outputRepeatCount + "\n")); + } + }); + } +}