On Mon, 27 Feb 2023 04:54:57 GMT, David Holmes <dhol...@openjdk.org> wrote:
>> Leonid Mesnik has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> updated to always check if process is alive.
>
> 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`.
Using await() is more effective than checking every second. However, might be
it is not essential for multi-process synchronization.
> 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. ???
Thanks! You are right, we need to check first if the line has been printed.
However, there is an interesting question the process can exit before streams
are read and the latch is set to zero. The documentation says that
linePredicate should detect if the process is warmed-up. So it is not expected
that it should exit right after the start.
> 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.
Thanks! Fixed, it is a mistake.
-------------
PR: https://git.openjdk.org/jdk/pull/12751
