Re: RFR: 8306946: jdk/test/lib/process/ProcessToolsStartProcessTest.java fails with "wrong number of lines in OutputAnalyzer output" [v4]
> The ProcessTools.startProcess (...) has been updated to completely read > streams after process has been completed. > The test was updated to run 5 times with different number of lines and line > sizes. Leonid Mesnik has updated the pull request incrementally with one additional commit since the last revision: catching Cancellation exception - Changes: - all: https://git.openjdk.org/jdk/pull/13683/files - new: https://git.openjdk.org/jdk/pull/13683/files/d02b889a..8f350c8f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13683=03 - incr: https://webrevs.openjdk.org/?repo=jdk=13683=02-03 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13683.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13683/head:pull/13683 PR: https://git.openjdk.org/jdk/pull/13683
Re: RFR: 8306946: jdk/test/lib/process/ProcessToolsStartProcessTest.java fails with "wrong number of lines in OutputAnalyzer output" [v3]
On Thu, 27 Apr 2023 16:35:59 GMT, Leonid Mesnik wrote:
>> The ProcessTools.startProcess (...) has been updated to completely read
>> streams after process has been completed.
>> The test was updated to run 5 times with different number of lines and line
>> sizes.
>
> Leonid Mesnik has updated the pull request incrementally with one additional
> commit since the last revision:
>
> move buffers registration before pumping start point
Okay I _think_ I understand the changes now. I have one small suggestion below
but otherwise this seems okay.
Thanks.
test/lib/jdk/test/lib/process/ProcessTools.java line 190:
> 188: } catch (TimeoutException e) {
> 189: // continue execution, so wait() give a chance
> to write
> 190: } catch (InterruptedException | ExecutionException
> e) {
Probably also need to catch `CancellationException` here for good measure.
-
Marked as reviewed by dholmes (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/13683#pullrequestreview-1411942808
PR Review Comment: https://git.openjdk.org/jdk/pull/13683#discussion_r1184323176
Re: RFR: 8306946: jdk/test/lib/process/ProcessToolsStartProcessTest.java fails with "wrong number of lines in OutputAnalyzer output" [v3]
On Thu, 27 Apr 2023 16:35:59 GMT, Leonid Mesnik wrote: >> The ProcessTools.startProcess (...) has been updated to completely read >> streams after process has been completed. >> The test was updated to run 5 times with different number of lines and line >> sizes. > > Leonid Mesnik has updated the pull request incrementally with one additional > commit since the last revision: > > move buffers registration before pumping start point The testing: - tier1 - tier5 to verify that nothing is brokens, - run jdk/test/lib/process/ProcessToolsStartProcessTest.java with 50 iterations instead of 5 on each platform 100 times - manually added some thread.sleep into startProcess and verify that not failures still appear - PR Comment: https://git.openjdk.org/jdk/pull/13683#issuecomment-1526061664
Re: RFR: 8306946: jdk/test/lib/process/ProcessToolsStartProcessTest.java fails with "wrong number of lines in OutputAnalyzer output" [v3]
On Thu, 27 Apr 2023 16:35:59 GMT, Leonid Mesnik wrote: >> The ProcessTools.startProcess (...) has been updated to completely read >> streams after process has been completed. >> The test was updated to run 5 times with different number of lines and line >> sizes. > > Leonid Mesnik has updated the pull request incrementally with one additional > commit since the last revision: > > move buffers registration before pumping start point What Mach5 testing has been done with this fix? - PR Comment: https://git.openjdk.org/jdk/pull/13683#issuecomment-1526034233
Re: RFR: 8306946: jdk/test/lib/process/ProcessToolsStartProcessTest.java fails with "wrong number of lines in OutputAnalyzer output" [v3]
> The ProcessTools.startProcess (...) has been updated to completely read > streams after process has been completed. > The test was updated to run 5 times with different number of lines and line > sizes. Leonid Mesnik has updated the pull request incrementally with one additional commit since the last revision: move buffers registration before pumping start point - Changes: - all: https://git.openjdk.org/jdk/pull/13683/files - new: https://git.openjdk.org/jdk/pull/13683/files/10f1c5c2..d02b889a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13683=02 - incr: https://webrevs.openjdk.org/?repo=jdk=13683=01-02 Stats: 28 lines in 1 file changed: 10 ins; 9 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/13683.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13683/head:pull/13683 PR: https://git.openjdk.org/jdk/pull/13683
Re: RFR: 8306946: jdk/test/lib/process/ProcessToolsStartProcessTest.java fails with "wrong number of lines in OutputAnalyzer output" [v2]
On Thu, 27 Apr 2023 04:59:00 GMT, David Holmes wrote:
>> Leonid Mesnik has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fix
>
> test/lib-test/jdk/test/lib/process/ProcessToolsStartProcessTest.java line 79:
>
>> 77: System.out.print("FAILED: wrong number of lines in
>> Consumer output\n");
>> 78: success = false;
>> 79: System.out.print(out.getStdout());
>
> Why isn't this printing output?
fixed.
> test/lib-test/jdk/test/lib/process/ProcessToolsStartProcessTest.java line 95:
>
>> 93: public static void main(String[] args) throws Exception {
>> 94: if (args.length > 0) {
>> 95: for (int i = 0; i < Integer.parseInt(args[0]); i++) {
>
> This will call parseInt on each iteration of the loop.
fixed.
> test/lib/jdk/test/lib/process/ProcessTools.java line 183:
>
>> 181: // It is needed to wait until stream is flushed
>> after
>> 182: // process is completed.
>> 183: task.get();
>
> This looks problematic - if you block here you are holding the object monitor
> as this is a synchronized method.
You are right. It is needed to give a chance to write to this buffer. fixed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13683#discussion_r1178707275
PR Review Comment: https://git.openjdk.org/jdk/pull/13683#discussion_r1178707373
PR Review Comment: https://git.openjdk.org/jdk/pull/13683#discussion_r1178708269
Re: RFR: 8306946: jdk/test/lib/process/ProcessToolsStartProcessTest.java fails with "wrong number of lines in OutputAnalyzer output" [v2]
> The ProcessTools.startProcess (...) has been updated to completely read > streams after process has been completed. > The test was updated to run 5 times with different number of lines and line > sizes. Leonid Mesnik has updated the pull request incrementally with one additional commit since the last revision: fix - Changes: - all: https://git.openjdk.org/jdk/pull/13683/files - new: https://git.openjdk.org/jdk/pull/13683/files/c16d33f8..10f1c5c2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13683=01 - incr: https://webrevs.openjdk.org/?repo=jdk=13683=00-01 Stats: 9 lines in 2 files changed: 4 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/13683.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13683/head:pull/13683 PR: https://git.openjdk.org/jdk/pull/13683
Re: RFR: 8306946: jdk/test/lib/process/ProcessToolsStartProcessTest.java fails with "wrong number of lines in OutputAnalyzer output"
On Thu, 27 Apr 2023 01:06:23 GMT, Leonid Mesnik wrote:
> The ProcessTools.startProcess (...) has been updated to completely read
> streams after process has been completed.
> The test was updated to run 5 times with different number of lines and line
> sizes.
I missed the previous changes to ProcessTools as I was away. This code is so
complicated that I find it impossible to discern if it is actually doing the
right thing. But I'm concerned by the added FutureTask usage.
test/lib-test/jdk/test/lib/process/ProcessToolsStartProcessTest.java line 79:
> 77: System.out.print("FAILED: wrong number of lines in
> Consumer output\n");
> 78: success = false;
> 79: System.out.print(out.getStdout());
Why isn't this printing output?
test/lib-test/jdk/test/lib/process/ProcessToolsStartProcessTest.java line 95:
> 93: public static void main(String[] args) throws Exception {
> 94: if (args.length > 0) {
> 95: for (int i = 0; i < Integer.parseInt(args[0]); i++) {
This will call parseInt on each iteration of the loop.
test/lib/jdk/test/lib/process/ProcessTools.java line 183:
> 181: // It is needed to wait until stream is flushed
> after
> 182: // process is completed.
> 183: task.get();
This looks problematic - if you block here you are holding the object monitor
as this is a synchronized method.
-
PR Review: https://git.openjdk.org/jdk/pull/13683#pullrequestreview-1403178899
PR Review Comment: https://git.openjdk.org/jdk/pull/13683#discussion_r1178625211
PR Review Comment: https://git.openjdk.org/jdk/pull/13683#discussion_r1178627264
PR Review Comment: https://git.openjdk.org/jdk/pull/13683#discussion_r1178629324
RFR: 8306946: jdk/test/lib/process/ProcessToolsStartProcessTest.java fails with "wrong number of lines in OutputAnalyzer output"
The ProcessTools.startProcess (...) has been updated to completely read streams after process has been completed. The test was updated to run 5 times with different number of lines and line sizes. - Commit messages: - fixed num of iters - fix Changes: https://git.openjdk.org/jdk/pull/13683/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13683=00 Issue: https://bugs.openjdk.org/browse/JDK-8306946 Stats: 63 lines in 2 files changed: 26 ins; 21 del; 16 mod Patch: https://git.openjdk.org/jdk/pull/13683.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13683/head:pull/13683 PR: https://git.openjdk.org/jdk/pull/13683
