Skip to content

Commit a5f1676

Browse files
authored
Merge pull request #101 from Marcono1234/process-handling
Move process output handling to separate class & add tests
2 parents d92eba9 + af7092f commit a5f1676

File tree

3 files changed

+369
-75
lines changed

3 files changed

+369
-75
lines changed

src/main/java/pl/project13/core/NativeGitProvider.java

+16-75
Original file line numberDiff line numberDiff line change
@@ -25,12 +25,11 @@
2525
import javax.annotation.Nonnull;
2626

2727
import java.io.*;
28-
import java.nio.charset.StandardCharsets;
2928
import java.text.SimpleDateFormat;
3029
import java.util.Optional;
3130
import java.util.concurrent.*;
3231
import java.util.concurrent.atomic.AtomicBoolean;
33-
import java.util.function.Function;
32+
import java.util.function.Consumer;
3433
import java.util.stream.Collectors;
3534
import java.util.stream.Stream;
3635

@@ -338,7 +337,7 @@ private String getOriginRemote(File directory, long nativeGitTimeoutInMs) throws
338337
}
339338

340339
/**
341-
* Runs a maven command and returns {@code true} if output was non empty.
340+
* Runs a Git command and returns {@code true} if output was non empty.
342341
* Can be used to short cut reading output from command when we know it may be a rather long one.
343342
* Return true if the result is empty.
344343
**/
@@ -468,12 +467,10 @@ public String run(File directory, long nativeGitTimeoutInMs, String command) thr
468467
try {
469468
final StringBuilder commandResult = new StringBuilder();
470469

471-
final Function<String, Boolean> stdoutConsumer = line -> {
470+
final Consumer<String> stdoutConsumer = line -> {
472471
if (line != null) {
473-
commandResult.append(line).append("\n");
472+
commandResult.append(line).append('\n');
474473
}
475-
// return true to indicate we want to read more content
476-
return true;
477474
};
478475
runProcess(directory, nativeGitTimeoutInMs, command, stdoutConsumer);
479476

@@ -489,12 +486,9 @@ public boolean runEmpty(File directory, long nativeGitTimeoutInMs, String comman
489486
final AtomicBoolean empty = new AtomicBoolean(true);
490487

491488
try {
492-
final Function<String, Boolean> stdoutConsumer = line -> {
493-
if (line != null) {
494-
empty.set(false);
495-
}
496-
// return false to indicate we don't need to read more content
497-
return false;
489+
final Consumer<String> stdoutConsumer = line -> {
490+
empty.set(false);
491+
// Ignore the content of the line
498492
};
499493
runProcess(directory, nativeGitTimeoutInMs, command, stdoutConsumer);
500494
} catch (final InterruptedException ex) {
@@ -507,75 +501,22 @@ private void runProcess(
507501
File directory,
508502
long nativeGitTimeoutInMs,
509503
String command,
510-
final Function<String, Boolean> stdoutConsumer) throws InterruptedException, IOException, GitCommitIdExecutionException {
504+
final Consumer<String> stdoutLineConsumer) throws InterruptedException, IOException, GitCommitIdExecutionException {
511505

512506
final ProcessBuilder builder = new ProcessBuilder(command.split("\\s"));
513507
final Process proc = builder.directory(directory).start();
514508

515-
final ExecutorService executorService = Executors.newFixedThreadPool(2);
516-
final StringBuilder errMsg = new StringBuilder();
517-
518-
final Future<Optional<RuntimeException>> stdoutFuture = executorService.submit(
519-
new CallableBufferedStreamReader(proc.getInputStream(), stdoutConsumer));
520-
final Future<Optional<RuntimeException>> stderrFuture = executorService.submit(
521-
new CallableBufferedStreamReader(proc.getErrorStream(),
522-
line -> {
523-
errMsg.append(line);
524-
// return true to indicate we want to read more content
525-
return true;
526-
}));
527-
528-
if (!proc.waitFor(nativeGitTimeoutInMs, TimeUnit.MILLISECONDS)) {
529-
proc.destroy();
530-
executorService.shutdownNow();
531-
throw new RuntimeException(String.format("GIT-Command '%s' did not finish in %d milliseconds", command, nativeGitTimeoutInMs));
532-
}
533-
534-
try {
535-
stdoutFuture.get()
536-
.ifPresent(e -> {
537-
throw e;
538-
});
539-
stderrFuture.get()
540-
.ifPresent(e -> {
541-
throw e;
542-
});
543-
} catch (final ExecutionException e) {
544-
throw new RuntimeException(String.format("Executing GIT-Command '%s' threw an '%s' exception.", command, e.getMessage()), e);
545-
}
509+
try (ProcessHandler processHandler = new ProcessHandler(proc, stdoutLineConsumer)) {
510+
int exitValue = processHandler.exitValue(nativeGitTimeoutInMs, TimeUnit.MILLISECONDS);
546511

547-
executorService.shutdown();
548-
if (proc.exitValue() != 0) {
549-
throw new NativeCommandException(proc.exitValue(), command, directory, "", errMsg.toString());
550-
}
551-
}
552-
553-
private static class CallableBufferedStreamReader implements Callable<Optional<RuntimeException>> {
554-
private final InputStream is;
555-
private final Function<String, Boolean> streamConsumer;
556-
557-
CallableBufferedStreamReader(final InputStream is, final Function<String, Boolean> streamConsumer) {
558-
this.is = is;
559-
this.streamConsumer = streamConsumer;
560-
}
561-
562-
@Override
563-
public Optional<RuntimeException> call() {
564-
RuntimeException thrownException = null;
565-
try (final BufferedReader br = new BufferedReader(
566-
new InputStreamReader(is, StandardCharsets.UTF_8))) {
567-
for (String line = br.readLine();
568-
line != null;
569-
line = br.readLine()) {
570-
if (!streamConsumer.apply(line)) {
571-
break;
572-
}
573-
}
574-
} catch (final IOException e) {
575-
thrownException = new RuntimeException(String.format("Executing GIT-Command threw an '%s' exception.", e.getMessage()), e);
512+
if (exitValue != 0) {
513+
throw new NativeCommandException(exitValue, command, directory, "", processHandler.getStderr());
576514
}
577515

578-
return Optional.ofNullable(thrownException);
516+
} catch (TimeoutException e) {
517+
throw new RuntimeException(String.format("GIT-Command '%s' did not finish in %d milliseconds", command, nativeGitTimeoutInMs), e);
518+
} catch (ExecutionException e) {
519+
throw new RuntimeException(String.format("Executing GIT-Command '%s' threw an '%s' exception.", command, e.getMessage()), e);
579520
}
580521
}
581522
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,160 @@
1+
/*
2+
* This file is part of git-commit-id-plugin-core by Konrad 'ktoso' Malawski <[email protected]>
3+
*
4+
* git-commit-id-plugin-core is free software: you can redistribute it and/or modify
5+
* it under the terms of the GNU Lesser General Public License as published by
6+
* the Free Software Foundation, either version 3 of the License, or
7+
* (at your option) any later version.
8+
*
9+
* git-commit-id-plugin-core is distributed in the hope that it will be useful,
10+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
11+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
12+
* GNU General Public License for more details.
13+
*
14+
* You should have received a copy of the GNU Lesser General Public License
15+
* along with git-commit-id-plugin-core. If not, see <http://www.gnu.org/licenses/>.
16+
*/
17+
18+
package pl.project13.core;
19+
20+
import java.io.BufferedReader;
21+
import java.io.InputStream;
22+
import java.io.InputStreamReader;
23+
import java.nio.charset.StandardCharsets;
24+
import java.util.Objects;
25+
import java.util.concurrent.Callable;
26+
import java.util.concurrent.ExecutionException;
27+
import java.util.concurrent.ExecutorService;
28+
import java.util.concurrent.Executors;
29+
import java.util.concurrent.Future;
30+
import java.util.concurrent.ThreadFactory;
31+
import java.util.concurrent.TimeUnit;
32+
import java.util.concurrent.TimeoutException;
33+
import java.util.function.Consumer;
34+
import java.util.function.Supplier;
35+
36+
/**
37+
* Handles waiting for a {@link Process} and reading its stdout and stderr output.
38+
*/
39+
class ProcessHandler implements AutoCloseable {
40+
private final Process process;
41+
42+
private final ExecutorService outputReaderExecutor;
43+
private final Future<Void> stdoutFuture;
44+
private final Future<String> stderrFuture;
45+
46+
private String stderrOutput = null;
47+
48+
/**
49+
* @param process the process which should be handled
50+
* @param stdoutLineConsumer called asynchronously with the lines read from stdout. The consumer
51+
* must either be thread-safe, or the result it is building must only be used after
52+
* {@link #exitValue(long, TimeUnit)} has returned without throwing an exception. The
53+
* consumer must not block, otherwise this could prevent the process from writing any
54+
* output, causing it to get stuck.
55+
*/
56+
public ProcessHandler(Process process, Consumer<String> stdoutLineConsumer) {
57+
this.process = Objects.requireNonNull(process);
58+
Objects.requireNonNull(stdoutLineConsumer);
59+
60+
// 2 threads, one for stdout, one for stderr
61+
// The process output is consumed concurrently by separate threads because otherwise the process
62+
// could get stuck if the output is not consumed and the output buffer is full
63+
ThreadFactory threadFactory = Executors.defaultThreadFactory();
64+
outputReaderExecutor = Executors.newFixedThreadPool(2, runnable -> {
65+
Thread t = threadFactory.newThread(runnable);
66+
// Don't prevent JVM exit
67+
t.setDaemon(true);
68+
return t;
69+
});
70+
71+
String processInfo;
72+
try {
73+
processInfo = this.process.info().command().orElse("?") + " [" + this.process.pid() + "]";
74+
} catch (UnsupportedOperationException e) {
75+
processInfo = "<unknown-process>";
76+
}
77+
stdoutFuture =
78+
outputReaderExecutor.submit(new ProcessOutputReader<>("stdout reader (" + processInfo + ")",
79+
this.process.getInputStream(), stdoutLineConsumer,
80+
// Don't create a 'result', `stdoutLineConsumer` will do that itself if needed
81+
() -> null));
82+
83+
StringBuilder stderrBuilder = new StringBuilder();
84+
stderrFuture =
85+
outputReaderExecutor.submit(new ProcessOutputReader<>("stderr reader (" + processInfo + ")",
86+
this.process.getErrorStream(), line -> stderrBuilder.append(line).append('\n'),
87+
stderrBuilder::toString));
88+
}
89+
90+
/**
91+
* Waits for the process to finish and returns the exit value.
92+
*
93+
* @throws TimeoutException if waiting for the process to finish times out
94+
*/
95+
public int exitValue(long timeout, TimeUnit timeUnit) throws InterruptedException, TimeoutException, ExecutionException {
96+
boolean finished = process.waitFor(timeout, timeUnit);
97+
if (finished) {
98+
99+
outputReaderExecutor.shutdown();
100+
try {
101+
stdoutFuture.get();
102+
} catch (ExecutionException e) {
103+
throw new ExecutionException("Failed waiting for stdout", e.getCause());
104+
}
105+
try {
106+
stderrOutput = stderrFuture.get();
107+
} catch (ExecutionException e) {
108+
throw new ExecutionException("Failed waiting for stderr", e.getCause());
109+
}
110+
return process.exitValue();
111+
}
112+
throw new TimeoutException();
113+
}
114+
115+
/**
116+
* Gets the stderr output. Must only be called after {@link #exitValue(long, TimeUnit)} has
117+
* returned successfully.
118+
*/
119+
public String getStderr() {
120+
if (stderrOutput == null) {
121+
throw new IllegalStateException("Process has not finished");
122+
}
123+
return stderrOutput;
124+
}
125+
126+
@Override
127+
public void close() {
128+
// Perform clean-up; has no effect if process or executor have already been stopped
129+
process.destroy();
130+
outputReaderExecutor.shutdownNow();
131+
}
132+
133+
private static class ProcessOutputReader<T> implements Callable<T> {
134+
private final String threadName;
135+
private final InputStream is;
136+
private final Consumer<String> lineConsumer;
137+
private final Supplier<T> resultCreator;
138+
139+
ProcessOutputReader(String threadName, InputStream is, Consumer<String> lineConsumer, Supplier<T> resultCreator) {
140+
this.threadName = threadName;
141+
this.is = is;
142+
this.lineConsumer = lineConsumer;
143+
this.resultCreator = resultCreator;
144+
}
145+
146+
@Override
147+
public T call() throws Exception {
148+
Thread.currentThread().setName(threadName);
149+
150+
try (BufferedReader br = new BufferedReader(new InputStreamReader(is, StandardCharsets.UTF_8))) {
151+
152+
String line;
153+
while ((line = br.readLine()) != null) {
154+
lineConsumer.accept(line);
155+
}
156+
}
157+
return resultCreator.get();
158+
}
159+
}
160+
}

0 commit comments

Comments
 (0)