current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 6 down vote favorite
1

Usually, people would create two extra-threads in order to read the standard output and error respectively. However, the following code would allow to handle a Process outputs without those threads (it is arguably a little bit more difficult to decode the characters if you are using something else than a single-byte encoding and if you can't buffer the output in memory).

Although the following code seems to work, it is different enough from the "standard" solution, that I'm wondering if anyone can find am issue with it.

public static void main(String[] args) throws IOException, InterruptedException {
  ProcessBuilder processBuilder = new ProcessBuilder(args);
  Process process = processBuilder.start();

  InputStream outputStream = null, errorStream = null;
  ByteArrayOutputStream outputBuffer = new ByteArrayOutputStream();
  ByteArrayOutputStream errorBuffer = new ByteArrayOutputStream();
  try {
    outputStream = process.getInputStream();
    errorStream = process.getErrorStream();

    byte[] tmp = new byte[1024];

    while (true) {
      int outputBytes = readAvailablOnce(outputStream, outputBuffer, tmp);
      int errorBytes = readAvailablOnce(errorStream, errorBuffer, tmp);
      if (outputBytes == 0 && errorBytes == 0) {
        try {
          process.exitValue();
          break;
        } catch (IllegalThreadStateException e) {
          // keep on looping
        }
      }
    }
    readAvailableAll(outputStream, outputBuffer, tmp);
    readAvailableAll(errorStream, errorBuffer, tmp);

  } finally {
    closeQuietly(outputStream);
    closeQuietly(errorStream);
  }

  System.out.println(outputBuffer.toString("ASCII"));
  System.err.println(errorBuffer.toString("ASCII"));
  System.err.println("exit code: " + process.exitValue());
}

private static void closeQuietly(InputStream in) {
  if (in != null) {
    try {
      in.close();
    } catch (IOException e) {
      // ignored
    }
  }
}

private static int readAvailablOnce(
  InputStream inputStream, OutputStream outputStream, byte[] buffer)
throws IOException {
  int bytesRead = 0;
  if (inputStream.available() > 0) {
    bytesRead = inputStream.read(buffer);
    outputStream.write(buffer, 0, bytesRead);
  }
  return bytesRead;
}

private static void readAvailableAll(
  InputStream inputStream, OutputStream outputStream, byte[] buffer)
throws IOException {
  if (inputStream.available() > 0) {
    int bytesRead = 0;
    while ((bytesRead = inputStream.read(buffer)) >= 0) {
      outputStream.write(buffer, 0, bytesRead);
    }
  }
}
share|improve this question

3 Answers 3

up vote 3 down vote accepted

Is inputStream.available() a blocking method?

The process might send an error message while your are blocked (possibly forever) in the standard output available() waiting for something that will never arrive.

share|improve this answer
2  
Although it is not explicitly stated by the Javadoc InputStream.available() has to be non-blocking (and is, in practice) otherwise its purpose would be rather thin: "returns the number of bytes that can be read from this input stream without blocking". –  viphe Jan 16 '12 at 2:30
up vote 2 down vote

I've found nothing too, but it looks little bit weird. Anyway, some notes:

  1. You shouldn't define two variables at the same line:

    InputStream outputStream = null, errorStream = null;

    It's hard to read and find the declaration if you need that.

  2. Apache Commons IO also has closeQuietly too.

  3. I'd create a separate byte[] tmp array for every method (inside the readAvailablOnce and readAvailableAll methods). Passing the same buffer array to every method looks premature optimization (which is usually bad).

    private static int readAvailablOnce(final InputStream inputStream, 
        final OutputStream outputStream) throws IOException {
    final byte[] buffer = new byte[1024];
    ...
}
...

  4. Instead of the readAvailableAll you could use the IOUtils.copy. It does the same.

share|improve this answer
    
Thanks! 2. Agreed. I didn't use IOUtils for the clarity of my example (I use it in my actual code). 3. Reuse of a buffer to avoid unnecessary and repetitive (we are in a loop) memory allocations is not at all premature. Not making things more readable though. Also, it could be argued that smart JIT + GC could do as well (I wonder...). 4. Agreed (with same remark about buffer reuse). The test of isAvailable() is kinda extra here as we can assume that because the process ended, the outputs are readily available (i.e. the OS is not waiting to fetch more contents in either). –  viphe Jan 19 '12 at 7:20
up vote 2 down vote

I'm concerned that you have implemented a very tight loop which will execute very quickly and place very high load on the system until the executing process terminates. This could be a big problem if this is being used to wait for a long running process to complete. The JVM will attempt to repeat those instructions within the while(true) loop as fast as it can.

Have you run this code and observed any load issues. Try executing a simple batch script which just does a sleep 60. I've not executed this code, just what I'd expect from looking at it.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.