On Thu, 20 Apr 2023 22:54:49 GMT, David Holmes <dhol...@openjdk.org> wrote:
>> test/lib/jdk/test/lib/process/ProcessTools.java line 750:
>>
>>> 748: public InputStream getInputStream() {
>>> 749: try {
>>> 750: waitFor();
>>
>> With this added `waitFor()` the assumption now is that the caller doesn't
>> intent to do incremental reads of the output as the process generates it.
>> For example, if the test were to send some command to the process and then
>> want to read the resulting output, and do this repeatedly, it won't able to
>> use the InputStream to do that.
>
> I have to agree with Chris. You are changing a fundamental property of this
> API. We no longer just start the process, we are forced to wait for it to
> complete.
Exactly. I added note about implementation in the javadoc to make it clear. I
don't see any good solution for this problem.
The only other possible solution which I see is to throw Exception here,
forcing user to use lineConsumer.
However the any usage of OutputAnalyzer with startProcess() would clearly and
quickly fails.
Also, the
public static Process startProcess(String name,
ProcessBuilder processBuilder)
could be modified to allow to read process streams. The test should drain tests
by itself in such case to avoid hang.
However, it don't see any good way to implement this method correctly if
already read the process streams.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13560#discussion_r1173161520
