RFR: 8303697: ProcessTools doesn't print last line of process output [v3]

[David Holmes]

On Wed, 15 Mar 2023 22:49:57 GMT, Leonid Mesnik <lmesnik at openjdk.org> wrote:

>> The StreamPumper is fixed to process the last line even it is not finishes with '\n' or '\r'. The test included. Testing with tier1-3 also to verify that tests are not broken.
>
> Leonid Mesnik has updated the pull request incrementally with one additional commit since the last revision:
> 
>   comments added

A few minor suggestions.

The additional commentary is very helpful. Figuring out exactly how this code works is painful.

Thanks.

test/lib-test/jdk/test/lib/process/ProcessToolsLastLineTest.java line 50:

> 48: 
> 49:         // The line which exceeds internal StreamPumper buffer (256 bytes)
> 50:         String VERY_LONG_LINE = "VERYLONGLINE".repeat(30);

It might be a bit more obvious that this does exceed 256 by simply doing:

String veryLongLine = "X".repeat(257);

test/lib-test/jdk/test/lib/process/ProcessToolsLastLineTest.java line 54:

> 52:             System.out.print(args[0]);
> 53:         } else {
> 54:             test("ARG1");

Some boundary testing here would be:

test("\n");
test("\nARG1");
test("\nARG1\n");
test("ARG1/n");
test("ARG1");

test/lib/jdk/test/lib/process/StreamPumper.java line 99:

> 97:      * Implements Thread.run(). Continuously read from {@code in} and write to
> 98:      * {@code out} until {@code in} has reached end of stream.
> 99:      * Additionally this method also split data read from buffer into the lines and process each line using linePumps.

A few grammatical nits. I suggest:
> Additionally this method also splits the data read from the buffer into lines, and processes each line using linePumps.

test/lib/jdk/test/lib/process/StreamPumper.java line 138:

> 136:                     }
> 137:                     // The remaining after last '\n' (lastcrlf position) buffer data is written into lineBos.
> 138:                     // The end of this line from next buffer is concatenated to this data in the next iteration.

In trying to help my own understanding here I suggest:

// If no crlf was found, or there was additional data after the last crlf was found, then write the leftover data
// in lineBos. If there is more data to read it will be concatenated with the current data on the next iteration.
// If there is no more data, or no more crlf found, all the remaining data will be processed after the loop, as the
// final line.

test/lib/jdk/test/lib/process/StreamPumper.java line 149:

> 147:             }
> 148:             // Process data remaining after last '\n' in the last buffer.
> 149:             // It is already written in the lineBos buffer but not processed because '\n' hasn't been met.

Suggestion:

// If there was no terminating crlf the remaining data has been written to lineBos, but this final line of data
// now needs to be processed by the linePumper.

-------------

PR: https://git.openjdk.org/jdk/pull/13034