Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams [v3]
On Mon, 24 May 2021 00:33:06 GMT, Roger Riggs 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. The updated javadoc addresses most of my points. The clarification to inputReader/errorReader about malformed input looks good but we will need the equivalent in outputWriter for the unmappable character case. I assume the "not null" can be dropped from the description of the charset parameter as NPE is now specified. - PR: https://git.openjdk.java.net/jdk/pull/4134
Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams [v3]
On Mon, 24 May 2021 01:21:09 GMT, Roger Riggs wrote: > On the question of process termination behavior, I'm not sure what can be > said that could be specification. > The implementations vary between Mac, Linux, and Windows; with the common > thread > to avoid losing process output. But this may have to be included in the > unspecified implementation behavior > or just an API note. Suggestions? I think the javadoc could set expectations that the behavior when the process has terminated, and streams have not been redirected, is unspecified. Reading from the process output/error streams may read some or no bytes/characters. - PR: https://git.openjdk.java.net/jdk/pull/4134
Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams [v3]
On Mon, 24 May 2021 18:55:33 GMT, Roger Riggs wrote:
>> src/java.base/share/classes/java/lang/Process.java line 771:
>>
>>> 769:
>>> 770: /**
>>> 771: * {@return Charset for the native encoding or {@link
>>> Charset#defaultCharset()}
>>
>> Need some edit here?
>
> Looks odd, but is the correct syntax for the new {@return javadoc tag}.
> See https://bugs.openjdk.java.net/browse/JDK-8075778
Hmm. Unless I'm missing something, you have mismatched curly braces.
-
PR: https://git.openjdk.java.net/jdk/pull/4134
Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams [v3]
On Thu, 20 May 2021 20:29:40 GMT, Naoto Sato wrote:
>> 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 771:
>
>> 769:
>> 770: /**
>> 771: * {@return Charset for the native encoding or {@link
>> Charset#defaultCharset()}
>
> Need some edit here?
Looks odd, but is the correct syntax for the new {@return javadoc tag}.
See https://bugs.openjdk.java.net/browse/JDK-8075778
-
PR: https://git.openjdk.java.net/jdk/pull/4134
Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams [v3]
On Mon, 24 May 2021 00:33:06 GMT, Roger Riggs 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 292:
> 290:
> 291: /**
> 292: * Returns a {@code BufferedWriter} connected to the normal input of
> the process the native encoding.
`the native encoding` here seems leftover.
-
PR: https://git.openjdk.java.net/jdk/pull/4134
Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams [v3]
On Mon, 24 May 2021 00:33:06 GMT, Roger Riggs 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: * 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: *
> 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
Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams [v3]
On Mon, 24 May 2021 00:33:06 GMT, Roger Riggs 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. Thanks for the comments, most are addressed. > The updated proposal looks reasonable, as does the names of the methods. > > I think the javadoc will need to specify how malformed or unmappable byte > sequence are handled. The implNote does document that it uses > InputStreamReader so this means that erroneous input is replaced but this > isn't normative text. > > It may be useful to specify how these methods are intended to work when the > process has terminated. I realise the existing get aren't clear on this > point and maybe they should. > > The API note that warns about unpredictable behavior then trying to use both > the byte and character stream probably needs to go into the existing methods > too. > > NPE will need to be documented as I don't think the Process class description > has a statement to avoid clutter in the method descriptions. You will > eventually need to add the @SInCE javadoc tag too. > > Is there more test coverage to come? I don't see tests that exercise the > redirect cases or NPE. The test updates includes an adjustment to how > SerialFilterTest is run - is that meant to be included? > > A formatting nit is that the line lengths are very really long compared to > the rest of the source file. On the question of process termination behavior, I'm not sure what can be said that could be specification. The implementations vary between Mac, Linux, and Windows; with the common thread to avoid losing process output. But this may have to be included in the unspecified implementation behavior or just an API note. Suggestions? I need to address what happens when these methods are called more than once. They should return the same instance as the previous call. - PR: https://git.openjdk.java.net/jdk/pull/4134
Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams [v3]
> 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. - Changes: - all: https://git.openjdk.java.net/jdk/pull/4134/files - new: https://git.openjdk.java.net/jdk/pull/4134/files/5a4a85d2..7724b82c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4134=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4134=01-02 Stats: 180 lines in 3 files changed: 136 ins; 0 del; 44 mod Patch: https://git.openjdk.java.net/jdk/pull/4134.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4134/head:pull/4134 PR: https://git.openjdk.java.net/jdk/pull/4134
