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@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 +}