modules/core/util/src/main/java/org/rhq/core/util/exec/ProcessExecutor.java | 92
+++++++---
modules/core/util/src/test/java/org/rhq/core/util/exec/ProcessExecTest.java | 18 -
2 files changed, 69 insertions(+), 41 deletions(-)
New commits:
commit 401c7a219876e5af9075ff09ee1bbb83cca6b884
Author: Elias Ross <elias_ross(a)apple.com>
Date: Sat Aug 18 23:03:34 2012 -0700
rhq-script-plugin does not always capture process output
Ensure output thread completes before exiting startProgram()
Use concurrent library to clean up exit code processing.
(cherry picked from commit bf4b25f421769909e886eb42f7b99c66b207b200)
diff --git a/modules/core/util/src/main/java/org/rhq/core/util/exec/ProcessExecutor.java
b/modules/core/util/src/main/java/org/rhq/core/util/exec/ProcessExecutor.java
index ccbe045..308d9bd 100644
--- a/modules/core/util/src/main/java/org/rhq/core/util/exec/ProcessExecutor.java
+++ b/modules/core/util/src/main/java/org/rhq/core/util/exec/ProcessExecutor.java
@@ -33,6 +33,12 @@ import java.io.InputStream;
import java.io.OutputStream;
import java.text.SimpleDateFormat;
import java.util.Date;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
import org.rhq.core.util.UtilI18NResourceKeys;
@@ -45,6 +51,9 @@ import org.rhq.core.util.UtilI18NResourceKeys;
* @author John Mazzitelli
*/
public class ProcessExecutor {
+
+ private static ExecutorService threadPool = Executors.newCachedThreadPool();
+
/**
* This executes any operating system process as described in the given start
command. When this method returns, it
* can be assumed that the process was launched but not necessarily finished. The
caller can ask this method to
@@ -78,7 +87,6 @@ public class ProcessExecutor {
*
* @return process exit code (if the method waited for it to exit) or
<code>null</code> if this method was to only
* start the process but not wait or was to wait and the wait time expired
before the process exited
- *
* @throws Exception if any error occurs while trying to start the child process
*/
protected Integer startProgram(final ProcessToStart process) throws Exception {
@@ -91,47 +99,77 @@ public class ProcessExecutor {
final Process childProcess = Runtime.getRuntime().exec(cmdline, environment,
workingDir);
// redirect the program's streams
- // WARNING:
- // It seems there is no way to get around a possible race condition - what if the
process
- // was so fast that it exited already? We didn't get a chance to capture its
output.
- // I see a unit test that periodically fails because it doesn't get any
captured output when
- // it should - I think it is because of this race condition. But there is no
Java API that
- // let's me redirect a process' streams before the process is told to
start.
- redirectStreams(process, childProcess);
-
- final Integer[] retExitCode = new Integer[1];
+ final RedirectThreads redirect = redirectStreams(process, childProcess);
+ Integer exitCode = null;
// wait if told to - note that the default is not to wait
if (process.getWaitForExit().intValue() > 0) {
- Thread waitThread = new Thread("ExecuteProcess-" +
process.getProgramTitle()) {
- public void run() {
+ Callable<Integer> call = new Callable() {
+ public Integer call() throws Exception {
+ Thread.currentThread().setName("ExecuteProcess-" +
process.getProgramTitle());
try {
- int exitCode = childProcess.waitFor();
- retExitCode[0] = new Integer(exitCode);
- } catch (InterruptedException e) {
+ return childProcess.waitFor();
+ } finally {
+ // wait for I/O to finish
+ redirect.join();
}
}
};
-
- waitThread.setDaemon(true);
- waitThread.start();
+ Future<Integer> future = threadPool.submit(call);
try {
- waitThread.join(process.getWaitForExit().intValue());
+ exitCode = future.get(process.getWaitForExit().intValue(),
TimeUnit.MILLISECONDS);
} catch (InterruptedException ie) {
// this might happen if the launching thread got interrupted
+ Thread.currentThread().interrupt();
+ } catch (TimeoutException e) {
+ // the documentation requires we return null
} finally {
- waitThread.interrupt();
+ future.cancel(true);
}
- if (retExitCode[0] == null) {
+ if (exitCode == null) {
// never got the exit code so the wait time must have expired, kill the
process if configured to do so
if (process.isKillOnTimeout().booleanValue()) {
childProcess.destroy();
}
+ // cancel the output threads
+ redirect.interrupt();
}
}
- return retExitCode[0];
+ return exitCode;
+ }
+
+ /**
+ * Wrapper for threads used for capturing output.
+ * Call {@link #join} to wait for output to be fully captured.
+ */
+ protected static class RedirectThreads {
+
+ private final StreamRedirector stdout;
+ private final StreamRedirector stderr;
+
+ private RedirectThreads(StreamRedirector stdout, StreamRedirector stderr) {
+ this.stdout = stdout;
+ this.stderr = stderr;
+ }
+
+ /**
+ * Waits for output to be fully captured.
+ */
+ public void join() throws InterruptedException {
+ stderr.join();
+ stdout.join();
+ }
+
+ /**
+ * Interrupts these threads.
+ */
+ public void interrupt() {
+ stderr.interrupt();
+ stdout.interrupt();
+ }
+
}
/**
@@ -146,8 +184,9 @@ public class ProcessExecutor {
* @param childProcess the newly spawned child process
*
* @throws IOException if failed to pipe data to/from stdin/stdout
+ * @return RedirectThreads containing a handle to the threads redirecting output
*/
- protected void redirectStreams(ProcessToStart process, Process childProcess) throws
IOException {
+ protected RedirectThreads redirectStreams(ProcessToStart process, Process
childProcess) throws IOException {
// Process.getInputStream is actually the process's stdout output
// Process.getOutputStream is actually the process's stdin intput
// Process.getErrorStream is the process's stderr output
@@ -175,7 +214,6 @@ public class ProcessExecutor {
}
StreamRedirector stdoutThread = new StreamRedirector(threadNamePrefix +
"-stdout", stdout, fileOutputStream);
-
StreamRedirector stderrThread = new StreamRedirector(threadNamePrefix +
"-stderr", stderr, fileOutputStream);
stdoutThread.start();
@@ -201,7 +239,7 @@ public class ProcessExecutor {
stdin.close();
- return;
+ return new RedirectThreads(stdoutThread, stderrThread);
}
/**
@@ -372,7 +410,7 @@ public class ProcessExecutor {
String result = progFile.getPath();
// If executable verification has been turned off then assume the caller wants
his executable "as-is".
- // Otherwise, validate and ensure a full path.
+ // Otherwise, validate and ensure a full path.
if (Boolean.TRUE.equals(process.isCheckExecutableExists())) {
if (!progFile.exists()) {
throw new FileNotFoundException(UtilI18NResourceKeys.MSG.getMsg(
@@ -453,4 +491,4 @@ public class ProcessExecutor {
String newFileName = file.getCanonicalPath() + timestamp;
file.renameTo(new File(newFileName));
}
-}
\ No newline at end of file
+}
diff --git a/modules/core/util/src/test/java/org/rhq/core/util/exec/ProcessExecTest.java
b/modules/core/util/src/test/java/org/rhq/core/util/exec/ProcessExecTest.java
index 97dcdaa..49ab3f5 100644
--- a/modules/core/util/src/test/java/org/rhq/core/util/exec/ProcessExecTest.java
+++ b/modules/core/util/src/test/java/org/rhq/core/util/exec/ProcessExecTest.java
@@ -34,10 +34,8 @@ import org.testng.annotations.Test;
@Test
public class ProcessExecTest {
public void testProcessExecOutputStream() {
- int tries = 0;
-
- while (true) {
- tries++;
+ // run multiple times to ensure race condition fixed
+ for (int i = 0; i < 100; i++) {
ProcessToStart start = new ProcessToStart();
setupProgram(start);
@@ -52,16 +50,8 @@ public class ProcessExecTest {
assert results.getError() == null : "Should not have failed: " +
results;
assert results.getExitCode() != null : "Should have had exit code:
" + results;
- // there are some times when we can't get the output - see comments in
ProcessExecutor.startProgram
- // if we failed to get the output this time, let's try again. This is
just allowing that rare
- // condition to occur in our test - I know of no way via the Java API to
avoid it, so let's not
- // fail our test just because it happened once (but do fail if we can't
get the output after so many tries)
byte[] output = baos.toByteArray();
- if (output.length > 0) {
- return; // we did get output so everything succeeded! we can pass the
test now and just return
- }
-
- if (tries >= 3) {
+ if (output.length == 0) {
assert false : "Should have had some output: " + results;
}
}
@@ -116,4 +106,4 @@ public class ProcessExecTest {
start.setArguments(new String[] { "/bin" });
}
}
-}
\ No newline at end of file
+}