5 Replies Latest reply: Aug 17, 2012 8:47 AM by 870997 RSS

    What is the best practice dealing with process.getErrorStream()

    870997
      I've been playing around creating Process objects with ProcessBuilder. I can use getErrorStream() and getOutputStream() to read the output from the process, but it seems I have to do this on another thread. If I simply call process.waitFor() and then try to read the streams that doesn't work. So I do something like
      final InputStream errorStream = process.getErrorStream();
      
      final StringWriter errWriter = new StringWriter();
      
      ExecutorService executorService = Executors.newCachedThreadPool();
      
      executorService.execute(
          new Runnable() {
              public void run() {
                  try {
                      IOUtils.copy(errorStream, errWriter, "UTF-8");
               } catch (IOException e) {
                      getLog().error(e.getMessage(), e);
               }
              }
          }
      );
      
      int exitValue = process.waitFor();
      getLog().info("exitValue = " + exitValue);
      getLog().info("errString =\n" + errWriter); 
      This works, but it seems rather inelegant somehow.

      The basic problem is that the Runnable never completes on its own. Through experimentation, I believe that when the process is actually done, errorStream is never closed, or never gets an end-of-file. My current code works because when it goes to read errWriter it just reads what is currently in the buffer. However, if I wanted to clean things up and use executorService.submit() to submit a Callable and get back a Future, then a lot more code is needed because "IOUtils.copy(errorStream, errWriter, "UTF-8");" never terminates.

      Am I misunderstanding something, or is process.getErrorStream() just a crappy API?

      What do other people do when they want to get the error and output results from running a process?

      Edited by: Eric Kolotyluk on Aug 16, 2012 5:26 PM
        • 1. Re: What is the best practice dealing with process.getErrorStream()
          EJP
          Define 'best practice'.

          There are two basic practices. Which is best in your situation depends entirely on your situation.

          1. Merge them; read the stdout stream either in the current thread or a separate thread, until EOS; then call waitFor(). This is useful in cases when you don't need to keep them separate.

          2. Don't merge them; read them on separate threads; and call waitFor() in the original thread. This is useful when you need to keep them separate, e.g. when you know that anything on stderr indicates a real error.

          Obviously there are further variations on the threading part. The key decision is whether or not to merge. You must ensure that the stream(s) have been/are concurrently being read to EOS before you call waitFor().
          • 2. Re: What is the best practice dealing with process.getErrorStream()
            sabre150
            EJP wrote:
            You must ensure that the stream(s) have been/are concurrently being read to EOS before you call waitFor().
            Not really necessary. If you read the stdin and stdout in threads separate from the thread creating the Process then calling waitFor() will block until the process terminates. The Javadoc for waitFor() says " causes the current thread to wait, if necessary, until the process represented by this Process object has terminated" .

            This does not mean that all the data on the stdout and stderr streams will have been read once waitFor() returns since there is some buffering. If one uses Daemon threads then to be safe it may be necessary to join() each of the two threads with the main thread before continuing since a simple non-forced program exit will abruptly terminate these Daemon threads.
            • 3. Re: What is the best practice dealing with process.getErrorStream()
              EJP
              You must ensure that the stream(s) have been/are concurrently being read to EOS before you call waitFor().
              Not really necessary. If you read the stdin and stdout in threads separate from the thread creating the Process then calling waitFor() will block until the process terminates.
              That's why I said "have been/are being concurrently" read.

              You do have to read to EOS, otherwise the process may be unable to exit due to stdio buffering. And if you close those streams prematurely you may cause broken pipe signals in the process, causing a failure condition.
              This does not mean that all the data on the stdout and stderr streams will have been read once waitFor() returns since there is some buffering.
              It does mean exactly that. The process can't exit until it clears its stdio buffers, and that can't happen unless your Java code is reading it all.
              • 4. Re: What is the best practice dealing with process.getErrorStream()
                sabre150
                I suspect we are talking a cross purposes.

                In my inept way I was not clear enough about the difference between the process being invoked and the Java Process object. The InputStream wraping the process stdout is buffered and can still hold unprocessed data after the process has terminated.
                • 5. Re: What is the best practice dealing with process.getErrorStream()
                  870997
                  OK, I found a better solution.
                  Future<String> errString = executorService.submit(
                      new Callable<String>() {
                          public String call() throws Exception {
                              StringWriter errWriter = new StringWriter();
                              IOUtil.copy(process.getErrorStream(), errWriter, "UTF-8");
                              return errWriter.toString();
                          }
                      }
                  );
                  
                  int exitValue = process.waitFor();
                               
                  getLog().info("exitValue = " + exitValue);
                               
                  try {
                      getLog().info("errString =\n" + errString.get());
                  } catch (ExecutionException e) {
                      throw new MojoExecutionException("proxygen: ExecutionException");
                  } 
                  The problem I was having before seemed to be that the call to Apache's IOUtil.copy(errorStream, errWriter, "UTF-8"); was not working right, it did not seem to be terminating on EOS. But now it seems to be working fine, so I must have been chasing some other problem (or non-problem).

                  So, it does seem the best thing to do is read the error and output streams from the process on their own daemon threads, and then call process.waitFor(). The ExecutorService API makes this easy, and using a Callable to return a future value does the right thing. Also, Callable is a little nicer as the call method can throw an Exception, so my code does not need to worry about that (and the readability is better).

                  Thanks for helping to clarify my thoughts and finding a good solution :-)

                  Now, it would be really nice if the Process API had a method like process.getFutureErrorString() which does what my code does.

                  Cheers, Eric