Attachment 614123 Details for Bug 849394 – Ensure output thread completes before exiting startProgram

[patch] Ensure output thread completes before exiting startProgram

0001-BZ-849394-rhq-script-plugin-does-not-always-capture-.patch (text/plain), 10.15 KB, created by Elias Ross on 2012-09-18 21:07:02 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Elias Ross

Created: 2012-09-18 21:07:02 UTC

Size: 10.15 KB

patch

obsolete

>From 4fc83f340284e2617bdd2afe897181835c70bf75 Mon Sep 17 00:00:00 2001
>From: Elias Ross <elias_ross@apple.com>
>Date: Sat, 18 Aug 2012 23:03:34 -0700
>Subject: [PATCH 1/2] [BZ 849394] 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.
>---
> .../org/rhq/core/util/exec/ProcessExecutor.java    |   92 ++++++++++++++------
> .../org/rhq/core/util/exec/ProcessExecTest.java    |   18 +---
> 2 files changed, 69 insertions(+), 41 deletions(-)
>
>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.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 @@
>  * @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 ProcessExecutorResults execute(ProcessToStart processToStart) {
>      *
>      * @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 @@ protected Integer startProgram(final ProcessToStart process) throws Exception {
>         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 void run() {
>      * @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 @@ protected void redirectStreams(ProcessToStart process, Process childProcess) thr
>         }
> 
>         StreamRedirector stdoutThread = new StreamRedirector(threadNamePrefix + "-stdout", stdout, fileOutputStream);
>-
>         StreamRedirector stderrThread = new StreamRedirector(threadNamePrefix + "-stderr", stderr, fileOutputStream);
> 
>         stdoutThread.start();
>@@ -201,7 +239,7 @@ protected void redirectStreams(ProcessToStart process, Process childProcess) thr
> 
>         stdin.close();
> 
>-        return;
>+        return new RedirectThreads(stdoutThread, stderrThread);
>     }
> 
>     /**
>@@ -372,7 +410,7 @@ protected String getFullProgramExecutablePath(ProcessToStart process) throws Fil
>         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 @@ private void renameFile(File file) throws IOException {
>         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 @@
> @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 void testProcessExecOutputStream() {
>             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 @@ private void setupProgram(ProcessToStart start) {
>             start.setArguments(new String[] { "/bin" });
>         }
>     }
>-}
>\ No newline at end of file
>+}
>-- 
>1.7.9.3
>

Actions: View | Diff

Attachments on bug 849394: 612648 | 614123 | 614124