The correct way to handle Process streams

Question

Can anyone clarify me if the below procedure is correct way to handle streams of process without any stream buffer full and blocking problems

I'm invoking external program from java program, I'm using ProcessBuilder to build the process and after I perform

Process gpgProcess = processBuilder.start();

I'm handling the process using a method

String executionResult = verifyExecution(gpgProcess);

and in my method i'm trying to handle the process streams

private String verifyExecution(Process gpgProcess) throws IOException, InterruptedException {

        String gpgResult = null;

        BufferedReader stdOut = new BufferedReader(new InputStreamReader(gpgProcess.getInputStream()));
        BufferedReader stdErr = new BufferedReader(new InputStreamReader(gpgProcess.getErrorStream()));

        gpgProcess.waitFor();

        if(stdErr.ready()) {
            gpgResult = "Exit code: " + gpgProcess.exitValue() + "\n" + readStream(stdErr);
        } else if(stdOut.ready()) {
            gpgResult = "Exit code: " + gpgProcess.exitValue() + "\n" + readStream(stdOut);
        } else {
            gpgResult = "Exit code: " + gpgProcess.exitValue();
        }


        int exitCode = gpgProcess.exitValue();  
        this.setExitCode(exitCode);
        stdOut.close();
        stdErr.close();

        if(exitCode != 0) {
            throw new RuntimeException("Pgp Exception: " + gpgResult);
        }

        return gpgResult;
    }

The readStream method is used to read my stream text.

private String readStream(BufferedReader reader) throws IOException {

        StringBuilder result = new StringBuilder();

        try {
            while(reader.ready()) {
                result.append(reader.readLine());
                if(reader.ready()) {
                    result.append("\n");
                }
            }
        } catch(IOException ioe) {
            System.err.println("Error while reading the stream: " + ioe.getMessage());
            throw ioe;
        }

        return result.toString();
    }

Answer 1

There's no reliable way to tell whether an stream has data available without a call to read()—but that call will block if there are no data available. Methods like available() and ready() aren't reliable, because they can give false negatives; they can report that no data are available, even when there are.

A general-purpose facility that will work with any process requires a separate thread to consume each InputStream. This is because, in general, processes could interleave output to stdout and stderr, and unblocking one could cause the other to block, and so on. The process might write partial standard output, then block on a write to standard error. If your master process uses just one thread, it will hang, regardless which stream it reads first. Independent threads consuming both streams will make sure the process runs smoothly.

If you are running a specific process, and you can guarantee it has certain output in every case, you could take some shortcuts… keeping in mind that, "Short cuts make long delays."

Answer 2

No, that is not the correct way to do it.

First, on some systems, your code will be stuck on the gpgProcess.waitFor() call forever, because the process cannot finish until its standard out and standard error have been fully read and consumed.

Second, you are not using the ready() method of Reader correctly. The documentation states that the method returns true only if reading a character is guaranteed not to block. Returning false does not mean that the end of the stream has been reached; it just means the next read might block (meaning, it might not return immediately).

The only ways to know when you have reached the end of a Reader’s data stream are:

• check whether any of its read methods return a negative number
• check whether the readLine method of BufferedReader returns null

So your readStream method should look like this:

String line;
while ((line = reader.readLine()) != null) {
    result.append(line).append("\n");
}

As of Java 8, you can make it even shorter:

return reader.lines().collect(Collectors.joining("\n"));

Similarly, you should not be calling stdErr.ready() or stdOut.ready(). Either or both methods might or might not return true, even when there are no characters available; the only guarantee for the ready() method is that returning true means the next read will not block. It is possible for ready() to return true even at the end of the character stream, when the next read would immediately return -1, as long as that read does not block.

In summary, don't use ready() at all. Consume all of both streams, and check whether the error stream is empty:

String output = readStream(stdErr);
if (output.isEmpty()) {
    String output = readStream(stdOut);
}
gpgResult = "Exit code: " + gpgProcess.exitValue() + "\n" + output;

That would address the case your question appears to present: Either the Process produces standard error and no lines on standard output, or the other way around. However, this will not properly handle Processes in general.

For the general case, the easiest solution is to have the process merge its standard error with standard output using redirectErrorStream, so there is only one stream to consume:

processBuilder.redirectErrorStream(true);
Process gpgProcess = processBuilder.start();

The verifyExecution method could then contain:

String output;
try (BufferedReader stdOut = new BufferedReader(new InputStreamReader(gpgProcess.getInputStream()))) {
    output = readStream(stdOut);
}

if (output.isEmpty()) {
    gpgResult = "Exit code: " + gpgProcess.waitFor();
} else {
    gpgResult = "Exit code: " + gpgProcess.waitFor() + "\n" + output;
}

If you absolutely must have separate standard error and standard output, you need at least one background thread. I find an ExecutorService makes passing a value from a background thread easier:

ExecutorService background = Executors.newSingleThreadExecutor();
Future<String> stdOutReader = background.submit(() -> readStream(stdOut));

String output = readStream(stdErr);
if (output.isEmpty()) {
    output = stdOutReader.get();
}

background.shutdown();

if (output.isEmpty()) {
    gpgResult = "Exit code: " + gpgProcess.waitFor();
} else {
    gpgResult = "Exit code: " + gpgProcess.waitFor() + "\n" + output;
}

Finally, you should not catch and re-throw IOException just to print it out. Whatever code calls verifyExecution will have to catch IOException anyway; it is that code’s job to print, log, or otherwise handle the IOException. Intercepting it like that will probably result in its being printed twice.