On Sat, 25 Feb 2023 00:19:33 GMT, Leonid Mesnik <[email protected]> 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
