RFR: CODETOOLS-7902337 jcov (JREInstr) hides results of running java commands: jlink, jimage

[Jonathan Gibbons]

Leonid,

Your use of Runtime.exec may cause problems.

In particular, you call Process.waitFor before reading any output from 
stdout, and not reading anything from stderr. This means that the 
process may block if there is a lot of output from the command which 
exceeds the available buffering.

To avoid blocking, you should follow these rules:

1. Read from stdout and stderr in a timely manner, i.e. before calling 
Process.waitFor

2. If you don't want to read from stdout and stderr separately, which 
requires multiple threads, you should start the process with 
ProcessBuilder, and set stderr to redirect to stdout using 
ProcessBuilder.redirectErrorStream(boolean)
https://download.java.net/java/early_access/jdk11/docs/api/java.base/java/lang/ProcessBuilder.html#redirectErrorStream(boolean)

3. Read the streams to completion before calling Process.waitFor

-- Jon


On 10/23/18 3:22 PM, Leonid Kuskov wrote:
> Thanks for the review, I've fixed  issues.
>
> Webrev: http://cr.openjdk.java.net/~lkuskov/7902337/webrev.01/
>
> Leonid
>
> On 10/23/18 13:58, Alexandre (Shura) Iline wrote:
>> 1. Did you mean to use Objects.requireNonNull(Object) where you use 
>> Objects.nonNull(Object)?
>> 2. I think it makes sense to close the resources in doCommand(String, 
>> File, String). Perhaps use try-with-resources.
>> 3. In getJavaVersion(), the way I read it
>> 3.1. VER9 will be returned for versions 16 or more, which is strange
>> 3.2. For version 11, the return value will be 210 = 100 + 11*10
>> 3.3. I would think just parsing the string with regex would give more 
>> transparent code
>>
>> Shura
>>
>>> On Oct 23, 2018, at 1:20 PM, Leonid Kuskov 
>>> <Leonid.Kuskov at Oracle.com> wrote:
>>>
>>> Please review and push the cosmetic fix for the
>>>
>>> Issue: https://bugs.openjdk.java.net/browse/CODETOOLS-7902337
>>> Webrev: http://cr.openjdk.java.net/~lkuskov/7902337/webrev.00/
>>>
>>> Thanks,
>>> Leonid
>>>
>