On Thu, 27 Apr 2023 01:06:23 GMT, Leonid Mesnik <lmes...@openjdk.org> 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
