Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams [v7]
On Wed, 2 Jun 2021 15:00:56 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: > > Editorial improvements in outputWriter and inputReader. Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4134
Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams [v7]
On Wed, 2 Jun 2021 15:00:56 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:
>
> Editorial improvements in outputWriter and inputReader.
We've gone through a few iterations on the javadoc and I think the latest
edition is okay. I don't have time right now for the latest version of the test
(I did look at the test in the initial patch).
src/java.base/share/classes/java/lang/Process.java line 231:
> 229: *
> 230: * The first call to this method creates the {@link
> BufferedReader BufferedReader},
> 231: * if called again with the same {@code charset} the same {@code
> BufferedReader} is returned.
"the same BufferedReader is returned" - a suggestion here to rephrase this to
"then the BufferedReader returned by the first call is returned".
-
Marked as reviewed by alanb (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/4134
Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams [v7]
> 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: Editorial improvements in outputWriter and inputReader. - Changes: - all: https://git.openjdk.java.net/jdk/pull/4134/files - new: https://git.openjdk.java.net/jdk/pull/4134/files/bf5fc724..45838b13 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4134=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4134=05-06 Stats: 9 lines in 1 file changed: 4 ins; 4 del; 1 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
Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams [v6]
> 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: Corrected change from IllegalArgumentException to IllegalStateException and tests. - Changes: - all: https://git.openjdk.java.net/jdk/pull/4134/files - new: https://git.openjdk.java.net/jdk/pull/4134/files/37bfa009..bf5fc724 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4134=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4134=04-05 Stats: 8 lines in 2 files changed: 0 ins; 0 del; 8 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
Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams [v5]
> 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: Clarified use of native.encoding property and api notes about useing both writers and streams - Changes: - all: https://git.openjdk.java.net/jdk/pull/4134/files - new: https://git.openjdk.java.net/jdk/pull/4134/files/a5ed7b73..37bfa009 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4134=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4134=03-04 Stats: 74 lines in 1 file changed: 12 ins; 28 del; 34 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
Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams [v4]
On Tue, 25 May 2021 16:53:28 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:
>
> Described charset mapping of malformed chars in outputWriter
> Repeated calls to inputReader, errorReader, and outputWriter now return
> the same instance
> and check for inconsistent charset argument
> Added warnings about concurrently use of input/output streams and
> readers/writers.
src/java.base/share/classes/java/lang/Process.java line 105:
> 103: // Readers and Writers created for this process; so repeated calls
> return the same object
> 104: // All updates must be done while synchronized on this Process.
> 105: private BufferedWriter outputWriter = null;
No need to explicitly initialise all these fields to null.
src/java.base/share/classes/java/lang/Process.java line 131:
> 129: * Use {@link #getOutputStream} and {@link #outputWriter} with
> extreme care.
> 130: * Output to the {@code BufferedWriter} may be held in the buffer
> until
> 131: * {@linkplain BufferedWriter#flush flush} is called.
I think this will need a bit of wordsmithing to make it clear that its the
usage of both streams for the same Process requires great care (as it stands,
it reads like the usage of either method is dangerous).
src/java.base/share/classes/java/lang/Process.java line 207:
> 205: * {@link Charset} named by the {@code native.encoding}
> 206: * system property or the {@link Charset#defaultCharset()} if the
> 207: * {@code native.encoding} is not supported.
"if the native.encoding is not supported". I think this needs an adjustment to
make it clear that the value of the property is not a valid charset.
src/java.base/share/classes/java/lang/Process.java line 425:
> 423: } else {
> 424: if (!outputCharset.equals(charset))
> 425: throw new IllegalArgumentException("BufferedWriter
> was created with charset: " + outputCharset);
I'm not sure that IAE is the right exception here, I think it's closer to
IllegalStateException because the first usage of outputWriter has the side
effect of setting the charset for the Process's writer. Another option here is
to just put it into the "unpredictable" bucket that is using getOutputStream
and outputWriter at the same time. In that case, it could just return a new
BufferedWriter when it doesn't match the charset of the first usage.
-
PR: https://git.openjdk.java.net/jdk/pull/4134
Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams [v4]
On Thu, 27 May 2021 06:53:18 GMT, Alan Bateman wrote: > > Process is abstract. Is there any use for these new methods to be > > overridden? > > Perhaps they should be final. > > It's not clear to me that it is useful to extend Process outside of the JDK. > Testing, wrapping, ...? It feels like this class wants to be sealed. Maybe we > should look at deprecating the no-arg constructor, like Joe did recently with > AccessibleObject. There are subclasses, even in the test suite, see ProcessTools, so its too late to seal it; It would be a compatibility issue. We can look at deprecation and strength reduction but in the current time frame (RPD1) I'd suggest making the new methods final. - PR: https://git.openjdk.java.net/jdk/pull/4134
Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams [v4]
On Wed, 26 May 2021 17:58:06 GMT, Roger Riggs wrote: > Process is abstract. Is there any use for these new methods to be overridden? > Perhaps they should be final. It's not clear to me that it is useful to extend Process outside of the JDK. Testing, wrapping, ...? It feels like this class wants to be sealed. Maybe we should look at deprecating the no-arg constructor, like Joe did recently with AccessibleObject. - PR: https://git.openjdk.java.net/jdk/pull/4134
Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams [v4]
On Tue, 25 May 2021 16:53:28 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:
>
> Described charset mapping of malformed chars in outputWriter
> Repeated calls to inputReader, errorReader, and outputWriter now return
> the same instance
> and check for inconsistent charset argument
> Added warnings about concurrently use of input/output streams and
> readers/writers.
src/java.base/share/classes/java/lang/Process.java line 254:
> 252: * {@code
> 253: * return new BufferedReader(new
> InputStreamReader(getInputStream(), charset));
> 254: * }
Does not seem to be equivalent any longer. Also, should it describe the
behavior that it rejects the different Charset after the first invocation?
src/java.base/share/classes/java/lang/Process.java line 326:
> 324: * {@code
> 325: * return new BufferedReader(new
> InputStreamReader(getErrorStream(), charset));
> 326: * }
Same comment in `inputReader(Charset)` stands here too.
-
PR: https://git.openjdk.java.net/jdk/pull/4134
Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams [v4]
On Tue, 25 May 2021 16:53:28 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: > > Described charset mapping of malformed chars in outputWriter > Repeated calls to inputReader, errorReader, and outputWriter now return > the same instance > and check for inconsistent charset argument > Added warnings about concurrently use of input/output streams and > readers/writers. Process is abstract. Is there any use for these new methods to be overridden? Perhaps they should be final. The suggestion in the CSR was to document them using @ implSpec, to acknowledge that the subclass can do something different. - PR: https://git.openjdk.java.net/jdk/pull/4134
Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams [v4]
> 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: Described charset mapping of malformed chars in outputWriter Repeated calls to inputReader, errorReader, and outputWriter now return the same instance and check for inconsistent charset argument Added warnings about concurrently use of input/output streams and readers/writers. - Changes: - all: https://git.openjdk.java.net/jdk/pull/4134/files - new: https://git.openjdk.java.net/jdk/pull/4134/files/7724b82c..a5ed7b73 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4134=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4134=02-03 Stats: 161 lines in 2 files changed: 131 ins; 0 del; 30 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
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
Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams [v2]
On Sat, 22 May 2021 01:24:24 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: > > Use BufferedWriter and OutputStreamWriter and address review comments 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. - PR: https://git.openjdk.java.net/jdk/pull/4134
Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams [v2]
> 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: Use BufferedWriter and OutputStreamWriter and address review comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/4134/files - new: https://git.openjdk.java.net/jdk/pull/4134/files/a49b0b72..5a4a85d2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4134=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4134=00-01 Stats: 66 lines in 3 files changed: 19 ins; 14 del; 33 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
Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams
Hi, Yes, I'm testing BufferedWriter-> OutputStreamWriter now. And it can be wrapped in PrintWriter if someone is eager to get the missing text formatting or auto-flush behaviors. Roger On 5/21/21 5:23 PM, Rémi Forax wrote: On Thu, 20 May 2021 20:37:57 GMT, Roger Riggs wrote: OutputStreamWriter would be a better choice with that in mind. It does not have the convenience methods for converting various types to strings but would not hide the exceptions. Developers could wrap it in a PrintWriter to get the convenience and take on the responsibility of dealing with exceptions by polling. yes, instead of OutputStreamWriter, i wonder if BufferedWriter is not better given that reader part uses BufferedReader and that is mirror java.nio.file.Files which also uses BufferedReader/BufferedWriter as types for the pair reader/writer. - PR: https://git.openjdk.java.net/jdk/pull/4134
Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams
On Thu, 20 May 2021 20:37:57 GMT, Roger Riggs wrote: > OutputStreamWriter would be a better choice with that in mind. It does not > have the convenience methods for converting various types to strings but > would not hide the exceptions. Developers could wrap it in a PrintWriter to > get the convenience and take on the responsibility of dealing with exceptions > by polling. yes, instead of OutputStreamWriter, i wonder if BufferedWriter is not better given that reader part uses BufferedReader and that is mirror java.nio.file.Files which also uses BufferedReader/BufferedWriter as types for the pair reader/writer. - PR: https://git.openjdk.java.net/jdk/pull/4134
Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams
Hi,
The list is used to test the inputReader and errorReader methods that
accept a Charset.
The child process is launched with -Dsun.stdout.encoding and
-Dsun.stderr.encoding
to make the child encode its output in the requested charset (overriding
the native encoding).
Then the parent reads the output with the same charset.
The test will be skipped if the encoding is not supported.
The test for the native charset uses only the native encoding in the
default configuration.
Thanks, Roger
On 5/20/21 5:30 PM, Bernd Eckenfels wrote:
Hello,
Hm, how is that list used? - StandardCharaet.ISO_8859_1 is a guaranteed Charset
for JVM, and since the encoding is done in Java it should be fine. Added
benefit is, it’s 8bit transparent.
As for OS there is not a single standard charset (ebcdic vs latin families) but
ASCII is probably the widest available (with latin1 variants to follow)
--
https://Bernd.eckenfels.net
From: core-libs-dev on behalf of Roger Riggs
Sent: Thursday, May 20, 2021 10:52 PM
To: core-libs-dev@openjdk.java.net
Subject: Re: RFR: 8191441: (Process) add Readers and Writer access to
java.lang.Process streams
On Thu, 20 May 2021 20:42:35 GMT, Naoto Sato 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.
test/jdk/java/lang/ProcessBuilder/ReaderWriterTest.java line 64:
62: return new Object[][] {
63: {"UTF-8"},
64: {"ISO8859-1"},
`ISO8859-1` may not be available on all underlying OSes.
Is there a safe subset?
I haven't seen a failure yet, if/when it occurs, we make an exception or narrow
the test to known systems.
test/jdk/java/lang/ProcessBuilder/ReaderWriterTest.java line 111:
109: @Test(dataProvider = "CharsetCases", enabled = true)
110: void testCase(String nativeEncoding) throws IOException {
111: String osName =
System.getProperty("os.name").toLowerCase(Locale.ROOT);
Not used anywhere else.
Right, dead code now without host dependencies.
test/jdk/java/lang/ProcessBuilder/ReaderWriterTest.java line 122:
120: "ReaderWriterTest$ChildWithCharset");
121: var env = pb.environment();
122: env.put("LANG", "en_US." + cleanCSName);
Does this work on Windows?
Should be removed, the tests work because they set sun.stdout/stderr.encoding.
-
PR: https://git.openjdk.java.net/jdk/pull/4134
Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams
Hello,
Hm, how is that list used? - StandardCharaet.ISO_8859_1 is a guaranteed Charset
for JVM, and since the encoding is done in Java it should be fine. Added
benefit is, it’s 8bit transparent.
As for OS there is not a single standard charset (ebcdic vs latin families) but
ASCII is probably the widest available (with latin1 variants to follow)
--
https://Bernd.eckenfels.net
From: core-libs-dev on behalf of Roger
Riggs
Sent: Thursday, May 20, 2021 10:52 PM
To: core-libs-dev@openjdk.java.net
Subject: Re: RFR: 8191441: (Process) add Readers and Writer access to
java.lang.Process streams
On Thu, 20 May 2021 20:42:35 GMT, Naoto Sato 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.
>
> test/jdk/java/lang/ProcessBuilder/ReaderWriterTest.java line 64:
>
>> 62: return new Object[][] {
>> 63: {"UTF-8"},
>> 64: {"ISO8859-1"},
>
> `ISO8859-1` may not be available on all underlying OSes.
Is there a safe subset?
I haven't seen a failure yet, if/when it occurs, we make an exception or narrow
the test to known systems.
> test/jdk/java/lang/ProcessBuilder/ReaderWriterTest.java line 111:
>
>> 109: @Test(dataProvider = "CharsetCases", enabled = true)
>> 110: void testCase(String nativeEncoding) throws IOException {
>> 111: String osName =
>> System.getProperty("os.name").toLowerCase(Locale.ROOT);
>
> Not used anywhere else.
Right, dead code now without host dependencies.
> test/jdk/java/lang/ProcessBuilder/ReaderWriterTest.java line 122:
>
>> 120: "ReaderWriterTest$ChildWithCharset");
>> 121: var env = pb.environment();
>> 122: env.put("LANG", "en_US." + cleanCSName);
>
> Does this work on Windows?
Should be removed, the tests work because they set sun.stdout/stderr.encoding.
-
PR: https://git.openjdk.java.net/jdk/pull/4134
Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams
On Thu, 20 May 2021 20:46:36 GMT, Roger Riggs wrote:
>> test/jdk/java/lang/ProcessBuilder/ReaderWriterTest.java line 64:
>>
>>> 62: return new Object[][] {
>>> 63: {"UTF-8"},
>>> 64: {"ISO8859-1"},
>>
>> `ISO8859-1` may not be available on all underlying OSes.
>
> Is there a safe subset?
> I haven't seen a failure yet, if/when it occurs, we make an exception or
> narrow the test to known systems.
Lucky you :-)
I wasn't so lucky that I got an intermittent issue
(https://bugs.openjdk.java.net/browse/JDK-8265918), where it only fails if the
test is run on an Ubuntu CI test machine. It's not only depending on Linux
distros, but on the user's configuration (minimal install or full, etc.), so I
don't think there is any safe subset.
That said, the test code uses those encoding names as `sun.stdout/err.encoding`
properties values, not setting the real native encoding, so the test should be
fine. Only the argument name in the test (`nativeEncoding`) is a bit confusing.
-
PR: https://git.openjdk.java.net/jdk/pull/4134
Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams
On Thu, 20 May 2021 20:42:35 GMT, Naoto Sato 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.
>
> test/jdk/java/lang/ProcessBuilder/ReaderWriterTest.java line 64:
>
>> 62: return new Object[][] {
>> 63: {"UTF-8"},
>> 64: {"ISO8859-1"},
>
> `ISO8859-1` may not be available on all underlying OSes.
Is there a safe subset?
I haven't seen a failure yet, if/when it occurs, we make an exception or narrow
the test to known systems.
> test/jdk/java/lang/ProcessBuilder/ReaderWriterTest.java line 111:
>
>> 109: @Test(dataProvider = "CharsetCases", enabled = true)
>> 110: void testCase(String nativeEncoding) throws IOException {
>> 111: String osName =
>> System.getProperty("os.name").toLowerCase(Locale.ROOT);
>
> Not used anywhere else.
Right, dead code now without host dependencies.
> test/jdk/java/lang/ProcessBuilder/ReaderWriterTest.java line 122:
>
>> 120: "ReaderWriterTest$ChildWithCharset");
>> 121: var env = pb.environment();
>> 122: env.put("LANG", "en_US." + cleanCSName);
>
> Does this work on Windows?
Should be removed, the tests work because they set sun.stdout/stderr.encoding.
-
PR: https://git.openjdk.java.net/jdk/pull/4134
Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams
On Thu, 20 May 2021 19:57:22 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.
Good to see `native.encoding` is utilized. Some comments follow.
src/java.base/share/classes/java/lang/Process.java line 25:
> 23: * questions.
> 24: */
> 25:
Copyright year -> 2021
src/java.base/share/classes/java/lang/Process.java line 181:
> 179: * @return a {@link BufferedReader BufferedReader} using the {@code
> native.encoding} if supported,
> 180: * otherwise, the {@link Charset#defaultCharset()}
> 181: */
`@since 17` is needed (and for other public methods)
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?
test/jdk/java/lang/ProcessBuilder/ReaderWriterTest.java line 64:
> 62: return new Object[][] {
> 63: {"UTF-8"},
> 64: {"ISO8859-1"},
`ISO8859-1` may not be available on all underlying OSes.
test/jdk/java/lang/ProcessBuilder/ReaderWriterTest.java line 111:
> 109: @Test(dataProvider = "CharsetCases", enabled = true)
> 110: void testCase(String nativeEncoding) throws IOException {
> 111: String osName =
> System.getProperty("os.name").toLowerCase(Locale.ROOT);
Not used anywhere else.
test/jdk/java/lang/ProcessBuilder/ReaderWriterTest.java line 122:
> 120: "ReaderWriterTest$ChildWithCharset");
> 121: var env = pb.environment();
> 122: env.put("LANG", "en_US." + cleanCSName);
Does this work on Windows?
-
PR: https://git.openjdk.java.net/jdk/pull/4134
Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams
On Thu, 20 May 2021 20:12:07 GMT, Rémi Forax wrote: > I don't think that using PrintWriter is a good idea here given that a > PrintWriter shallow the IOException Good point, but... PrintStream does also and it is used frequently for Stdout and Stderr. OutputStreamWriter would be a better choice with that in mind. It does not have the convenience methods for converting various types to strings but would not hide the exceptions. Developers could wrap it in a PrintWriter to get the convenience and take on the responsibility of dealing with exceptions by polling. - PR: https://git.openjdk.java.net/jdk/pull/4134
Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams
On Thu, 20 May 2021 19:57:22 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. I've added the same comment on the bug itself. I don't think that using PrintWriter is a good idea here given that a PrintWriter shallow the IOException - PR: https://git.openjdk.java.net/jdk/pull/4134
RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams
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. - Commit messages: - 8191441: (Process) add Readers and Writer access to java.lang.Process streams Changes: https://git.openjdk.java.net/jdk/pull/4134/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4134=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8191441 Stats: 416 lines in 2 files changed: 412 ins; 0 del; 4 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
