DefaultExecutor.executeSystemCommand() has deadlock potential

[GoogleCodeExporter]

DefaultExecutor.executeSystemCommand() sequentially reads all input from 
process.getInputStream() and then process.getErrorStream().

As the JavaDoc for java.lang.Process indicates (but does not explain in 
adequate detail, IMHO), this can lead to deadlock on systems with limited 
buffer sizes, which is every OS I've run Java on.

For example, if the process you run does the following:

void main() {
    char buf[1024];
    for (;;) {
        fwrite(buf, 1, 1024, stdout);
        fflush(stdout);
        fwrite(buf, 1, 1024, stderr);
        fflush(stderr);
    }
}

At some point, the pipe buffer for stderr in the OS will fill and the program 
will block.

DefaultExecutor will hang waiting forever for a read from 
process.getInputStream(). In order for it to make progress, it has to read from 
process.getErrorStream() concurrently.

Also, not using process.waitFor() means that zombies may accumulate. Finally, 
not returning process.exitValue() along with the String output limits 
usefulness.

I will work on a patch for these issues, as I've written this sort of code more 
than once. ;-)

Original issue reported on code.google.com by patrick....@gmail.com on 25 Aug 2010 at 9:35

[GoogleCodeExporter]

Patch attached.

Original comment by patrick....@gmail.com on 26 Aug 2010 at 12:28

Attachments:

• 0001-Make-Executor-return-ExecuteResult-and-fix-deadlock-.patch

[GoogleCodeExporter]

The patch was against the head of the 1.4 branch (r1465).

Original comment by patrick....@gmail.com on 26 Aug 2010 at 12:31

[GoogleCodeExporter]

Original comment by patrick....@gmail.com on 15 Sep 2010 at 12:17

• Changed state: Started
• Added labels: Usability

[GoogleCodeExporter]

The patch has been applied to the 1.4 branch. I will work on getting it into 
trunk next.

Original comment by patrick....@gmail.com on 15 Sep 2010 at 12:17

[GoogleCodeExporter]

A reworked patch has now been applied to trunk.

Original comment by patrick....@gmail.com on 15 Sep 2010 at 11:45

• Changed state: Fixed