Re: RFR: 8303133: Update ProcessTools.startProcess(...) to exit early if process exit before linePredicate is printed.
On Fri, 24 Feb 2023 22:15:18 GMT, Leonid Mesnik 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. Thanks - PR Comment: https://git.openjdk.org/jdk/pull/12751#issuecomment-1444608371
Re: RFR: 8303133: Update ProcessTools.startProcess(...) to exit early if process exit before linePredicate is printed. [v5]
On Mon, 27 Feb 2023 23:11:27 GMT, Leonid Mesnik 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 two additional > commits since the last revision: > > - latch != updated > - message improved We are seeing large numbers of failures after this change was integrated, so it will likely need to be backed out. - PR: https://git.openjdk.org/jdk/pull/12751
Re: RFR: 8303133: Update ProcessTools.startProcess(...) to exit early if process exit before linePredicate is printed. [v5]
On Mon, 27 Feb 2023 23:11:27 GMT, Leonid Mesnik 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 two additional > commits since the last revision: > > - latch != updated > - message improved Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.org/jdk/pull/12751
Re: RFR: 8303133: Update ProcessTools.startProcess(...) to exit early if process exit before linePredicate is printed. [v5]
On Tue, 28 Feb 2023 02:42:33 GMT, David Holmes wrote: >> Leonid Mesnik has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - latch != updated >> - message improved > > This seems okay to me now. Thanks. @dholmes-ora, Thank you for your comments. - PR: https://git.openjdk.org/jdk/pull/12751
Re: RFR: 8303133: Update ProcessTools.startProcess(...) to exit early if process exit before linePredicate is printed. [v5]
On Mon, 27 Feb 2023 23:11:27 GMT, Leonid Mesnik 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 two additional > commits since the last revision: > > - latch != updated > - message improved This seems okay to me now. Thanks. - Marked as reviewed by dholmes (Reviewer). PR: https://git.openjdk.org/jdk/pull/12751
Re: RFR: 8303133: Update ProcessTools.startProcess(...) to exit early if process exit before linePredicate is printed. [v4]
On Mon, 27 Feb 2023 22:48:46 GMT, David Holmes wrote:
>> Leonid Mesnik has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> added some extra time to dead process.
>
> test/lib/jdk/test/lib/process/ProcessTools.java line 227:
>
>> 225: // Give some extra time for the StreamPumper to
>> run after the process completed
>> 226: Thread.sleep(1000);
>> 227: if (latch.getCount() > 0) {
>
> Nit: we use > 0 here but != 0 above.
let use > in all places
-
PR: https://git.openjdk.org/jdk/pull/12751
Re: RFR: 8303133: Update ProcessTools.startProcess(...) to exit early if process exit before linePredicate is printed. [v5]
> 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 two additional commits since the last revision: - latch != updated - message improved - Changes: - all: https://git.openjdk.org/jdk/pull/12751/files - new: https://git.openjdk.org/jdk/pull/12751/files/02770459..5ecb0047 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12751=04 - incr: https://webrevs.openjdk.org/?repo=jdk=12751=03-04 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/12751.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12751/head:pull/12751 PR: https://git.openjdk.org/jdk/pull/12751
Re: RFR: 8303133: Update ProcessTools.startProcess(...) to exit early if process exit before linePredicate is printed. [v4]
On Mon, 27 Feb 2023 22:12:22 GMT, Leonid Mesnik 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:
>
> added some extra time to dead process.
test/lib/jdk/test/lib/process/ProcessTools.java line 227:
> 225: // Give some extra time for the StreamPumper to
> run after the process completed
> 226: Thread.sleep(1000);
> 227: if (latch.getCount() > 0) {
Nit: we use > 0 here but != 0 above.
test/lib/jdk/test/lib/process/ProcessTools.java line 228:
> 226: Thread.sleep(1000);
> 227: if (latch.getCount() > 0) {
> 228: throw new RuntimeException("Started process
> " + name + " is not alive.");
Nit: the message is not very informative - we expect the process to die, the
problem is it died before giving the necessary output. Suggestion:
> "Started process " + name + "terminated before producing the expected output"
-
PR: https://git.openjdk.org/jdk/pull/12751
Re: RFR: 8303133: Update ProcessTools.startProcess(...) to exit early if process exit before linePredicate is printed. [v4]
> 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: added some extra time to dead process. - Changes: - all: https://git.openjdk.org/jdk/pull/12751/files - new: https://git.openjdk.org/jdk/pull/12751/files/5b7e3897..02770459 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12751=03 - incr: https://webrevs.openjdk.org/?repo=jdk=12751=02-03 Stats: 6 lines in 1 file changed: 4 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/12751.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12751/head:pull/12751 PR: https://git.openjdk.org/jdk/pull/12751
Re: RFR: 8303133: Update ProcessTools.startProcess(...) to exit early if process exit before linePredicate is printed. [v2]
On Mon, 27 Feb 2023 17:55:40 GMT, Leonid Mesnik wrote:
>> 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.
Yes there remains a race, if the process exits but the streamPumper has not yet
had the chance to call `countDown` then we would again throw the exception.
Perhaps all we can do is give a little extra time after decting a dead process?
if (!p.isAlive()) {
if (latch.getCount() > 0) {
// Give some extra time for the StreamPumper to run after the process
completed
Thread.sleep(1000);
if (latch.getCount() > 0) {
throw new RuntimeException("Started process " + name + " but it terminated
without the expected output");
}
}
-
PR: https://git.openjdk.org/jdk/pull/12751
Re: RFR: 8303133: Update ProcessTools.startProcess(...) to exit early if process exit before linePredicate is printed. [v2]
On Mon, 27 Feb 2023 04:54:57 GMT, David Holmes 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
Re: RFR: 8303133: Update ProcessTools.startProcess(...) to exit early if process exit before linePredicate is printed. [v3]
> 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: fixed conditions. - Changes: - all: https://git.openjdk.org/jdk/pull/12751/files - new: https://git.openjdk.org/jdk/pull/12751/files/890dcc34..5b7e3897 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12751=02 - incr: https://webrevs.openjdk.org/?repo=jdk=12751=01-02 Stats: 19 lines in 2 files changed: 4 ins; 12 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/12751.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12751/head:pull/12751 PR: https://git.openjdk.org/jdk/pull/12751
Re: RFR: 8303133: Update ProcessTools.startProcess(...) to exit early if process exit before linePredicate is printed. [v2]
On Sat, 25 Feb 2023 00:19:33 GMT, Leonid Mesnik 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
Re: RFR: 8303133: Update ProcessTools.startProcess(...) to exit early if process exit before linePredicate is printed. [v2]
> 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. - Changes: - all: https://git.openjdk.org/jdk/pull/12751/files - new: https://git.openjdk.org/jdk/pull/12751/files/71dad3cc..890dcc34 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12751=01 - incr: https://webrevs.openjdk.org/?repo=jdk=12751=00-01 Stats: 16 lines in 1 file changed: 5 ins; 1 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/12751.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12751/head:pull/12751 PR: https://git.openjdk.org/jdk/pull/12751
Re: RFR: 8303133: Update ProcessTools.startProcess(...) to exit early if process exit before linePredicate is printed. [v2]
On Fri, 24 Feb 2023 22:41:34 GMT, Roger Riggs 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 228:
>
>> 226: }
>> 227: } else {
>> 228: if (!latch.await(Utils.adjustTimeout(timeout),
>> unit)) {
>
> Checking for liveness in both timeout ==0 and > 0 would be useful.
fixed
> test/lib/jdk/test/lib/thread/ProcessThread.java line 168:
>
>> 166: output = new OutputAnalyzer(this.process);
>> 167: // Will block...
>> 168: this.process.waitFor();
>
> It would be useful to capture the exit status here for the log.
It is printed anyway in ProcessTools.getProcessLog(...)
-
PR: https://git.openjdk.org/jdk/pull/12751
Re: RFR: 8303133: Update ProcessTools.startProcess(...) to exit early if process exit before linePredicate is printed.
On Fri, 24 Feb 2023 22:15:18 GMT, Leonid Mesnik 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. test/lib/jdk/test/lib/thread/ProcessThread.java line 168: > 166: output = new OutputAnalyzer(this.process); > 167: // Will block... > 168: this.process.waitFor(); It would be useful to capture the exit status here for the log. - PR: https://git.openjdk.org/jdk/pull/12751
Re: RFR: 8303133: Update ProcessTools.startProcess(...) to exit early if process exit before linePredicate is printed.
On Fri, 24 Feb 2023 22:15:18 GMT, Leonid Mesnik 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.
test/lib/jdk/test/lib/process/ProcessTools.java line 224:
> 222: //Fail if process finished before printed
> expected string
> 223: latch.countDown();
> 224: throw new RuntimeException("Started process
> is not alive.");
Please add the name to the exception; it will be easier to determine what
failed.
test/lib/jdk/test/lib/process/ProcessTools.java line 228:
> 226: }
> 227: } else {
> 228: if (!latch.await(Utils.adjustTimeout(timeout),
> unit)) {
Checking for liveness in both timeout ==0 and > 0 would be useful.
-
PR: https://git.openjdk.org/jdk/pull/12751
