RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams [v3]

[Daniel Fuchs]

On Mon, 24 May 2021 00:33:06 GMT, Roger Riggs <rriggs at openjdk.org> wrote:

>> Methods are added to java.lang.Process to read and write characters and lines from and to a spawned Process.
>> The Charset used to encode and decode characters to bytes can be specified or use the
>> operating system native encoding as is available from the "native.encoding" system property.
>
> Roger Riggs has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Added throws for NPE, delegated zero-arg methods that use native.encoding to
>   the 1-arg method with a charset, added tests for Redirect cases where the streams are null-input or null-output streams.

src/java.base/share/classes/java/lang/Process.java line 178:

> 176:      *
> 177:      * <p>This method delegates to {@link #inputReader(Charset)} using the
> 178:      * {@link Charset} named by the {@systemProperty native.encoding}

I thought that `{@systemProperty ... }` was supposed to be used at the place where the system property is defined, and *not* at the place where it's being used? I might be mistaken, but if I'm not - then there would be several places to fix in this file.

src/java.base/share/classes/java/lang/Process.java line 226:

> 224:      *
> 225:      * @param charset the {@code Charset} used to decode bytes to characters, not null
> 226:      * @return a BufferedReader for the standard output of the process using the {@code charset}

`{@code BufferedReader}`

src/java.base/share/classes/java/lang/Process.java line 231:

> 229:      */
> 230:     public BufferedReader inputReader(Charset charset) {
> 231:         return new BufferedReader(new InputStreamReader(getInputStream(), charset));

I'd suggest calling Objects.requireNonNull(charset) first thing in this method instead of relying on the InputStreamReader constructor to throw the NPE.

src/java.base/share/classes/java/lang/Process.java line 283:

> 281:      *
> 282:      * @param charset the {@code Charset} used to decode bytes to characters, not null
> 283:      * @return a BufferedReader for the standard error of the process using the {@code charset}

`{@code BufferedReader}`

src/java.base/share/classes/java/lang/Process.java line 288:

> 286:      */
> 287:     public BufferedReader errorReader(Charset charset) {
> 288:         return new BufferedReader(new InputStreamReader(getErrorStream(), charset));

Same remark here: I'd suggest calling Objects.requireNonNull(charset) first thing in this method instead of relying on the InputStreamReader constructor to throw the NPE.

src/java.base/share/classes/java/lang/Process.java line 317:

> 315:      * {@linkplain BufferedWriter#flush flush} is called.
> 316:      *
> 317:      * @return a BufferedWriter to the standard input of the process using the charset

`{@code BufferedWriter}`

src/java.base/share/classes/java/lang/Process.java line 351:

> 349:      *
> 350:      * @param charset the {@code Charset} to encode characters to bytes, not null
> 351:      * @return a BufferedWriter to the standard input of the process using the {@code charset}

`{@code BufferedWriter}`

src/java.base/share/classes/java/lang/Process.java line 356:

> 354:      */
> 355:     public BufferedWriter outputWriter(Charset charset) {
> 356:         return new BufferedWriter(new OutputStreamWriter(getOutputStream(), charset));

I'd suggest calling Objects.requireNonNull(charset) first thing in this method instead of relying on the InputStreamWriter constructor to throw the NPE.

src/java.base/share/classes/java/lang/Process.java line 459:

> 457:      * <p>
> 458:      * Invoking this method on {@code Process} objects returned by
> 459:      * {@link ProcessBuilder#start()} and {@link Runtime#exec} forcibly terminate

Should the link to `Runtime#exec` be fixed in the same manner, here and elsewhere in this file?

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

PR: https://git.openjdk.java.net/jdk/pull/4134