RFR: 8303133: Update ProcessTools.startProcess(...) to exit early if process exit before linePredicate is printed. [v2]

[David Holmes]

On Sat, 25 Feb 2023 00:19:33 GMT, Leonid Mesnik <lmesnik at openjdk.org> wrote:

>> The solution proposed by Stefan K
>> 
>> The startProcess() might wait forever for the expected line if the process exits (failed to start). It makes sense to just fail earlier in such cases.
>> 
>> The fix also move
>> 'output = new OutputAnalyzer(this.process);'
>> in method xrun() to be able to try to print them in waitFor is failed/interrupted.
>
> Leonid Mesnik has updated the pull request incrementally with one additional commit since the last revision:
> 
>   updated to always check if process is alive.

I have a number of issues with the changes. I think you have basically addressed the problem of waiting forever if no predicate line was forthcoming, but I think you have introduced a race with normal termination of the process.

test/lib/jdk/test/lib/process/ProcessTools.java line 220:

> 218:             if (timeout > -1) {
> 219:                 // Every second check if process is still alive
> 220:                 boolean succeeded = Utils.waitForCondition(() -> {

This use of `waitForCondition` with a sleep time of zero confused me quite a bit. Now I realize that you are putting `waitForCondition` into a potential busy-poll loop, but then you introduce the one-second timed `await` as part of the condition, thus effectively checking the condition once a second. This seems somewhat convoluted compared to just using a sleep time of 1000ms and changing the `await` to a call to `getCount() > 0`.

test/lib/jdk/test/lib/process/ProcessTools.java line 223:

> 221:                     //Fail if process finished before printed expected string
> 222:                     if (!p.isAlive()) {
> 223:                         latch.countDown();

This `countdown` serves no purpose. The latch is used to coordinate the current thread and the stream pumper thread: the current thread calls `await` and the streamPumper calls `countDown`. Here the current thread is not going to call `await` and it doesn't need to `countDown` to release itself.

test/lib/jdk/test/lib/process/ProcessTools.java line 224:

> 222:                     if (!p.isAlive()) {
> 223:                         latch.countDown();
> 224:                         throw new RuntimeException("Started process " + name + " is not alive.");

This seems problematic. The process has terminated but you don't know why - it may have completed normally and produced all the output such that the `await` below would return immediately with `true`, but you are now going to throw an exception. ???

test/lib/jdk/test/lib/thread/ProcessThread.java line 153:

> 151:         @Override
> 152:         public void xrun() throws Throwable {
> 153:             String name = Thread.currentThread().getName();

The original code passes the name of the `ProcessRunnable` to `startProcess`, not the name of the current thread. It is not obvious/apparent that they are the same. But if they are then it is cheaper to use `name` than do a dynamic query on the current thread.

-------------

Changes requested by dholmes (Reviewer).

PR: https://git.openjdk.org/jdk/pull/12751