This discussion is archived
1 2 Previous Next 15 Replies Latest reply: Dec 22, 2012 1:47 PM by 852321 RSS

race condition with Runtime.exec()?

852321 Newbie
Currently Being Moderated
Hi all -- I'm having an issue trying to execute a shell command to determine architecture type (uname -m on Linux and Mac). For the record I have read both of the following links thoroughly, and several times:

When Runtime.exec() won't: http://www.javaworld.com/jw-12-2000/jw-1229-traps.html

Five Common java.lang.Process Pitfalls: http://kylecartmell.com/?p=9




The problem seems to be some sort of race condition with the StreamGobblers, which are streams for STDOUT and STDERR. I process the streams and invoke waitFor() and then I package up the return code and the output from the two streams into an Object array. However, it seems like by the time it's returning the Object array, the streams haven't been properly flushed. Just because the initial thread has finished and received a return value doesn't mean that the two gobbler threads have finished flushing their data out...does it?

I'm looking at the first article there and I would assume they need to call join() on the StreamGobblers before invoking p.waitFor(). It seems to resolve the problem for me. Am I wrong about this? If I don't call join() on those threads then I don't see any output in my caller method unless I introduce some sort of delay in the callee, such as stepping through the debugger, or adding in a call to Thread.sleep() right before invoking p.waitFor().

This article is 12 years old so I would expect that someone would have corrected them by now. I can't honestly believe nobody ever noticed this until now, especially with all of the comments there saying that the article saved them. Am I just missing something here? It sure seems like they need to call join() on the gobblers. See "Listing 4.5 GoodWindowsExec.java" on page 4 (http://www.javaworld.com/jw-12-2000/jw-1229-traps.html?page=4) and then compare that to my proposed changes below:
// a bunch of setup stuff above here but not necessary for this example
            Runtime rt = Runtime.getRuntime();
            System.out.println("Execing " + cmd[0] + " " + cmd[1] 
                               + " " + cmd[2]);
            Process proc = rt.exec(cmd);
            // any error message?
            StreamGobbler errorGobbler = new 
                StreamGobbler(proc.getErrorStream(), "ERROR");            
            
            // any output?
            StreamGobbler outputGobbler = new 
                StreamGobbler(proc.getInputStream(), "OUTPUT");
                
            // kick them off
            errorGobbler.start();
            outputGobbler.start();
            errorGobbler.join();      // NEED TO ADD THIS LINE
            outputGobbler.join();      // NEED TO ADD THIS LINE
                                    
            // any error???
            int exitVal = proc.waitFor();
            System.out.println("ExitValue: " + exitVal);

            // I return an object array like this to my caller:
            Object[] ret = new Object[3];
            ret[0] = new Integer(exitVal);
            ret[1] = outputGobbler.getOutputStream();  // I implemented getOutputStream in my version to return a String
            ret[2] = errorGobbler.getOutputStream();   // rather than printing everything with System.out.println()
            return ret;        
        } catch (Throwable t)
          {
            t.printStackTrace();
          }
    }
}
Edited by: gamblor01 on Dec 18, 2012 3:42 PM

Edited by: gamblor01 on Dec 18, 2012 3:43 PM

Edited by: gamblor01 on Dec 18, 2012 3:46 PM
  • 1. Re: race condition with Runtime.exec()?
    sabre150 Expert
    Currently Being Moderated
    The original 'traps' article is still valid but it is not the whole story. If the program can exit before the 'gobbler' threads have terminated then a join() is needed or the exit takes place too soon. I place the join() after the waitFor() rather than before the waitFor() in all my examples but in real world situations I find it is rarely needed. Still, I am surprised that you need the join() since I assume the 'gobbler' threads are not daemon threads so the application cannot exit until the 'gobbler' threads exit. We don't have a view of your 'gobbler' code and until I see that I cannot comment further. Of course an SSCCE would allow us to investigate and comment with more authority.

    One point to consider - since you are not passing anything to the Process 'stdin' you can do away with one of the 'gobbler' threads and process either 'stdout' or 'stderr' (but not both) in the main thread. In general you need a separate thread for each of the Process streams but you can normally use the main thread for one of the three streams.
  • 2. Re: race condition with Runtime.exec()?
    852321 Newbie
    Currently Being Moderated
    Thanks for the reply sabre. I think I actually might have figured it out. Isn't it the case that System.out.println() is synchronized? If that is the case then the println statements in the JavaWorld example should finish prior to the call that prints the return val...yes?

    I think the problem is that my gobblers are looping through output using BufferedReader to process the InputStream and simply appending each line to a StringBuffer. Then the call to StreamGobbler.getOutput() simply returns the buffer.toString(). There is nothing synchronized about this so of course I'm running into a race condition.

    I don't know...maybe I'm just wrong. Anyway, I don't feel like firing up VPN tonight and copying the code. I'll do that tomorrow when I get to work and I can post exactly what I have. I have been putting in too many hours at work lately so this will just have to wait for now. Playing with the kids and cracking open a beer take precedence tonight. :)
  • 3. Re: race condition with Runtime.exec()?
    EJP Guru
    Currently Being Moderated
    You may need to close the process's input stream so it will exit.
  • 4. Re: race condition with Runtime.exec()?
    852321 Newbie
    Currently Being Moderated
    Ok...here is an example below. Compile this and run it. Sometimes it will print only the return code, sometimes it will print both the return code and the uname result, and sometimes it will throw an exception (which is likely because the main thread has already destroyed the Process object before the gobbler thread(s) has/have had a chance to fully process the output stream).

    Uncomment the calls to join and it will work every time. Again though, I believe the JavaWorld example only succeeds because calls to println() are synchronized. If they had chosen a different example, such as appending to a StringBuffer as I do then they would need to call join, synchronize blocks, etc. I'm just looking for confirmation here. I'm fairly competent with threads but it's always good to reinforce what I know and/or learn more. Thanks!

        public void println(Object x) {
            String s = String.valueOf(x);
            synchronized (this) {
                print(s);
                newLine();
            }
        }
    import java.io.BufferedReader;
    import java.io.IOException;
    import java.io.InputStream;
    import java.io.InputStreamReader;
    
    
    public class Test
    {
         public static void main(String[] args)
         {
              try
              {
                   Object[] ret = executeProcess("uname -m");
                 System.out.println(ret[0]);
                 System.out.println(ret[1]);
                 System.out.println(ret[2]);
              }
              catch (Exception e)
              {
                   e.printStackTrace();
              }
         }
         
         
         /**
         * Executes the specified command and arguments in a separate process.  This a convenience method.  An
         * invocation of this method behaves in exactly the same way as {@link executeProcess(cmdArray)}, where
         * cmdArray is an array of all the tokens in <code>command</code>.
         * @param command A specified system command.
         * @return An object array with three items in it arranged in the following manner:<br/>
         * - object[0]: an Integer containing the return code of the command from the OS<br/>
         * - object[1]: a String containing the output the process sent to STDOUT<br/>
         * - object[2]: a String containing the output the process sent to STDERR
         * @throws TRDIException
         */
        public static Object[] executeProcess(String command) throws Exception
        {
             return executeProcess(command.split(" "));
        }
         
        
         /**
         * Executes the specified command and arguments in a separate process.
         * @param cmdArray An array containing the command to call and its arguments.
         * @return An object array with three items in it arranged in the following manner:<br/>
         * - object[0]: an Integer containing the return code of the command from the OS<br/>
         * - object[1]: a String containing the output the process sent to STDOUT<br/>
         * - object[2]: a String containing the output the process sent to STDERR
         * @throws TRDIException
         */
        public static Object[] executeProcess(String[] cmdArray) throws Exception
        {
             Object[] ret = new Object[3];
             int returnCode = -1;
             Runtime runtime = Runtime.getRuntime();
             Thread outputStream = null;               // wrapper thread for output gobbler
             Thread errorStream = null;               // wrapper thread for error gobbler
             StreamGobbler outputGobbler = null;     // gobbler for STDOUT
             StreamGobbler errorGobbler = null;     // gobbler for STDERR
             Process p = null;
             
             try
             {
                  // execute process
                  p = runtime.exec(cmdArray);
                  
                  // obtain both STDOUT and STDERR streams
                 outputGobbler = new StreamGobbler(p.getInputStream());
                 errorGobbler = new StreamGobbler(p.getErrorStream());
                 outputStream = new Thread(outputGobbler);
                 errorStream = new Thread(errorGobbler);
                 outputStream.start();
                 errorStream.start();
                 //outputStream.join();
                 //errorStream.join();
                 
                 // save the return code
                 returnCode = p.waitFor();
            }
            catch (Throwable e)
            {
                 throw new Exception(e);
            }
             finally
             {
                  try
                  {
                       // clean up all resources
                       if (p.getInputStream() != null) {
                            p.getInputStream().close();
                       }
                       if (p.getOutputStream() != null) {
                            p.getOutputStream().close();
                       }
                       if (p.getErrorStream() != null) {
                            p.getErrorStream().close();
                       }
                  }
                  catch (Throwable e)
                  {
                       throw new Exception(e);
                  }
                  finally
                  {
                       p.destroy();
                  }
             }
            
            
            if (outputGobbler.hasException()) {
                 throw outputGobbler.getException();
            }
            else if (errorGobbler.hasException()) {
                 throw errorGobbler.getException();
            }
             
             ret[0] = new Integer(returnCode);
             ret[1] = outputGobbler.getOutput();
             ret[2] = errorGobbler.getOutput();
             return ret;
        }
    }
    
    
    
    
    class StreamGobbler implements Runnable
    {
         private InputStream input;
         private StringBuffer output;
         private Exception e;
         
         public StreamGobbler(InputStream input)
         {
              this.input = input;
              this.output = new StringBuffer("");
              this.e = null;
         }
         
         public void run()
         {
              BufferedReader br = null;
              try
              {
                   br = new BufferedReader(new InputStreamReader(this.input));
                   while (br.ready()) {
                        this.output.append(br.readLine());
                   }
              }
              catch (Throwable e)
              {
                   this.e = new Exception(e);
              }
              finally
              {
                   try
                   {
                        if (br != null) {
                             br.close();
                        }
                   }
                   catch (IOException ioe)
                   {
                        this.e = new Exception(ioe);
                   }
              }
         }
         
         public String getOutput()
         {
              return this.output.toString();
         }
         
         public boolean hasException()
         {
              return this.e != null;
         }
         
         public Exception getException()
         {
              return this.e;
         }
    }
  • 5. Re: race condition with Runtime.exec()?
    jtahlborn Expert
    Currently Being Moderated
    gamblor01 wrote:
    Ok...here is an example below. Compile this and run it. Sometimes it will print only the return code, sometimes it will print both the return code and the uname result, and sometimes it will throw an exception (which is likely because the main thread has already destroyed the Process object before the gobbler thread(s) has/have had a chance to fully process the output stream).

    Uncomment the calls to join and it will work every time.
    yes, the join calls are necessary. in our own code-base, i have the join() calls after waitFor(), but either way is probably fine.
    Again though, I believe the JavaWorld example only succeeds because calls to println() are synchronized. If they had chosen a different example, such as appending to a StringBuffer as I do then they would need to call join, synchronize blocks, etc. I'm just looking for confirmation here. I'm fairly competent with threads but it's always good to reinforce what I know and/or learn more. Thanks!
    the JavaWorld example "succeeds" because it is just passing along the info, not accumulating it, and probably also because the output can be processed fast enough before the parent program finishes. in your situation, yes, you need join() both to wait for the output to be completely processed and to establish the necessary "happens before" relationship so that the main thread can read the entire results.
  • 6. Re: race condition with Runtime.exec()?
    852321 Newbie
    Currently Being Moderated
    Thank you all for the replies. I guess I really wasn't crazy...or at least this is not a sufficient condition to prove such claims. :)
  • 7. Re: race condition with Runtime.exec()?
    EJP Guru
    Currently Being Moderated
    I repeat. You may need to close the process's input stream so it will exit. If so, obviously you need to do that before calling Process.waitFor().
  • 8. Re: race condition with Runtime.exec()?
    852321 Newbie
    Currently Being Moderated
    Ok so I should just move everything that is in the finally's try block to happen before invoking p.waitFor()? When you say the input stream I'm actually not sure if you mean p.getInputStream().close() or p.getOutputStream().close() -- since getInputStream() is piped from STDOUT and and getOutputStream() is piped from STDIN of the process. So input stream from which perspective...that of Java or that of the process being spawned?

    If you can clarify that would be great. :)
  • 9. Re: race condition with Runtime.exec()?
    jtahlborn Expert
    Currently Being Moderated
    if you don't need to write to the forked process' input stream (p.getOutputStream()), then you should close it immediately after starting the forked process (right after "runtime.exec(cmdArray)"). everything else is fine where it is, i believe.
  • 10. Re: race condition with Runtime.exec()?
    EJP Guru
    Currently Being Moderated
    Ok so I should just move everything that is in the finally's try block to happen before invoking p.waitFor()?
    No, that's not what I sad. Read it again.
    When you say the input stream I'm actually not sure if you mean p.getInputStream().close()
    Yes.
  • 11. Re: race condition with Runtime.exec()?
    jtahlborn Expert
    Currently Being Moderated
    EJP wrote:
    When you say the input stream I'm actually not sure if you mean p.getInputStream().close()
    Yes.
    you sure you don't mean "p.getOutputStream()" which is the OutputStream which corresponds to the stdin of the forked process (as i said in my previous comment)?
  • 12. Re: race condition with Runtime.exec()?
    EJP Guru
    Currently Being Moderated
    Err, oops, yes, that's it.
  • 13. Re: race condition with Runtime.exec()?
    852321 Newbie
    Currently Being Moderated
    jtahlborn -- thank you very much for all of your input.


    As for you EJP -- I'm mixed.

    >
    No, that's not what I sad. Read it again.
    >

    Well what you said was to close the input stream BEFORE the waitFor() call. So I was simply asking if that means that I need to close all 3 streams or just the one. Your responses are all mostly terse, cryptic, and nonspecific so reading them again is often not very beneficial. What may seem obvious to you isn't going to be obvious to everyone. While I appreciate your help your tone is extremely condescending and unappreciated.

    For example, go back and read your initial comment and you will notice that it DID NOT specify that I should close the input stream BEFORE the call to waitFor(). It simply said:

    >
    You may need to close the process's input stream so it will exit.
    >


    I read your comment and assumed that my code was fine because I was closing the input stream...it gets closed inside of the finally block. So then you said to move it before the call to waitFor(). So my question was whether or not I needed to close all 3 steams before waitFor() and if not, then did you mean p.getInputStream() or p.getOutputStream()? You didn't directly answer either question directly, though you did answer the first part in a roundabout/snarky fashion.

    Perhaps if you took the time to elaborate and answer questions directly/respectfully, then you wouldn't need to go back and comment time and time again.
  • 14. Re: race condition with Runtime.exec()?
    EJP Guru
    Currently Being Moderated
    I'm sorry you're so confused between 'one' and 'three' but clearly nothing I could ever say would ever reduce your confusion. I'm also sorry you find 'one' so 'terse', 'cryptic', 'non-specific', 'indirect','disrespectful', etc etc etc, but again I doubt I can do any better.
1 2 Previous Next

Legend

  • Correct Answers - 10 points
  • Helpful Answers - 5 points