Bug 849394

The problem is that the I/O threads are never run to completion: The main process waits for the exit status, but the I/O threads may not have starts.

I will provide a patch, but the solution is simply to return a handle containing both stdout/stderr Thread instances and wait for these threads...

modules/core/util/src/main/java/org/rhq/core/util/exec/ProcessExecutor.java

    /**
     * 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;

...

    protected RedirectThreads redirectStreams(ProcessToStart process, Process childProcess) throws IOException {
...
        return new RedirectThreads(stdoutThread, stderrThread);

then ... 

+        final RedirectThreads redirect = redirectStreams(process, childProcess);
...
-                        int exitCode = childProcess.waitFor();
-                        retExitCode[0] = new Integer(exitCode);
+                    } finally {
+                        redirect.join();
                     }

Simple as this.

Created attachment 612648 [details]
fixes process leak

(In reply to comment #2)
> Created attachment 612648 [details]
> fixes process leak

I think the wrong patch was attached - the attached patch deals with PIQL queries. Can you remove that patch and attach the one for this issue?

Created attachment 614123 [details]
Ensure output thread completes before exiting startProgram

Sorry I had the wrong patch.

One thing you might want to adjust (for sake of the build) is adjust the number of times we check for correct output:

modules/core/util/src/test/java/org/rhq/core/util/exec/ProcessExecTest.java

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 49ab3f5..60b7aed 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
@@ -35,7 +35,7 @@
 public class ProcessExecTest {
     public void testProcessExecOutputStream() {
         // run multiple times to ensure race condition fixed
-        for (int i = 0; i < 100; i++) {
+        for (int i = 0; i < 10; i++) {
             ProcessToStart start = new ProcessToStart();

             setupProgram(start);

I also have a version with thread pools being attached. I don't think a thread pool is worth it, but might be good idea.

Created attachment 614124 [details]
optional use of a thread pool

Additional possible change to use a thread pool to handle I/O redirect. Might be good if there are a lot of script executions, though I don't know if it is worth it for the average case.

git commit to master: bf4b25f421769909e886eb42f7b99c66b207b200

Setting correct Target Release.

need to fix the api change (redirectStreams is protected, so we want to maintain that API and @Deprecate it, and change the name of the new method and use that one).

git commit to deprecate old API: e4b77aefd05bd093816e46792a928c49d2f3b402

Bulk closing of items that are on_qa and in old RHQ releases, which are out for a long time and where the issue has not been re-opened since.