Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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
add comment

3 Answers

up vote 2 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
add comment

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
add comment

Looked through it, found nothing.

share|improve this answer
    
Thanks for looking through it. I'll give points, when I'll have the right to. ;-) –  viphe Jan 11 '12 at 9:06
    
@viphe You can give points simply by clicking the up-arrow to the left of the answer (up-vote the answer). Also as the one asking the question, you can select one answer as THE right answer, which gives even more points. –  toto2 Jan 15 '12 at 16:46
    
I tried but I need a minimum reputation of 15 to up a response. As for THE right answer, if someone finds a problem, I will know. Otherwise, I might not choose one... –  viphe Jan 16 '12 at 2:35
add comment

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.